HYPERFLEET-1090 - feat: WIFConfig plugin registration#198
Conversation
|
Hi @sherine-k. Thanks for your PR. I'm waiting for a openshift-hyperfleet member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Warning Review limit reached
More reviews will be available in 28 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR makes spec-schema discovery registry-driven and pluraI-keyed. It adds registry.WithSpecSchema(), refactors NewSchemaValidator to build schemas map by entity plural (with temporary clusters/nodepools seeding), converts middleware to regex-based rightmost-plural matching for POST/PATCH and to call Validate(plural,...), and registers a new WifConfig plugin + integration tests plus a permissive test validation-schema.yaml. Sequence DiagramsequenceDiagram
participant Client
participant Middleware as SchemaValidation<br/>Middleware
participant Registry
participant Validator as SchemaValidator
participant Handler
Client->>Middleware: POST /clusters/{id}/nodepools<br/>{"spec": {...}}
Middleware->>Registry: WithSpecSchema()
Registry-->>Middleware: [Cluster, NodePool, WifConfig, ...]
Middleware->>Middleware: shouldValidateRequest(method, path, entities)<br/>&`#x2192`; "nodepools"
Middleware->>Validator: Validate("nodepools", spec)
Validator->>Validator: Look up schema by plural key
Validator-->>Middleware: nil (valid) or ValidationWithDetails
alt Valid
Middleware->>Handler: next()
Handler-->>Client: 201 Created
else Invalid
Middleware-->>Client: 400 ProblemDetails<br/>(HYPERFLEET-VAL-000)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1285 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api plugins/wifconfigs | +1 |
Computed by hyperfleet-risk-scorer
sherine-k
changed the title
|
/test validate-commits |
sherine-k
changed the title
|
/test validate-commits |
| ) | ||
|
|
||
| const ( | ||
| kindWifConfig = "Wifconfig" |
There was a problem hiding this comment.
I wonder if the kind should be WifConfig or even WIFConfig
We have nodepools with kind being NodePool
But, in this case WIF is an acronym for "Workload Identity Federation", so the uppercase could also be considered valid
There was a problem hiding this comment.
In OCM we ended up pascalcasing everything, As we had var names like OCMAWSSTSConfig, instead we followed OcmAwsStsConfig to make it a bit easier.
Not sure if that is something we want to follow here. I do not envision we will have as many use cases as we had in OCM for this. But best to chose one and we capture it in our standards
There was a problem hiding this comment.
In this case we better using Pascal Case for consistency with other plugins
There was a problem hiding this comment.
Just note that this would be up to our partners to decide when they can configure their own generic API resources
There was a problem hiding this comment.
OK, in that case, I'll also sync with https://github.com/openshift-hyperfleet/hyperfleet-api-spec-template and PR#2 which we'd need to put in pascalcasing too?
cc @tirthct @ciaranRoche @rh-amarin
sherine-k
force-pushed
the
1090-WIFConfig
branch
4 times, most recently
from
ec791d2 to
dda3fa0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/validators/schema_validator_test.go`:
- Around line 580-596: The test helper functions setupTestValidator and
setupTestValidatorWithOptionalEntities need to call t.Helper() at the top so
test failures are reported at the caller site; update each function to begin
with t.Helper() (inside the function body, before any t.Fatalf calls or other
test assertions) to mark them as helpers and improve failure location reporting.
In `@test/integration/wifconfigs_test.go`:
- Around line 63-70: The test currently discards the error return from
resty.Post and asserts on resp2; update the call to capture the error (e.g.,
resp2, err := resty.R()....Post(h.RestURL(wifConfigsPath))) and assert that err
is nil before checking resp2.StatusCode() so transport failures fail the test
clearly; if err is non-nil, fail the test (Expect(err).To(BeNil()) or t.Fatalf)
and only then assert that resp2.StatusCode() equals http.StatusBadRequest for
the non-object spec case (references: resp2, err, resty.R().Post,
invalidTypeJSON, h.RestURL(wifConfigsPath), wifConfigsPath).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 16f5ec3f-6519-409c-b646-28717b4845dd
📒 Files selected for processing (14)
cmd/hyperfleet-api/main.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_match_test.gopkg/middleware/schema_validation_test.gopkg/registry/registry.gopkg/registry/registry_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.goplugins/wifconfigs/plugin.gotest/CLAUDE.mdtest/integration/clusters_test.gotest/integration/integration_test.gotest/integration/wifconfigs_test.gotest/validation-schema.yaml
…idation
Register WIFConfig as a generic resource plugin using the new ResourceHandler pattern,
with spec validation driven by the entity registry rather than hardcoded resource types.
- Add WIFConfig plugin with CRUD routes and registry descriptor
- Refactor SchemaValidationMiddleware to discover entities from registry.WithSpecSchema()
- Refactor SchemaValidator to dynamically load schemas for all registered entities
- Add rightmost-match path logic so nested routes (e.g. /clusters/{id}/nodepools) use the child schema
- Add registry.WithSpecSchema() and registry.Reset() helpers
- Add permissive test/validation-schema.yaml for integration tests
- Add WIFConfig integration tests covering CRUD, schema validation, and error details
- Add schema_validation_match_test.go for nested path disambiguation
- Extend schema_validator_test.go with registry-driven validation coverage
|
|
||
| // shouldValidateRequest determines if the request requires spec validation | ||
| // Returns (shouldValidate bool, resourceType string) | ||
| func shouldValidateRequest(method, path string) (bool, string) { |
There was a problem hiding this comment.
Could name the return values to be inline with the comment:
func shouldValidateRequest(method, path string) (shouldValidate bool, resourcePlural string)
There was a problem hiding this comment.
But I don't know if we follow that go-ism here, so might be unnecessary.
| // | ||
| // When a path contains multiple registered plurals (e.g. /clusters/{id}/nodepools), | ||
| // the rightmost matching segment wins so nested resources use their own spec schema. | ||
| func shouldValidateRequest( |
There was a problem hiding this comment.
I wonder if it will be more clear to use a regex here to validate that giving a , the URL will match ending in
<plural><plural>/<plural>/<uuid>
In go
func buildRegex(word string) *regexp.Regexp {
pattern := fmt.Sprintf(
`/%s(?:/?|/[0-9a-fA-F-]{36})$`,
regexp.QuoteMeta(word),
)
return regexp.MustCompile(pattern)
}
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit ef2f472
into
openshift-hyperfleet:main
5 participants
Summary
This PR adds a plugin to register WIFConfig as an entity at API startup.
It also introduces schema validation for registered resources (channels, versions, wifConfigs) that have a
specdefined (provided by an external openapi schema).This PR also aligns the kind naming convention to PascalCase with hyperfleet-api-spec-template [PR#5}(https://github.com/openshift-hyperfleet/hyperfleet-api-spec-template/pull/5)
Out of Scope: Deletion protection (preventing accidental removal of WIF configs that active clusters depend on)
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)