HYPERFLEET-1158 - feat: config-driven entity registration via LoadDescriptors#267
HYPERFLEET-1158 - feat: config-driven entity registration via LoadDescriptors#267tirthct wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (26)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughSummary by CodeRabbit
WalkthroughConfig-driven entity descriptors now flow from Helm values and example config into Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCmd as servecmd.runServe
participant Registry as pkg/registry
participant EntitiesPlugin as plugins/entities
participant Router as API router
ServeCmd->>Registry: LoadDescriptors(cfg.Entities)
ServeCmd->>Registry: Validate()
ServeCmd->>Registry: ValidateSchemas(schemaPath)
ServeCmd->>EntitiesPlugin: init() during startup
EntitiesPlugin->>Registry: All()
Registry-->>EntitiesPlugin: descriptors
EntitiesPlugin->>Router: register top-level and nested CRUD routes
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/registry/registry.go (2)
86-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecompose
Validate()— now over 50 lines with 7+ branches.Adding the reference checks pushed
Validate()well past the size/branching threshold. Extract the reference-integrity checks (target-kind, duplicateref_type, max<min) into a helper, e.g.validateReferences(d EntityDescriptor).As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
♻️ Suggested decomposition
+func validateReferences(d EntityDescriptor) { + refTypes := make(map[string]bool, len(d.References)) + for _, ref := range d.References { + if _, ok := descriptors[ref.TargetKind]; !ok { + panic(fmt.Sprintf( + "entity %q: reference %q targets unregistered kind %q", + d.Kind, ref.RefType, ref.TargetKind, + )) + } + if refTypes[ref.RefType] { + panic(fmt.Sprintf( + "entity %q: duplicate ref_type %q in references", + d.Kind, ref.RefType, + )) + } + refTypes[ref.RefType] = true + if ref.Max > 0 && ref.Max < ref.Min { + panic(fmt.Sprintf( + "entity %q: reference %q has max (%d) < min (%d)", + d.Kind, ref.RefType, ref.Max, ref.Min, + )) + } + } +} + func Validate() { plurals := make(map[string]string, len(descriptors)) for _, d := range descriptors { ... plurals[d.Plural] = d.Kind - // Track seen RefType values ... - refTypes := make(map[string]bool, len(d.References)) - for _, ref := range d.References { ... } + validateReferences(d) } }🤖 Prompt for 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. In `@pkg/registry/registry.go` around lines 86 - 138, The Validate() function now exceeds the size and branching threshold due to inline reference-integrity logic. Refactor pkg/registry/registry.go by extracting the per-entity reference checks from Validate() into a helper such as validateReferences(d EntityDescriptor), and move the target-kind lookup, duplicate ref_type detection, and max<min validation there while keeping Validate() focused on the top-level descriptor checks.Source: Path instructions
140-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "schema exists" check vs
pkg/validators/schema_validator.gobuildSchemasMap.Both
ValidateSchemashere andbuildSchemasMapload the OpenAPI doc and checkd.SpecSchemaNameagainstdoc.Components.Schemasfor every registered descriptor. The messages have already diverged ("not found in OpenAPI spec at %s" here vs "not found in OpenAPI spec" there) — evidence this is going to drift further. Extract a sharedcheckSchemaPresence(doc, descriptors)used by both, or drop one of the two enforcement points if they always run in the same startup path.🤖 Prompt for 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. In `@pkg/registry/registry.go` around lines 140 - 166, The OpenAPI schema existence validation is duplicated between ValidateSchemas and buildSchemasMap, which will keep drifting in behavior and error messaging. Refactor the shared schema-presence logic into a common helper that checks each descriptor’s SpecSchemaName against doc.Components.Schemas and is used by both ValidateSchemas and pkg/validators/schema_validator.go’s buildSchemasMap, or remove one enforcement point if startup already guarantees the other runs.pkg/validators/schema_validator_test.go (1)
175-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test + misleading name.
TestValidate_ErrorsWhenRegisteredEntitySchemaNotLoaded(248-287) is functionally identical toTestNewSchemaValidator_RegisteredEntityMissingOpenAPISchema_Errors(175-215) — same registry setup, same fixture, same assertions, and it never actually callsValidate()(construction fails inNewSchemaValidatorfirst, so the name misrepresents what's tested). Consolidate into one test, or convert the four "missing schema" variants (126-152, 175-215, 217-246, 248-287) into a single table-driven test witht.Run().As per path instructions: "Table-driven tests with t.Run() for repeated patterns" and "Test names describe the scenario, not the implementation."
Also applies to: 248-287
🤖 Prompt for 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. In `@pkg/validators/schema_validator_test.go` around lines 175 - 215, Remove the duplicate “missing schema” test coverage by consolidating TestNewSchemaValidator_RegisteredEntityMissingOpenAPISchema_Errors with TestValidate_ErrorsWhenRegisteredEntitySchemaNotLoaded, since they use the same registry setup, fixture, and assertions. Rename or restructure the remaining test so the name matches the actual behavior in NewSchemaValidator (construction failing before Validate is reached), and consider folding the four schema-missing cases into a single table-driven test with t.Run() to cover each scenario once.Source: Path instructions
plugins/entities/plugin_test.go (1)
33-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test for unresolvable
ParentKind— the exact panic path inplugin.go.Add a case registering a child descriptor whose
ParentKindhas no matching registeredKind, and assert the documented behavior (Expect(func(){...}).To(Panic())if panic is intended, or an error return ifMustGetis replaced with a guarded lookup per the companion comment onplugin.go). Right now this failure mode is untested and undocumented.As per path instructions for
**/*_test.go: "Error paths SHOULD be tested, not just happy paths."🤖 Prompt for 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. In `@plugins/entities/plugin_test.go` around lines 33 - 59, Add an error-path test in RegisterEntityRoutes coverage for an unresolvable ParentKind: register a child EntityDescriptor whose ParentKind has no matching registered Kind, then assert the behavior at the plugin.go lookup path (either a panic via Expect(...).To(Panic()) if RegisterEntityRoutes/GetParentKind still uses MustGet, or an error result if that lookup is changed to be guarded). Place the new case alongside TestRegisterEntityRoutes_ChildEntity_NestedOnly so the failure mode is explicitly covered with the existing registry/RegisterEntityRoutes helpers.Source: Path instructions
🤖 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 `@charts/templates/configmap.yaml`:
- Around line 145-153: The entities block in the configmap template is rendering
several string fields without quoting, which can produce malformed YAML or type
coercion when values are overridden. Update the entity entries in the template
to pipe `kind`, `plural`, `spec_schema_name`, `parent_kind`, `on_parent_delete`,
`ref_type`, and `target_kind` through `| quote`, matching the existing pattern
used for other settings, so `LoadDescriptors` receives valid string values.
In `@cmd/hyperfleet-api/servecmd/cmd.go`:
- Line 22: The registry validation path in runServe currently lets
`registry.LoadDescriptors`, `Validate`, and `ValidateSchemas` panic, which
bypasses the existing fail-fast logging flow. Add a local `recover()` around
those calls in `cmd/hyperfleet-api/servecmd/cmd.go` and convert any panic into a
`logger.WithError(...).Error(...)` followed by `os.Exit(1)`, matching the
handling used for config load and `Initialize()`. Use the `runServe` function
and the `registry` package calls as the main points to update, and keep the
startup behavior consistent with the rest of the command.
In `@plugins/CLAUDE.md`:
- Around line 3-25: The registry reference path in the docs is stale and should
point to the actual loader location used by this PR. Update the “Reference”
entry under the config-driven entities section to use the correct registry
symbol location, `pkg/registry/registry.go`, so it matches the implementation.
Keep the surrounding guidance intact and ensure any other references to the
loader path in this doc are consistent with the same symbol.
---
Nitpick comments:
In `@pkg/registry/registry.go`:
- Around line 86-138: The Validate() function now exceeds the size and branching
threshold due to inline reference-integrity logic. Refactor
pkg/registry/registry.go by extracting the per-entity reference checks from
Validate() into a helper such as validateReferences(d EntityDescriptor), and
move the target-kind lookup, duplicate ref_type detection, and max<min
validation there while keeping Validate() focused on the top-level descriptor
checks.
- Around line 140-166: The OpenAPI schema existence validation is duplicated
between ValidateSchemas and buildSchemasMap, which will keep drifting in
behavior and error messaging. Refactor the shared schema-presence logic into a
common helper that checks each descriptor’s SpecSchemaName against
doc.Components.Schemas and is used by both ValidateSchemas and
pkg/validators/schema_validator.go’s buildSchemasMap, or remove one enforcement
point if startup already guarantees the other runs.
In `@pkg/validators/schema_validator_test.go`:
- Around line 175-215: Remove the duplicate “missing schema” test coverage by
consolidating TestNewSchemaValidator_RegisteredEntityMissingOpenAPISchema_Errors
with TestValidate_ErrorsWhenRegisteredEntitySchemaNotLoaded, since they use the
same registry setup, fixture, and assertions. Rename or restructure the
remaining test so the name matches the actual behavior in NewSchemaValidator
(construction failing before Validate is reached), and consider folding the four
schema-missing cases into a single table-driven test with t.Run() to cover each
scenario once.
In `@plugins/entities/plugin_test.go`:
- Around line 33-59: Add an error-path test in RegisterEntityRoutes coverage for
an unresolvable ParentKind: register a child EntityDescriptor whose ParentKind
has no matching registered Kind, then assert the behavior at the plugin.go
lookup path (either a panic via Expect(...).To(Panic()) if
RegisterEntityRoutes/GetParentKind still uses MustGet, or an error result if
that lookup is changed to be guarded). Place the new case alongside
TestRegisterEntityRoutes_ChildEntity_NestedOnly so the failure mode is
explicitly covered with the existing registry/RegisterEntityRoutes helpers.
🪄 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: b4ddec05-b9c5-42a8-85b5-5df773428538
📒 Files selected for processing (26)
CLAUDE.mdcharts/CLAUDE.mdcharts/README.mdcharts/templates/configmap.yamlcharts/values.schema.jsoncharts/values.yamlcmd/hyperfleet-api/main.gocmd/hyperfleet-api/servecmd/cmd.goconfigs/config.yaml.examplepkg/config/config.gopkg/config/dump.gopkg/config/loader.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/registry/registry_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.goplugins/CLAUDE.mdplugins/channels/plugin.goplugins/entities/plugin.goplugins/entities/plugin_test.goplugins/versions/plugin.goplugins/wifconfigs/plugin.gotest/helper.gotest/integration/versions_test.gotest/integration/wifconfigs_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (5)
- plugins/wifconfigs/plugin.go
- plugins/versions/plugin.go
- plugins/channels/plugin.go
- test/integration/wifconfigs_test.go
- test/integration/versions_test.go
| - kind: {{ .kind }} | ||
| plural: {{ .plural }} | ||
| {{- if .parent_kind }} | ||
| parent_kind: {{ .parent_kind }} | ||
| {{- end }} | ||
| {{- if .on_parent_delete }} | ||
| on_parent_delete: {{ .on_parent_delete }} | ||
| {{- end }} | ||
| spec_schema_name: {{ .spec_schema_name }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unquoted entity scalars — YAML injection/malformed-config risk (CWE-1284-adjacent).
Every other string value in this template is piped through | quote (see hostname, cert_file, issuer_url, dialect, level, etc.). The new entities block does not quote kind, plural, spec_schema_name, parent_kind, on_parent_delete, ref_type, target_kind. Since config.entities is documented as overridable via --set-json, an operator-supplied value containing :, or a bare yes/no/null-like token, will corrupt the rendered YAML or get silently reinterpreted as a non-string type, breaking LoadDescriptors at startup.
🔒 Proposed fix
- kind: {{ .kind }}
- plural: {{ .plural }}
+ - kind: {{ .kind | quote }}
+ plural: {{ .plural | quote }}
{{- if .parent_kind }}
- parent_kind: {{ .parent_kind }}
+ parent_kind: {{ .parent_kind | quote }}
{{- end }}
{{- if .on_parent_delete }}
- on_parent_delete: {{ .on_parent_delete }}
+ on_parent_delete: {{ .on_parent_delete | quote }}
{{- end }}
- spec_schema_name: {{ .spec_schema_name }}
+ spec_schema_name: {{ .spec_schema_name | quote }}
@@
- - ref_type: {{ .ref_type }}
- target_kind: {{ .target_kind }}
+ - ref_type: {{ .ref_type | quote }}
+ target_kind: {{ .target_kind | quote }}Also applies to: 169-170
🤖 Prompt for 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.
In `@charts/templates/configmap.yaml` around lines 145 - 153, The entities block
in the configmap template is rendering several string fields without quoting,
which can produce malformed YAML or type coercion when values are overridden.
Update the entity entries in the template to pipe `kind`, `plural`,
`spec_schema_name`, `parent_kind`, `on_parent_delete`, `ref_type`, and
`target_kind` through `| quote`, matching the existing pattern used for other
settings, so `LoadDescriptors` receives valid string values.
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/health" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/metrics" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Panic-based startup validation breaks the established fail-fast pattern (CWE-248).
registry.LoadDescriptors/Validate/ValidateSchemas all panic() on bad config (per pkg/registry/registry.go), but every other failure path in this same function (config load, Initialize()) uses logger.WithError(ctx, err).Error(...) + os.Exit(1). Since this call happens before initLogger() (line 74), a malformed config.entities (e.g. bad Helm --set-json) crashes with a raw Go stack trace on stderr instead of the app's structured logger/format — inconsistent operator experience and harder to correlate in log aggregation. Wrap the calls with a local recover() and route through logger.WithError(...).Error(...) + os.Exit(1) for parity with the rest of runServe, or have the registry package return error instead of panicking.
🛡️ Proposed fix
- registry.LoadDescriptors(cfg.Entities)
- registry.Validate()
- registry.ValidateSchemas(cfg.Server.OpenAPISchemaPath)
+ func() {
+ defer func() {
+ if r := recover(); r != nil {
+ logger.With(ctx, "panic", r).Error("Entity registry validation failed")
+ os.Exit(1)
+ }
+ }()
+ registry.LoadDescriptors(cfg.Entities)
+ registry.Validate()
+ registry.ValidateSchemas(cfg.Server.OpenAPISchemaPath)
+ }()Also applies to: 59-65
🤖 Prompt for 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.
In `@cmd/hyperfleet-api/servecmd/cmd.go` at line 22, The registry validation path
in runServe currently lets `registry.LoadDescriptors`, `Validate`, and
`ValidateSchemas` panic, which bypasses the existing fail-fast logging flow. Add
a local `recover()` around those calls in `cmd/hyperfleet-api/servecmd/cmd.go`
and convert any panic into a `logger.WithError(...).Error(...)` followed by
`os.Exit(1)`, matching the handling used for config load and `Initialize()`. Use
the `runServe` function and the `registry` package calls as the main points to
update, and keep the startup behavior consistent with the rest of the command.
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1005 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api cmd/hyperfleet-api/servecmd pkg/config plugins/channels plugins/versions plugins/wifconfigs test | +1 |
Computed by hyperfleet-risk-scorer
1 participant
Summary
Moves generic resource entity registration (Channel, Version, WifConfig) from
compile-time Go plugin
init()functions to the application YAML config. Adding anew generic entity type now requires a config entry, a matching OpenAPI spec schema,
and a redeploy — no new Go plugin or route wiring.
Cluster and NodePool remain on their legacy typed plugins and are unchanged.
What changed
entitiesconfig section —ApplicationConfigunmarshalsentities:YAMLentries directly into
[]registry.EntityDescriptorvia mapstructure tagsLoadDescriptors+Validate+ValidateSchemas— called inEnv.Initialize()before routes are built; panics on unknownparent_kind,duplicate
plural, missing OpenAPI spec schemas, and invalid reference constraintsplugins/entities/plugin.goreplaces per-entity plugins;iterates
registry.All()and auto-generates top-level routes for root entities andnested routes for child entities
buildSchemasMap()now errors (not warns) when a registeredentity's
spec_schema_nameis missing from the OpenAPI specReferenceDescriptor— added toEntityDescriptorsoreferencesconfigsub-field is parseable (implementation in HYPERFLEET-1156)
DumpConfig— shows entity count and kind names in startup logsvalues.yamlincludes Channel/Version/WifConfig;configmap.yamltemplates all entity fields explicitly;
values.schema.jsonvalidates entity structureDeleted
plugins/channels/plugin.goplugins/versions/plugin.goplugins/wifconfigs/plugin.goDesign notes
plugins/channels,plugins/versions,plugins/wifconfigsregister descriptors + routes ininit()plugins/entitiesplugin; descriptors from YAMLSpecSchemaName/SearchDisallowedFieldsstored but unused at validation/search layerSpecSchemaNamevalidated at startup viaValidateSchemas;SearchDisallowedFieldsparsed but enforcement deferred (seedocs/search-disallowed-fields-gap.md)Example config:
Out of scope
SearchDisallowedFieldsenforcement at the search layer — pre-existing gap documentedin
docs/search-disallowed-fields-gap.md, requires per-queryListArgumentsplumbingTest plan
make verify-all— 1311 tests passmake test-helm— chart lint + template validationLoadDescriptors,ValidateSchemas,RegisterEntityRoutes,reference validation in
Validate()make test-integration— channels/versions/wifconfigs CRUD with config-drivenregistration (requires database)