feat(oaslint): add oaslint#371
Conversation
…tion status fields
…cation validation
Co-authored-by: Copilot <copilot@github.com>
… spec Co-authored-by: Copilot <copilot@github.com>
…andler logic Co-authored-by: Copilot <copilot@github.com>
…guration handling Co-authored-by: Copilot <copilot@github.com>
…itelistedBasepaths Co-authored-by: Copilot <copilot@github.com>
…sepaths description
…tion logic Co-authored-by: Copilot <copilot@github.com>
…ort and improved HTTP client handling Co-authored-by: Copilot <copilot@github.com>
ron96g
left a comment
There was a problem hiding this comment.
There are some design decision in here that I dont really like. Lets talk about this.
I have added a few comments with questions, feedback is welcome
| // When set, linting is enabled for this category. | ||
| // +kubebuilder:validation:Format=uri | ||
| // +optional | ||
| URL string `json:"url,omitempty"` |
There was a problem hiding this comment.
This will probably be the same for all Categories :/
There was a problem hiding this comment.
I've moved to Rover-Server now, alongside the dashboard URL, as it is a different endpoint compared to API endpoint
There was a problem hiding this comment.
Pull request overview
Adds OpenAPI (OAS) linting support to the rover-server upload flow and propagates lint outcomes into the Rover ApiSpecification lifecycle, including optional blocking behavior based on ApiCategory configuration.
Changes:
- Introduces an external OAS linter client + API-lint orchestration (ruleset support, dashboard links, basepath whitelist, hash-based reuse).
- Extends
ApiSpecificationwith aspec.lintresult field and uses it in the rover controller to gateApicreation when category linting mode isBlock. - Updates
ApiCategorylinting configuration model (mode/whitelist) and wires in ApiCategory storage/indexing/RBAC needed for lookups.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| rover/internal/index/index.go | Adds a shared field-index key constant for ApiCategory labelValue lookup. |
| rover/internal/handler/apispecification/suite_test.go | Adds Ginkgo suite for ApiSpecification handler tests. |
| rover/internal/handler/apispecification/handler.go | Gates Api creation based on ApiSpecification.spec.lint and category linting mode. |
| rover/internal/handler/apispecification/handler_test.go | Adds tests for the lint gating behavior in the handler. |
| rover/internal/controller/index.go | Registers a field index on ApiCategory.spec.labelValue (lowercased). |
| rover/internal/controller/apispecification_controller.go | Adjusts imports/setup and adds RBAC annotation changes. |
| rover/config/crd/bases/rover.cp.ei.telekom.de_apispecifications.yaml | Extends CRD schema with spec.lint. |
| rover/api/v1/zz_generated.deepcopy.go | Updates deepcopy generation for new Lint fields/types. |
| rover/api/v1/apispecification_types.go | Adds Lint *LintResult to ApiSpecificationSpec and defines LintResult. |
| rover-server/test/mocks/mocks_ApiCategory.go | Adds ApiCategory store mock for rover-server tests. |
| rover-server/pkg/store/stores.go | Adds APICategoryStore to rover-server store bundle. |
| rover-server/internal/oaslint/suite_test.go | Adds Ginkgo suite for oaslint package tests. |
| rover-server/internal/oaslint/noop.go | Adds a no-op linter implementation (always passes). |
| rover-server/internal/oaslint/linter.go | Defines the linter interface and its result type for rover-server. |
| rover-server/internal/oaslint/external.go | Implements external linter HTTP client (Atlas-compatible scan endpoint). |
| rover-server/internal/oaslint/external_test.go | Adds tests for external/noop linter behavior. |
| rover-server/internal/mapper/status/response.go | Changes timestamp mapping behavior for status responses. |
| rover-server/internal/mapper/apispecification/in/snapshots/apispecification_test.snap | Updates snapshot to include the new lint field. |
| rover-server/internal/controller/suite_controller_test.go | Updates controller test wiring for new ApiSpecificationController constructor. |
| rover-server/internal/controller/apispecification.go | Adds linting + ApiCategory validation/lookup into ApiSpecification update flow. |
| rover-server/internal/controller/apispecification_lint_test.go | Adds unit tests for lint helper logic and lint result reuse behavior. |
| rover-server/internal/controller/apilinter.go | Adds the ApiSpecification lint orchestration (mode handling, whitelist, dashboard URL). |
| rover-server/internal/config/config.go | Adds OAS linting configuration and defaults. |
| rover-server/config/rbac/role.yaml | Grants rover-server RBAC to list/watch ApiCategories. |
| rover-server/cmd/main.go | Wires OAS linting config into rover-server controller creation. |
| api/internal/controller/apicategory_controller_test.go | Updates ApiCategory controller test data after linting model changes. |
| api/config/crd/bases/api.cp.ei.telekom.de_apicategories.yaml | Updates ApiCategory CRD schema for linting mode + whitelisted basepaths. |
| api/api/v1/zz_generated.deepcopy.go | Fixes deepcopy behavior for linting config (slice deep copy). |
| api/api/v1/apicategory_types.go | Replaces enabled with mode and adds whitelisted basepaths to linting config. |
Files not reviewed (2)
- api/api/v1/zz_generated.deepcopy.go: Language not supported
- rover/api/v1/zz_generated.deepcopy.go: Language not supported
Comments suppressed due to low confidence (3)
rover-server/internal/mapper/status/response.go:70
- These timestamps were previously normalized to UTC (via
.UTC()); removing that changes the API’s serialized time zone behavior and makes it inconsistent with other responses that still call.UTC()elsewhere in rover-server. Consider restoring.UTC()(or ensure a single consistent convention for all timestamp fields).
processing := meta.FindStatusCondition(apiSpec.GetConditions(), condition.ConditionTypeProcessing)
var processedAtTime time.Time
if processing != nil {
processedAtTime = processing.LastTransitionTime.Time
}
parentOverall := CalculateOverallStatus(status.State, status.ProcessingState)
finalOverall := CompareAndReturn(parentOverall, result.WorstOverallStatus)
return api.ResourceStatusResponse{
CreatedAt: apiSpec.GetCreationTimestamp().Time,
ProcessedAt: processedAtTime,
State: status.State,
rover-server/internal/mapper/status/response.go:108
- These timestamps were previously normalized to UTC (via
.UTC()); removing that changes the API’s serialized time zone behavior and makes it inconsistent with other responses that still call.UTC()elsewhere in rover-server. Consider restoring.UTC()(or ensure a single consistent convention for all timestamp fields).
processing := meta.FindStatusCondition(rover.GetConditions(), condition.ConditionTypeProcessing)
var processedAtTime time.Time
if processing != nil {
processedAtTime = processing.LastTransitionTime.Time
}
parentOverall := CalculateOverallStatus(status.State, status.ProcessingState)
finalOverall := CompareAndReturn(parentOverall, result.WorstOverallStatus)
return api.ResourceStatusResponse{
CreatedAt: rover.GetCreationTimestamp().Time,
ProcessedAt: processedAtTime,
State: status.State,
rover-server/internal/mapper/status/response.go:146
- These timestamps were previously normalized to UTC (via
.UTC()); removing that changes the API’s serialized time zone behavior and makes it inconsistent with other responses that still call.UTC()elsewhere in rover-server. Consider restoring.UTC()(or ensure a single consistent convention for all timestamp fields).
processing := meta.FindStatusCondition(eventSpec.GetConditions(), condition.ConditionTypeProcessing)
var processedAtTime time.Time
if processing != nil {
processedAtTime = processing.LastTransitionTime.Time
}
parentOverall := CalculateOverallStatus(status.State, status.ProcessingState)
finalOverall := CompareAndReturn(parentOverall, result.WorstOverallStatus)
return api.ResourceStatusResponse{
CreatedAt: eventSpec.GetCreationTimestamp().Time,
ProcessedAt: processedAtTime,
State: status.State,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…into feat/oaslint
ron96g
left a comment
There was a problem hiding this comment.
Keeping it simple for the start by using sync-mode is good. However, some minor comments + the question if the code-changes in Rover server any purpose at this point
| // Lint performs the full linting lifecycle for an ApiSpecification. | ||
| // It looks up the linting config from the category list, checks whitelists, | ||
| // and runs the linter synchronously. | ||
| Lint(ctx context.Context, apiSpec *roverv1.ApiSpecification, category *apiv1.ApiCategory, specBytes []byte) (LintOutcome, error) |
There was a problem hiding this comment.
Minor: Normally you would pass []byte via io.Reader interfaces. It a bit nicer
|
|
||
| // apiLinterImpl is the production implementation of ApiLinter. | ||
| type apiLinterImpl struct { | ||
| errorMessage string |
There was a problem hiding this comment.
On first look this confused me a bit, but this basically is the errorMessage returned if the lint fails? Maybe name it errorMessageTemplate or add a comment
| // Check if linting failed and the category config blocks on failure. | ||
| // If Spec.Lint is nil (no result yet or linting not configured), proceed normally | ||
| // to avoid blocking indefinitely if the linter is unavailable. | ||
| if apiSpec.Spec.Lint != nil && !apiSpec.Spec.Lint.Passed && mode == apiapi.LintingModeBlock { |
There was a problem hiding this comment.
Is this logic in here still needed? I thought in sync-mode it gets blocked in rover-server
No description provided.