ROSAENG-61802: feat: field validation, conversion, openapi gen - #197
ROSAENG-61802: feat: field validation, conversion, openapi gen#197cdoan1 wants to merge 19 commits into
Conversation
Port the build-time code generators from cdoan1/hyperfleet-api-codegen into the monorepo under hack/api-codegen/. Includes 7 generator commands, 7 library packages, Makefile integration, and codegen integration plan. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Bump go directive to 1.26.5 in hack/api-codegen and hack/tools - Fix conversion generator: conditional imports, OutputDir-derived package names, duplicate helper prevention, go/printer fallback for unsupported AST expressions - Fix CRD variant generator: encoder.Close(), file close error propagation, hidden field exclusion, transparent items/additionalProperties - Fix marker scanner: per-directory type cache, cycle guard for recursive structs - Fix OpenAPI generator: shared resolveTypeSchema helper, pointer/slice stripping for array elements and map values - Fix passthrough generator: repeated pointer/slice prefix stripping - Fix validator: explicit matched flag for feature-gate overrides - Move codegen binaries to repo-root bin/ for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Avoid mutating g.FieldPrefix so each type derives its own prefix
independently when processing multiple types
- Render interface, func, ellipsis, and generic AST expressions via
go/printer instead of silently dropping to interface{}
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add generated passthrough structs (HostedClusterSpecPassthrough, NodePoolSpecPassthrough) with per-field write-mode, visibility, and feature-gate markers to v1alpha1 CRD types. Add envelope fields (DisplayName, DeleteProtection, Tags, etc.) to ClusterSpec and NodePoolSpec. Add runtime libraries for field registry, feature gates, and service-set conversion. Add Makefile codegen pipeline targets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use --source-dir with go list from hyperfleet-operator/api/ (which has the HyperShift dependency) instead of --import-path from repo root. Remove the conflicting zz_generated.passthrough.go after generation since the curated hostedclusterspec.passthrough.go is the authoritative file. Regenerate registry from v1alpha1 markers (75 fields). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The collectImports function iterated a map without sorting, causing non-deterministic import ordering in the generated .raw file across runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ypes The stale deepcopy caused integration test failures — controller-runtime silently dropped new fields during reconciliation, preventing status propagation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Align openshift/hypershift/api and transitive deps with the rest of the monorepo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the embedded registry from passthrough-gen and make --registry a required flag. The Makefile now passes the runtime registry (platform-api/internal/codegen/registry/field_metadata.json) directly, eliminating the stale-copy problem. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
isRootType excluded *Passthrough types, so the scanner never walked their fields into the registry. This meant curating markers on passthrough fields (e.g. flipping openapi-gen to true) had no effect on the generated registry. Treat Passthrough types as scan roots, bringing the registry from 75 to 119 fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UBI go-toolset defaults GOCACHE to /opt/app-root/src/.cache/go-build which is not writable as user 1001. Create /tmp/gocache and set GOCACHE to point there. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Enforce write-mode (mutable/immutable/service-set) and feature-gate rules on cluster and nodepool Create/Update using the generated field registry. Returns 422 with field-level error details on violation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…onversion functions Rewrites the dead map-based conversion code with typed struct functions and wires them into the cluster and nodepool handlers. Also fixes a bug where ApplyPlatformUpdateToClusterCR silently wiped CreatorARN, AccountID, and InternalID on updates — only IssuerURL was previously preserved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add openapi-merge tool that reads Swagger 2.0 definitions from openapi-gen, converts $ref paths to OAS 3.0, strips marker descriptions, inlines self-referential refs, and replaces CRD schemas in openapi.yaml. ClusterSpec/NodePoolSpec now show only visible fields — hidden service-set fields (creatorARN, accountID, internalID, etc.) are excluded. Sub-type schemas (ClusterConfiguration, KubeletConfig, MachineConfigSpec, HostedClusterSpecPassthrough) are generated and merged automatically. New Makefile targets: codegen-openapi, verify-openapi. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-61802 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1 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-ci
Bot
added
the
approved
WalkthroughAdds an API codegen module with marker scanning, passthrough and OpenAPI generation, conversion helpers, feature-gated field metadata, request validation, service-set preservation, expanded API models, and Makefile integration. ChangesAPI codegen and generated contracts
Runtime validation and API handling
Build and OpenAPI integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
hack/api-codegen/README.md-10-26 (1)
10-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTable and binary count omit
openapi-merge.
make build-api-codegenbuilds 8 binaries (includingopenapi-merge, used bycodegen-openapi), but the table lists 7 and line 23 says "all 7".📝 Proposed doc fix
| `verify-configuration` | Validate marker consistency across types | +| `openapi-merge` | Merge generated schemas into `platform-api/openapi/openapi.yaml` | ## Usage ```bash -make build-api-codegen # Build all 7 generator binaries +make build-api-codegen # Build all 8 generator binaries🤖 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 `@hack/api-codegen/README.md` around lines 10 - 26, Update the api-codegen command table to include the openapi-merge binary and change the make build-api-codegen usage comment from “all 7” to “all 8” generators. Keep the existing descriptions and other command documentation unchanged.hack/api-codegen/README.md-3-3 (1)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the README link target and make the coverage badge dynamic.
hack/api-codegen/README.mdresolves./docs/api/api-management.mdtohack/api-codegen/docs/api/api-management.md, but the file lives atdocs/api/api-management.md, so the link is broken. The31.3%coverage badge is also hardcoded and will drift frommake coverage-api-codegen.🤖 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 `@hack/api-codegen/README.md` at line 3, Update the README coverage badge to use the dynamic coverage output produced by make coverage-api-codegen instead of hardcoding 31.3%, and correct the api-management documentation link target so it resolves to the repository-level docs/api/api-management.md location.hack/api-codegen/pkg/passthrough/loader.go-252-259 (1)
252-259: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the empty base name before indexing.
deriveFieldPrefix("Spec")trims to""and panics atrunes[0]. Return no prefix whenbaseis empty.🤖 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 `@hack/api-codegen/pkg/passthrough/loader.go` around lines 252 - 259, Update deriveFieldPrefix to check whether the trimmed base name is empty before converting it to runes or indexing runes[0]; return no prefix for an input of "Spec", while preserving the existing behavior for other type names.hack/api-codegen/pkg/validation/validator.go-79-90 (1)
79-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the runtime validator in sync.
hack/api-codegen/pkg/validation/validator.gois a separate copy ofplatform-api/pkg/validation/field_validator.go, so any fix to the immutable-field comparison here needs the same change in the runtime path or the bug will persist there.🤖 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 `@hack/api-codegen/pkg/validation/validator.go` around lines 79 - 90, Apply the same immutable-field comparison fix from the runtime validator to the corresponding validation logic in Validator, ensuring hack/api-codegen/pkg/validation/validator.go remains behaviorally identical to platform-api/pkg/validation/field_validator.go. Locate the immutable-field validation method or comparison and update it consistently without changing unrelated registry or constructor code.platform-api/pkg/validation/field_validator_test.go-60-76 (1)
60-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing coverage for "immutable field resubmitted unchanged."
See consolidated comment (anchored on
platform-api/pkg/validation/field_validator.go#L95-158) for the underlying bug this test gap masks.🤖 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 `@platform-api/pkg/validation/field_validator_test.go` around lines 60 - 76, The validation tests lack coverage for resubmitting an immutable field with its existing value. Extend TestValidate_ImmutableFieldChangedOnUpdate with an unchanged-value update and assert validation succeeds with no errors, while preserving the existing changed-value assertion.hack/api-codegen/pkg/validation/validator_test.go-11-111 (1)
11-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllow unchanged immutable fields on update
validateWriteModeerrors on any immutable field present inExistingFields, even when the value is unchanged. Compare old/new values before returning the immutable error, and add a test for the unchanged-value case.🤖 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 `@hack/api-codegen/pkg/validation/validator_test.go` around lines 11 - 111, Update validateWriteMode to compare the immutable field’s existing and incoming values before rejecting an update; allow the update when they are equal and retain the error for changed values. Extend TestValidator_Validate_WriteMode with a case covering an unchanged immutable value in ExistingFields.platform-api/internal/codegen/featuregate/registry.go-3-37 (1)
3-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the feature-gate registry single-sourced
platform-api/internal/codegen/featuregate/registry.goandhack/api-codegen/pkg/featuregate/registry.goare identical copies with no generated header or build step tying them together. Generate one from the other, or add a sync check, so feature-gate handling in CRD filtering and request validation can’t drift.🤖 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 `@platform-api/internal/codegen/featuregate/registry.go` around lines 3 - 37, Make the feature-gate registry single-sourced by generating one registry from the other or adding an automated synchronization check covering HyperFleetFeatureGates in both registry.go copies. Ensure the check or generation step detects any drift in gate names, stages, descriptions, and entries used by CRD filtering and request validation.hack/api-codegen/cmd/openapi-merge/main.go-103-108 (1)
103-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnchecked
enc.Close()error.
yaml.Encoder.Closeflushes and finalizes the document; discarding its error can let a truncated spec be written to disk.🛠️ Proposed fix
if err := enc.Encode(&doc); err != nil { log.Fatalf("marshaling updated spec: %v", err) } - enc.Close() + if err := enc.Close(); err != nil { + log.Fatalf("finalizing updated spec: %v", err) + }As per path instructions: "Never ignore error returns".
🤖 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 `@hack/api-codegen/cmd/openapi-merge/main.go` around lines 103 - 108, Handle the error returned by enc.Close() immediately after enc.Encode in the spec generation flow, using the same fatal logging behavior as the encoding error so os.WriteFile is not reached when finalization fails. Update the code around enc and preserve the existing marshaling error context.Source: Path instructions
hack/api-codegen/pkg/conversion/mirror_types.go-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win"DO NOT EDIT" header conflicts with the "add more mirror types here" instruction.
This file is a hand-maintained registry (and is parsed as ordinary source, not skipped like
zz_generated*). Keeping the generated-file banner invites someone to regenerate over it or refuse to edit it. Drop the banner.📝 Proposed fix
-// Code generated by conversion-gen. DO NOT EDIT. - package conversionAlso applies to: 35-42
🤖 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 `@hack/api-codegen/pkg/conversion/mirror_types.go` at line 1, Remove the “Code generated by conversion-gen. DO NOT EDIT.” header from mirror_types.go, including the related occurrences near the mirror-type registry, so the hand-maintained registry is clearly editable.hack/api-codegen/pkg/conversion/generator.go-633-652 (1)
633-652: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
pathToGoNamesubstring replacement mangles words and misses bareid.
ReplaceAll(lastPart, "Id", "ID")turnsproviderIdentityintoProviderIDentity, and a leaf namedidstaysIdsince the lowercase form never matches. Apply the initialism only at a word boundary / suffix.🤖 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 `@hack/api-codegen/pkg/conversion/generator.go` around lines 633 - 652, Update pathToGoName so Id/Arn initialism conversion only applies to the relevant word boundary or suffix, rather than replacing substrings within larger words. Ensure bare lowercase leaf names such as id and arn convert to ID and ARN, while names like providerIdentity remain unaffected except for normal capitalization.
🧹 Nitpick comments (22)
hack/api-codegen/cmd/featuregate-info/main.go (1)
42-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort the gate list for deterministic output;
os.Exit(0)is redundant.
GatesForFeatureSetiterates a map, so the printed order changes between runs.♻️ Proposed tweak
gates := featuregate.GatesForFeatureSet(fs) + sort.Strings(gates) fmt.Printf("%s:\n", fs) fmt.Printf(" Total visible fields: %d\n", len(fields)) fmt.Printf(" Enabled gates: %v\n", gates) fmt.Println() } - - os.Exit(0) }Dropping
os.Exit(0)also lets theosimport go away.🤖 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 `@hack/api-codegen/cmd/featuregate-info/main.go` around lines 42 - 50, Sort the gates returned by GatesForFeatureSet before printing them so the output is deterministic, and remove the redundant os.Exit(0) at the end of the command. Delete the now-unused os import.hack/api-codegen/cmd/crd-variants/main.go (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOutput-path convention is duplicated from the featuregate package.
fmt.Sprintf("%s/%s_%s.yaml", ...)restates the naming scheme already encoded inGenerateAllVariants(hack/api-codegen/pkg/featuregate/crd_variant.golines 191-192); the two will drift if the suffix scheme changes. Exporting aVariantPath(outputDir, baseName, suffix string)helper in the package and usingfilepath.Joinwould keep both call sites in sync.🤖 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 `@hack/api-codegen/cmd/crd-variants/main.go` at line 55, Replace the duplicated output-path formatting in the crd-variants command with an exported featuregate helper, such as VariantPath, that accepts outputDir, baseName, and suffix, uses filepath.Join, and encapsulates the existing naming convention; update both this call site and GenerateAllVariants to use the helper.Makefile (1)
288-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
verify-openapimutates the working tree, and its message contradicts what already happened.Because it depends on
codegen-openapi,openapi.yamlis regenerated in place before the check, so the failure message ("run 'make codegen-openapi'") tells the user to redo a step that just ran. Consider generating to a temp copy and diffing, or reword to "openapi.yaml was out of date; commit the regenerated file".♻️ Reword suggestion
verify-openapi: codegen-openapi `@git` diff --exit-code platform-api/openapi/openapi.yaml || \ - (echo "openapi.yaml is out of date; run 'make codegen-openapi'" && exit 1) + (echo "openapi.yaml was out of date and has been regenerated; commit the changes" && exit 1)🤖 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 `@Makefile` around lines 288 - 290, Update the verify-openapi target so it does not leave regenerated changes in the working tree, or revise its failure message to instruct users to commit the regenerated openapi.yaml rather than rerun codegen. Preserve the existing diff-based verification behavior and target dependency.hack/api-codegen/pkg/passthrough/generator.go (2)
87-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnconditional debug artifact (
.rawfile) written on every generation run.Consider gating this behind a
--debug/verbose flag (or dropping it) so normal runs don't leave an extra unformatted file in the output directory.🤖 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 `@hack/api-codegen/pkg/passthrough/generator.go` around lines 87 - 91, Remove the unconditional raw-output write in the generator flow around outputFile, or gate it behind the existing debug/verbose configuration if one is available. Normal generation runs should only produce the intended generated file and must not create the “.raw” artifact.
108-148: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded import-prefix mapping is brittle for new external types.
Only
configv1.,corev1., andmetav1.prefixes are recognized; any other external package referenced by a passthrough field (e.g. a new upstream type) silently produces generated code with a missing import, which only fails atgo buildtime. Since the source files are already parsed viago/ast(seeLoadSourceFiles/ParsedFiles), the actual import alias→path mapping could be read directly from each file'sImportSpecs instead of hardcoding known prefixes.🤖 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 `@hack/api-codegen/pkg/passthrough/generator.go` around lines 108 - 148, Update Generator.collectImports to derive external type imports from the parsed source files’ ImportSpec alias-to-path mappings exposed by LoadSourceFiles/ParsedFiles, instead of recognizing only configv1., corev1., and metav1. prefixes. Match each field’s referenced package qualifier to the parsed alias, add the corresponding import, and preserve existing source-package import handling.hack/api-codegen/pkg/openapi/generator.go (1)
378-398: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnknown/external types silently degrade to generic
objectschema.Types like
metav1.Time,resource.Quantity, orintstr.IntOrStringreferenced from scanned specs aren't ing.knownTypes(only types scanned from the sameInputDirsare), so they fall back to a bareobjectschema, losing format/type information in the generated OpenAPI doc.♻️ Suggested well-known-type mapping
func (g *Generator) resolveTypeSchema(goType string) *spec.Schema { switch goType { case "string": return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"string"}}} case "bool": return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"boolean"}}} case "int", "int32", "int64": return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"integer"}}} case "float32", "float64": return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"number"}}} + case "metav1.Time", "Time": + return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"string"}, Format: "date-time"}} + case "resource.Quantity", "Quantity": + return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"string"}}} + case "intstr.IntOrString", "IntOrString": + return &spec.Schema{SchemaProps: spec.SchemaProps{Type: []string{"string"}}} default:🤖 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 `@hack/api-codegen/pkg/openapi/generator.go` around lines 378 - 398, The resolveTypeSchema method should recognize common external Kubernetes types before falling back to a generic object schema. Add well-known mappings for types such as metav1.Time, resource.Quantity, and intstr.IntOrString, preserving their appropriate OpenAPI primitive, format, or union schemas; keep knownTypes reference resolution unchanged and use the generic object fallback only for genuinely unknown types.hack/api-codegen/pkg/markers/generator.go (2)
13-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmit string literals via
printf "%q"instead of raw interpolation.
"{{ .FieldPath }}"/"{{ .FeatureGate }}"are unescaped, so any quote or backslash in a JSON tag produces invalid Go. Cheap hardening for a generator:{{ printf "%q" .FieldPath }}.🤖 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 `@hack/api-codegen/pkg/markers/generator.go` around lines 13 - 82, Update registryTemplate to emit all interpolated string values using printf "%q" rather than raw quoted interpolation. Apply this to FieldPath, FeatureGate, and each GatedWriteModes FeatureGate value so quotes and backslashes produce valid Go string literals while preserving the generated registry structure.
129-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the
WriteMode→ identifier mapping and fail loudly on unknown modes.The switch is duplicated, and an unrecognized
WriteModesilently yields an empty string that emitsWriteMode: ,— surfacing later as an opaquego/formaterror instead of a clear codegen failure.♻️ Suggested helper
+func writeModeConst(m WriteMode) (string, error) { + switch m { + case "": + return "", nil + case Mutable: + return "Mutable", nil + case Immutable: + return "Immutable", nil + case ServiceSet: + return "ServiceSet", nil + default: + return "", fmt.Errorf("unknown write mode %q", m) + } +}- // Convert WriteMode to const reference - switch meta.WriteMode { - case Mutable: - field.WriteMode = "Mutable" - case Immutable: - field.WriteMode = "Immutable" - case ServiceSet: - field.WriteMode = "ServiceSet" - } + field.WriteMode, err = writeModeConst(meta.WriteMode) + if err != nil { + return fmt.Errorf("field %s: %w", path, err) + } // Convert FeatureGateAwareWriteModes for _, gated := range meta.FeatureGateAwareWriteModes { - var writeModeStr string - switch gated.WriteMode { - case Mutable: - writeModeStr = "Mutable" - case Immutable: - writeModeStr = "Immutable" - case ServiceSet: - writeModeStr = "ServiceSet" - } + writeModeStr, err := writeModeConst(gated.WriteMode) + if err != nil { + return fmt.Errorf("field %s: %w", path, err) + }Declare
var err errorbefore the loop (or restructure) so the assignment compiles.🤖 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 `@hack/api-codegen/pkg/markers/generator.go` around lines 129 - 154, Extract the duplicated WriteMode-to-identifier switch from the generator flow around meta.WriteMode and meta.FeatureGateAwareWriteModes into a shared helper. Have the helper return an error for any unrecognized WriteMode instead of an empty string, and propagate that error from the surrounding generation function so code generation fails with a clear message; declare or restructure the error variable as needed for compilation.hack/api-codegen/pkg/featuregate/crd_variant.go (1)
26-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse
GenerateVariantintoWriteVariantToWriter.Read/parse/filter/encode is duplicated verbatim; the two will drift (note
GenerateVariantsets indent and closes the encoder,WriteVariantToWriternever callsencoder.Close()).♻️ Suggested restructure
func (g *CRDVariantGenerator) GenerateVariant(inputPath string, outputPath string, featureSet FeatureSet) error { - // Read input CRD - data, err := os.ReadFile(inputPath) - if err != nil { - return fmt.Errorf("reading CRD: %w", err) - } - - // Parse YAML - var crd yaml.Node - if err := yaml.Unmarshal(data, &crd); err != nil { - return fmt.Errorf("parsing YAML: %w", err) - } - - // Filter the CRD based on feature set - ctx := &filterContext{ - featureSet: featureSet, - inSchema: false, - fieldPath: "", - } - if err := g.filterCRDNode(&crd, ctx); err != nil { - return fmt.Errorf("filtering CRD: %w", err) - } - - // Write output f, err := os.Create(outputPath) if err != nil { return fmt.Errorf("creating output file: %w", err) } - - encoder := yaml.NewEncoder(f) - encoder.SetIndent(2) - if err := encoder.Encode(&crd); err != nil { - f.Close() - return fmt.Errorf("writing YAML: %w", err) - } - if err := encoder.Close(); err != nil { - f.Close() - return fmt.Errorf("closing YAML encoder: %w", err) - } + if err := g.WriteVariantToWriter(inputPath, f, featureSet); err != nil { + _ = f.Close() + return err + } if err := f.Close(); err != nil { return fmt.Errorf("closing output file: %w", err) } - return nil }And add
defer-free explicitencoder.Close()insideWriteVariantToWriterso buffered output is always flushed.Also applies to: 204-235
🤖 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 `@hack/api-codegen/pkg/featuregate/crd_variant.go` around lines 26 - 70, Refactor GenerateVariant to handle only input/output file setup and delegate CRD reading, parsing, filtering, and encoding to WriteVariantToWriter. Move the shared encoder configuration and explicit encoder.Close() flushing into WriteVariantToWriter, preserving the existing error wrapping and ensuring the output writer is closed by GenerateVariant.hack/api-codegen/pkg/markers/scanner.go (1)
277-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ValidateAllFieldsis a stub that silently under-validates.It's documented as the strict CI check but just delegates to
Registry.Validate(), which only sees fields that already carry a marker — exactly the fields a strict check is meant to catch are missed. Want me to open an issue to track implementing the full-struct walk?🤖 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 `@hack/api-codegen/pkg/markers/scanner.go` around lines 277 - 283, The ValidateAllFields method is a stub that does not perform the documented strict validation. Implement a full walk over every struct field discovered by MarkerScanner, validating unmarked fields against the same requirements used by Registry.Validate, and return any violations; do not delegate solely to s.Registry.Validate().hack/api-codegen/pkg/featuregate/crd_filter.go (1)
11-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a deterministic order and drop the duplicated gating logic.
FilterCRDFieldsiterates a map, so the returned slice order varies per run — undesirable for a codegen tool where output stability matters. The gating logic is also an exact copy ofFieldsForFeatureSet.♻️ Reuse `FieldsForFeatureSet` and sort
func FilterCRDFields(featureSet FeatureSet) []string { - var includedFields []string - - for fieldPath, meta := range registry.FieldRegistry { - // Skip hidden fields - they never appear in CRDs - if meta.Hidden { - continue - } - - // If field has no gate, it's always included (GA) - if meta.FeatureGate == "" { - includedFields = append(includedFields, fieldPath) - continue - } - - // Check if this feature gate is enabled for the feature set - if IsGateEnabled(meta.FeatureGate, featureSet) { - includedFields = append(includedFields, fieldPath) - } - } - - return includedFields + fields := FieldsForFeatureSet(featureSet) + includedFields := make([]string, 0, len(fields)) + for fieldPath := range fields { + includedFields = append(includedFields, fieldPath) + } + sort.Strings(includedFields) + return includedFields }Add
"sort"to the import block.🤖 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 `@hack/api-codegen/pkg/featuregate/crd_filter.go` around lines 11 - 33, Update FilterCRDFields to reuse FieldsForFeatureSet for feature-gate filtering instead of duplicating the registry traversal and IsGateEnabled logic, while preserving exclusion of hidden fields as supported by that helper. Sort the resulting field paths with sort.Strings before returning them, and add the required sort import to ensure deterministic code generation.hack/api-codegen/pkg/passthrough/generator_test.go (1)
39-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on generated content, not just file existence.
The test passes as long as an empty file is produced; lines 45-66 only log output, which is debug scaffolding rather than verification. Assert that expected symbols (e.g. the
HostedClusterSpecmirror type / passthrough funcs) appear in the generated source.♻️ Tighten the assertions
content, err := os.ReadFile(outputFile) if err != nil { t.Fatalf("Failed to read output: %v", err) } - t.Logf("Generated file size: %d bytes", len(content)) - - // Show first 50 lines - lines := 0 - for i, b := range content { - if b == '\n' { - lines++ - if lines >= 50 { - t.Logf("First 50 lines of generated code:\n%s\n... (truncated)", content[:i]) - break - } - } - } - if lines < 50 { - t.Logf("Generated code:\n%s", content) - } + for _, want := range []string{"// Code generated", "HostedClusterSpec"} { + if !strings.Contains(string(content), want) { + t.Errorf("generated output missing %q", want) + } + }Add
"strings"to the imports.🤖 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 `@hack/api-codegen/pkg/passthrough/generator_test.go` around lines 39 - 67, Strengthen the test after reading outputFile in the generator test by importing strings and asserting that the generated content contains expected symbols such as HostedClusterSpec and the passthrough functions. Replace the lines-only logging scaffold with verification of generated source, while retaining only any useful failure context.hack/api-codegen/pkg/validation/gated_writemode_test.go (1)
21-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for "immutable field resent with unchanged value" on update.
All immutable-on-update cases use different old/new values. There's no case verifying behavior when a client resends the same value for an immutable field (a very common PUT/PATCH pattern for full-spec resubmission). This gap let the missing value-comparison check in
validateWriteMode(validator.go, Immutable case) go untested.Also applies to: 96-104
🤖 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 `@hack/api-codegen/pkg/validation/gated_writemode_test.go` around lines 21 - 33, Add an update test alongside the existing immutable cases in gated_writemode_test.go where the immutable field’s old and new values are identical, and assert validation succeeds without an error. Cover the default immutable mode and retain the existing differing-value test to verify changed values remain blocked.platform-api/pkg/config/config.go (1)
26-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating
FeatureSetagainst known values at startup.An unrecognized value here (typo, e.g.
"TechPreview"instead of"TechPreviewNoUpgrade") will silently behave asDefault(perFeatureSet.MaxStage()'s default case), with no error or log. A startup-time check/log for unrecognized values would help catch config typos early.🤖 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 `@platform-api/pkg/config/config.go` at line 26, Validate the FeatureSet configuration during startup against the supported values before runtime feature evaluation. Reuse the existing FeatureSet constants or validation mechanism, and return a clear configuration error or log an explicit warning for unrecognized values instead of allowing FeatureSet.MaxStage() to silently use its default case.platform-api/internal/codegen/conversion/nodepool.go (1)
10-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHardcoded field list risks silent regressions when new service-set fields are added.
This manually enumerates service-set fields rather than deriving them from the field registry/markers already generated elsewhere in this PR. A future
+hyperfleet:write-mode=service-setfield added toNodePoolSpecwithout updating this function would silently lose its value on update — the same class of bug this function was written to prevent.Consider adding a registry-driven test (using the generated
field_metadata) that asserts everyservice-setNodePoolSpecfield is covered by this function, so drift is caught at test time even if the function itself stays hand-written.🤖 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 `@platform-api/internal/codegen/conversion/nodepool.go` around lines 10 - 13, Add a registry-driven test for PreserveNodePoolServiceSet that reads generated field_metadata, identifies every NodePoolSpec field marked with write-mode=service-set, and verifies the function preserves each field from snapshot to updated. Keep the existing function implementation unchanged unless needed to make the coverage assertion reliable.platform-api/pkg/server/server.go (1)
41-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate/log unrecognized
cfg.Regional.FeatureSetvalues.Only an empty string falls back to
featuregate.Default; a typo'd or unsupported value (e.g."TechPreview") is silently accepted here and only resolves to a safe default deep insideFeatureSet.MaxStage(), with no warning logged. A misconfiguration would silently disable all gated fields for the whole region without any operator-visible signal.♻️ Suggested guard
fs := featuregate.FeatureSet(cfg.Regional.FeatureSet) if fs == "" { fs = featuregate.Default + } else if fs != featuregate.Default && fs != featuregate.TechPreviewNoUpgrade && fs != featuregate.DevPreviewNoUpgrade { + logger.Warn("unrecognized feature set in config, falling back to GA behavior", "configured_feature_set", cfg.Regional.FeatureSet) }🤖 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 `@platform-api/pkg/server/server.go` around lines 41 - 46, Validate cfg.Regional.FeatureSet when constructing fs in the server initialization flow, including rejecting or warning on non-empty values that are not recognized featuregate.FeatureSet values such as “TechPreview”. Preserve the existing empty-value fallback to featuregate.Default, and emit an operator-visible log message before applying the safe fallback for unsupported values.hack/api-codegen/cmd/openapi-merge/main.go (1)
211-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cleanDescriptionmisses composition keywords, and the key sort is a no-op.Recursion covers
properties,items, andadditionalPropertiesbut notallOf/oneOf/anyOf, so+kubebuilder/+hyperfleetmarker lines survive in those descriptions. Thesort.Strings(keys)at Lines 231-235 has no effect on output since only map values are mutated.♻️ Proposed refactor
- if props, ok := m["properties"].(map[string]interface{}); ok { - keys := make([]string, 0, len(props)) - for k := range props { - keys = append(keys, k) - } - sort.Strings(keys) - for _, k := range keys { - if pm, ok := props[k].(map[string]interface{}); ok { - cleanDescription(pm) - } - } - } - - if items, ok := m["items"].(map[string]interface{}); ok { - cleanDescription(items) - } - - if addl, ok := m["additionalProperties"].(map[string]interface{}); ok { - cleanDescription(addl) - } + if props, ok := m["properties"].(map[string]interface{}); ok { + for _, v := range props { + if pm, ok := v.(map[string]interface{}); ok { + cleanDescription(pm) + } + } + } + + for _, key := range []string{"items", "additionalProperties"} { + if sub, ok := m[key].(map[string]interface{}); ok { + cleanDescription(sub) + } + } + + for _, key := range []string{"allOf", "oneOf", "anyOf"} { + if list, ok := m[key].([]interface{}); ok { + for _, item := range list { + if im, ok := item.(map[string]interface{}); ok { + cleanDescription(im) + } + } + } + }🤖 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 `@hack/api-codegen/cmd/openapi-merge/main.go` around lines 211 - 250, Update cleanDescription to recursively process schemas contained in allOf, oneOf, and anyOf, removing marker lines from their descriptions just as it does for properties, items, and additionalProperties. Remove the ineffective key collection and sort in the properties traversal, since map iteration order does not affect the mutated output.hack/api-codegen/cmd/verify-configuration/main.go (1)
61-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
hasVisibilitytracking and de-duplicate the marker scan.
hasVisibilityis computed twice and then explicitly discarded at Line 99 — dead code kept for a hypothetical future check. The Doc and Comment scan blocks are identical; one helper over both comment groups covers it.♻️ Proposed refactor
- // Check if field has markers (in Doc or Comment) - hasWriteMode := false - hasVisibility := false - - // Check Doc comments (above the field) - if field.Doc != nil { - for _, comment := range field.Doc.List { - text := comment.Text - if strings.Contains(text, "+hyperfleet:write-mode=") { - hasWriteMode = true - } - if strings.Contains(text, "+k8s:openapi-gen=") { - hasVisibility = true - } - } - } - - // Check inline comments (after the field) - if field.Comment != nil { - for _, comment := range field.Comment.List { - text := comment.Text - if strings.Contains(text, "+hyperfleet:write-mode=") { - hasWriteMode = true - } - if strings.Contains(text, "+k8s:openapi-gen=") { - hasVisibility = true - } - } - } - - // Fields should have write-mode marker - if !hasWriteMode { + // Visibility markers are intentionally not enforced: + // no marker = visible (default), +k8s:openapi-gen=false = hidden. + if !hasMarker(writeModeMarker, field.Doc, field.Comment) { errors = append(errors, fmt.Sprintf("%s.%s: missing +hyperfleet:write-mode marker", typeName, fieldName)) } - - // Note: Visibility markers are optional - // - No marker = visible (default, standard Kubernetes convention) - // - +k8s:openapi-gen=false = hidden (explicit) - // We only enforce write-mode markers, not visibility markers - _ = hasVisibility // Acknowledged - used for future enforcement if neededAdd the helper:
const writeModeMarker = "+hyperfleet:write-mode=" func hasMarker(marker string, groups ...*ast.CommentGroup) bool { for _, g := range groups { if g == nil { continue } for _, c := range g.List { if strings.Contains(c.Text, marker) { return true } } } return false }🤖 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 `@hack/api-codegen/cmd/verify-configuration/main.go` around lines 61 - 99, Remove the unused hasVisibility tracking and _ = hasVisibility statement. Add a hasMarker helper that accepts a marker and variadic *ast.CommentGroup values, skips nil groups, and scans each comment group once; define the writeModeMarker constant and use hasMarker with field.Doc and field.Comment to set hasWriteMode while preserving the existing validation.hack/api-codegen/pkg/conversion/mirror_types_test.go (1)
118-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
FieldNameuniqueness in the completeness test.
GetMirrorMappingreturns the first match, so a duplicateFieldNamewould silently shadow a later mapping and the current loop would still pass. A set check here catches that as the registry grows.💚 Proposed addition
func TestMirrorTypeMappings_Completeness(t *testing.T) { + seen := make(map[string]bool) // Verify that all mirror type mappings have required fields for _, mapping := range mirrorTypeMappings { if mapping.FieldName == "" { t.Error("Found mapping with empty FieldName") } + if seen[mapping.FieldName] { + t.Errorf("Duplicate FieldName in registry: %s", mapping.FieldName) + } + seen[mapping.FieldName] = true🤖 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 `@hack/api-codegen/pkg/conversion/mirror_types_test.go` around lines 118 - 143, Extend TestMirrorTypeMappings_Completeness to track encountered mapping.FieldName values in a set and fail when a name appears more than once, before or alongside the existing field validation and GetMirrorMapping lookup checks. Keep the current completeness assertions unchanged.hack/api-codegen/pkg/conversion/generator_test.go (1)
32-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend table coverage to the pure helpers that shape generated output.
qualifyType,pathToGoName, andpathToJSONTagare string transforms with real edge cases (pointer prefixes, already-qualified types,Id/Arninitialisms) and no tests. These are the cheapest guards against the generator emitting bad Go.🤖 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 `@hack/api-codegen/pkg/conversion/generator_test.go` around lines 32 - 81, Extend TestBuildFieldPath with table-driven coverage for the pure helpers qualifyType, pathToGoName, and pathToJSONTag. Add cases for pointer-prefixed and already-qualified types, plus Id/Arn initialism transformations, and assert each helper’s expected generated Go name or JSON tag.hack/api-codegen/pkg/conversion/mirror_types.go (1)
25-33: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMatching mirror types by bare field name is collision-prone, and the declared types are never used.
IsMirrorType("Configuration")is true for any struct field namedConfigurationin any parsed type, regardless of its actual Go type;generator.go(Lines 786-802) then derives the helper name from the field's own type and ignoresHyperFleetType/HyperShiftType/ConversionStrategyentirely. Consider keying the lookup on (type name, field name) or on the field's Go type, and validating the field type againstHyperShiftTypebefore emitting a conversion call.Also applies to: 47-59
🤖 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 `@hack/api-codegen/pkg/conversion/mirror_types.go` around lines 25 - 33, Update mirror-type lookup and generation around mirrorTypeMappings, IsMirrorType, and the generator conversion path to match both the containing type and field name (or validate the field’s Go type against HyperShiftType) instead of using the bare field name. Use the matched HyperFleetType, HyperShiftType, and ConversionStrategy when emitting conversions, and reject or skip fields whose declared type does not match the mapping.hack/api-codegen/pkg/conversion/generator.go (1)
709-725: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGenerated code hardcodes the
conversion.package qualifier.The parent package is imported unaliased from
parentPkg, butUnproject%salways referencesconversion.ServiceSetFields, whilegenerateServiceSetFieldsderives the actual package name fromfilepath.Base(filepath.Dir(OutputDir))(Line 585). Any output layout whose parent directory isn't namedconversionyields uncompilable output. Reuse the derived package name, or emit an explicit alias in the import block.♻️ Alias the parent import
- fmt.Fprintf(&b, "\t\"%s\"\n", parentPkg) + fmt.Fprintf(&b, "\tconversion \"%s\"\n", parentPkg)Also applies to: 861-862
🤖 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 `@hack/api-codegen/pkg/conversion/generator.go` around lines 709 - 725, Update the generated parent-package import and all Unproject%s references to use the package name derived by generateServiceSetFields, rather than hardcoded conversion. Apply the same correction to the corresponding references around the additional affected lines, ensuring generated code compiles when the parent output directory has a different name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61ae6762-bfaa-4e01-b489-e77087db16fa
⛔ Files ignored due to path filters (2)
hack/api-codegen/go.sumis excluded by!**/*.sumhyperfleet-operator/api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (67)
.gitignoreMakefilehack/api-codegen/README.mdhack/api-codegen/cmd/conversion-gen/main.gohack/api-codegen/cmd/crd-variants/main.gohack/api-codegen/cmd/featuregate-info/main.gohack/api-codegen/cmd/marker-scanner/main.gohack/api-codegen/cmd/openapi-gen/main.gohack/api-codegen/cmd/openapi-merge/main.gohack/api-codegen/cmd/passthrough-gen/main.gohack/api-codegen/cmd/verify-configuration/main.gohack/api-codegen/go.modhack/api-codegen/pkg/conversion/generator.gohack/api-codegen/pkg/conversion/generator_test.gohack/api-codegen/pkg/conversion/mirror_types.gohack/api-codegen/pkg/conversion/mirror_types_test.gohack/api-codegen/pkg/featuregate/crd_filter.gohack/api-codegen/pkg/featuregate/crd_variant.gohack/api-codegen/pkg/featuregate/crd_variant_test.gohack/api-codegen/pkg/featuregate/featuregate_test.gohack/api-codegen/pkg/featuregate/registry.gohack/api-codegen/pkg/featuregate/types.gohack/api-codegen/pkg/markers/gated_writemode_test.gohack/api-codegen/pkg/markers/generator.gohack/api-codegen/pkg/markers/json.gohack/api-codegen/pkg/markers/json_test.gohack/api-codegen/pkg/markers/scanner.gohack/api-codegen/pkg/markers/scanner_test.gohack/api-codegen/pkg/markers/types.gohack/api-codegen/pkg/openapi/generator.gohack/api-codegen/pkg/openapi/generator_test.gohack/api-codegen/pkg/openapi/types.gohack/api-codegen/pkg/passthrough/generator.gohack/api-codegen/pkg/passthrough/generator_test.gohack/api-codegen/pkg/passthrough/gomod.gohack/api-codegen/pkg/passthrough/integration_test.gohack/api-codegen/pkg/passthrough/loader.gohack/api-codegen/pkg/passthrough/loader_test.gohack/api-codegen/pkg/passthrough/types.gohack/api-codegen/pkg/registry/field_metadata.gohack/api-codegen/pkg/registry/field_metadata.jsonhack/api-codegen/pkg/validation/example_test.gohack/api-codegen/pkg/validation/gated_writemode_test.gohack/api-codegen/pkg/validation/validator.gohack/api-codegen/pkg/validation/validator_test.gohack/tools/go.modhyperfleet-operator/Containerfilehyperfleet-operator/api/go.modhyperfleet-operator/api/v1alpha1/cluster_types.gohyperfleet-operator/api/v1alpha1/configuration.gohyperfleet-operator/api/v1alpha1/hostedclusterspec.passthrough.gohyperfleet-operator/api/v1alpha1/nodepool_types.goplatform-api/internal/codegen/conversion/cluster.goplatform-api/internal/codegen/conversion/cluster_test.goplatform-api/internal/codegen/conversion/nodepool.goplatform-api/internal/codegen/conversion/nodepool_test.goplatform-api/internal/codegen/featuregate/registry.goplatform-api/internal/codegen/featuregate/types.goplatform-api/internal/codegen/registry/field_metadata.goplatform-api/internal/codegen/registry/field_metadata.jsonplatform-api/openapi/openapi.yamlplatform-api/pkg/config/config.goplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/server/server.goplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
|
/test on-demand-e2e |
…passthrough schema Add swagger-ui-serve/swagger-ui-open Makefile targets for local API docs browsing. Fix ClusterSpec.hostedCluster to $ref HostedClusterSpecPassthrough instead of rendering as an opaque object. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
platform-api/openapi/openapi.yaml (1)
2223-2250: 🗄️ Data Integrity & Integration | 🟠 MajorKeep
hostedClusterandnodePooloptional in reusable specs.These schemas are referenced by both create and update requests. Requiring these fields breaks existing clients that omit them and repeats the previously reported OpenAPI compatibility issue. Remove them from
requiredunless this is an intentional versioned breaking change.🤖 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 `@platform-api/openapi/openapi.yaml` around lines 2223 - 2250, Remove hostedCluster and nodePool from the required lists of the reusable OpenAPI schemas, including the NodePoolSpec definition and the corresponding hosted-cluster schema. Keep both properties declared but optional so create and update requests remain compatible with clients that omit them.
🤖 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 `@platform-api/openapi/swagger-ui/index.html`:
- Around line 6-16: Update the Swagger UI asset imports in the HTML document to
use integrity hashes with crossorigin="anonymous" for both the stylesheet and
scripts, or replace the unpkg references with locally vendored assets. Apply the
protection consistently to the Swagger UI CSS, bundle, and standalone preset
resources.
---
Duplicate comments:
In `@platform-api/openapi/openapi.yaml`:
- Around line 2223-2250: Remove hostedCluster and nodePool from the required
lists of the reusable OpenAPI schemas, including the NodePoolSpec definition and
the corresponding hosted-cluster schema. Keep both properties declared but
optional so create and update requests remain compatible with clients that omit
them.
🪄 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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 733edf63-db74-46b3-b943-baa1439d5489
📒 Files selected for processing (4)
Makefilehack/api-codegen/cmd/openapi-merge/main.goplatform-api/openapi/openapi.yamlplatform-api/openapi/swagger-ui/index.html
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/api-codegen/cmd/openapi-merge/main.go
- Makefile
…codegen README Fields are visible by default — only +k8s:openapi-gen=false has an effect. Remove the redundant =true markers and fix blank lines they left in comment blocks. Update hack/api-codegen/README.md with full pipeline documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hack/api-codegen/README.md`:
- Line 24: Update the fenced pipeline diagram code block in the README to
specify the text language by changing its opening fence to use text, preserving
the diagram content unchanged.
🪄 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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b5471fb1-813b-4d82-b1e8-e42fdd1d27aa
📒 Files selected for processing (3)
hack/api-codegen/README.mdhyperfleet-operator/api/v1alpha1/cluster_types.gohyperfleet-operator/api/v1alpha1/hostedclusterspec.passthrough.go
💤 Files with no reviewable changes (2)
- hyperfleet-operator/api/v1alpha1/hostedclusterspec.passthrough.go
- hyperfleet-operator/api/v1alpha1/cluster_types.go
|
/test |
|
/test on-demand-e2e |
1 similar comment
|
/test on-demand-e2e |
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
cdoan1
added
the
do-not-merge/hold
|
@cdoan1: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/internal/codegen/registry/field_metadata.go (1)
48-330: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd a generated-registry drift check.
make codegen-registryupdates only theplatform-apiartifacts, andmake verifydoes not compare generated files. The Go registries currently match, buthack/api-codegen/pkg/registry/field_metadata.jsonhas 122 entries while theplatform-apiJSON has 58, with additional metadata differences. Add a check for all generated outputs, or remove the duplicate artifacts and keep one source.🤖 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 `@platform-api/internal/codegen/registry/field_metadata.go` around lines 48 - 330, The generated registry artifacts drift because code generation and verification do not compare all outputs. Add a generated-output consistency check covering platform-api/internal/codegen/registry/field_metadata.go (lines 48-330), hack/api-codegen/pkg/registry/field_metadata.go (lines 47-330), and platform-api/internal/codegen/registry/field_metadata.json (lines 3-271), or remove the duplicate artifacts and retain one authoritative source; ensure make verify detects any registry differences.
🧹 Nitpick comments (3)
hack/api-codegen/pkg/markers/scanner.go (1)
100-122: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDetect configured root types that no input directory contains.
rootTypesis now the single source of truth for scanner entry points. The scan only visits root types found indirCache. If a root type is renamed, moved, or misspelled,Scanstill returns success and the registry silently loses every field below that root. Hidden and service-set fields then fall through the "not in registry" branch in consumers such ashack/api-codegen/pkg/featuregate/crd_variant.go.Track the root types that were matched across all input directories and fail when one is never found.
♻️ Proposed fix: report unmatched root types
Change
Scanto collect matches and verify coverage:func (s *MarkerScanner) Scan() error { seen := make(map[string]bool, len(rootTypes)) for _, dir := range s.InputDirs { if err := s.scanDir(dir, seen); err != nil { return fmt.Errorf("scanning directory %s: %w", dir, err) } } var missing []string for typeName := range rootTypes { if !seen[typeName] { missing = append(missing, typeName) } } if len(missing) > 0 { sort.Strings(missing) return fmt.Errorf("root types not found in any input directory: %s", strings.Join(missing, ", ")) } return nil }Then record each processed root in
scanDir:for _, typeName := range roots { + seen[typeName] = true visited := make(map[string]bool) visited[typeName] = true s.processStruct(typeName, dirCache[typeName], rootTypes[typeName], visited) }🤖 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 `@hack/api-codegen/pkg/markers/scanner.go` around lines 100 - 122, Update MarkerScanner.Scan to track root types matched across all input directories, passing the shared seen set into scanDir and returning an error listing sorted entries from rootTypes that were never found. In scanDir, mark each processed root type in that set while preserving existing directory error wrapping and successful scanning behavior.platform-api/pkg/validation/field_validator.go (2)
126-146: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDefine precedence when several gate-aware entries are enabled.
The first loop accepts the first entry whose gate is enabled and stops. If a field lists two gated entries and the feature set enables both, the effective write mode depends on slice order in the generated metadata. Add a defined precedence, or reject metadata that contains more than one entry for gates that can be enabled together.
🤖 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 `@platform-api/pkg/validation/field_validator.go` around lines 126 - 146, In the validateWriteMode function, the first loop that iterates through FeatureGateAwareWriteModes accepts the first entry with an enabled gate and stops, creating undefined behavior when multiple gates are enabled simultaneously since the result depends on slice order. Either establish explicit precedence by sorting or prioritizing entries before iteration, or add validation logic to detect and reject metadata configurations where multiple enabled feature gates would apply to the same field. Preserve the existing fallback behavior that matches an override with an empty FeatureGate when no gated entries apply.
148-172: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFail closed on an unrecognized write mode.
The
defaultbranch returnsnil, so a field whose generatedWriteModeis empty or unrecognized is writable by customers. The generator guards this today throughFieldRegistry.Validate(), but the runtime enforcement point should not depend on that guarantee. A malformed or truncated registry entry would silently open a platform-managed field.Reject unknown modes instead.
🔒️ Proposed fix
case registry.Mutable: return nil default: - return nil + return &ValidationError{ + Field: fieldPath, + Reason: fmt.Sprintf("field has unrecognized write mode %q", effectiveMode), + } }🤖 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 `@platform-api/pkg/validation/field_validator.go` around lines 148 - 172, The default case in the effectiveMode switch statement currently returns nil, which silently allows unrecognized or empty write modes to pass validation. Update the default case to return a ValidationError instead, rejecting unknown modes with an appropriate message that identifies the unrecognized write mode value. This ensures that malformed or truncated registry entries do not inadvertently open platform-managed fields.
🤖 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 `@hack/api-codegen/pkg/validation/validator_test.go`:
- Around line 113-133: Align hack-side Validator.validateWriteMode and
platform-api FieldValidator.validateWriteMode on a shared, compatible
feature-gate input and resolution mechanism. Preserve FeatureGateAwareWriteModes
precedence: select a matching specific gate before falling back to the default
empty gate, while retaining each validator’s existing write-mode behavior.
Prefer reusing shared resolution logic rather than maintaining separate
EnabledGates and FeatureSet implementations.
---
Outside diff comments:
In `@platform-api/internal/codegen/registry/field_metadata.go`:
- Around line 48-330: The generated registry artifacts drift because code
generation and verification do not compare all outputs. Add a generated-output
consistency check covering
platform-api/internal/codegen/registry/field_metadata.go (lines 48-330),
hack/api-codegen/pkg/registry/field_metadata.go (lines 47-330), and
platform-api/internal/codegen/registry/field_metadata.json (lines 3-271), or
remove the duplicate artifacts and retain one authoritative source; ensure make
verify detects any registry differences.
---
Nitpick comments:
In `@hack/api-codegen/pkg/markers/scanner.go`:
- Around line 100-122: Update MarkerScanner.Scan to track root types matched
across all input directories, passing the shared seen set into scanDir and
returning an error listing sorted entries from rootTypes that were never found.
In scanDir, mark each processed root type in that set while preserving existing
directory error wrapping and successful scanning behavior.
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 126-146: In the validateWriteMode function, the first loop that
iterates through FeatureGateAwareWriteModes accepts the first entry with an
enabled gate and stops, creating undefined behavior when multiple gates are
enabled simultaneously since the result depends on slice order. Either establish
explicit precedence by sorting or prioritizing entries before iteration, or add
validation logic to detect and reject metadata configurations where multiple
enabled feature gates would apply to the same field. Preserve the existing
fallback behavior that matches an override with an empty FeatureGate when no
gated entries apply.
- Around line 148-172: The default case in the effectiveMode switch statement
currently returns nil, which silently allows unrecognized or empty write modes
to pass validation. Update the default case to return a ValidationError instead,
rejecting unknown modes with an appropriate message that identifies the
unrecognized write mode value. This ensures that malformed or truncated registry
entries do not inadvertently open platform-managed fields.
🪄 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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 566ccf37-eecb-40fa-b5e0-aec88caf6aef
📒 Files selected for processing (16)
hack/api-codegen/README.mdhack/api-codegen/cmd/openapi-merge/main.gohack/api-codegen/pkg/conversion/generator.gohack/api-codegen/pkg/conversion/generator_test.gohack/api-codegen/pkg/featuregate/crd_variant.gohack/api-codegen/pkg/featuregate/crd_variant_test.gohack/api-codegen/pkg/markers/gated_writemode_test.gohack/api-codegen/pkg/markers/scanner.gohack/api-codegen/pkg/markers/scanner_test.gohack/api-codegen/pkg/registry/field_metadata.gohack/api-codegen/pkg/validation/validator.gohack/api-codegen/pkg/validation/validator_test.goplatform-api/internal/codegen/registry/field_metadata.goplatform-api/internal/codegen/registry/field_metadata.jsonplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/api-codegen/pkg/markers/gated_writemode_test.go
- hack/api-codegen/README.md
- hack/api-codegen/pkg/validation/validator.go
- hack/api-codegen/pkg/featuregate/crd_variant_test.go
- hack/api-codegen/cmd/openapi-merge/main.go
- hack/api-codegen/pkg/featuregate/crd_variant.go
- hack/api-codegen/pkg/conversion/generator.go
| func TestValidator_Validate_ImmutableUnchanged(t *testing.T) { | ||
| v := &Validator{ | ||
| registry: map[string]registry.FieldMeta{ | ||
| "spec.name": { | ||
| FieldPath: "spec.name", | ||
| WriteMode: registry.Immutable, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| req := &Request{ | ||
| Operation: OperationUpdate, | ||
| Fields: map[string]interface{}{"spec.name": "same-value"}, | ||
| ExistingFields: map[string]interface{}{"spec.name": "same-value"}, | ||
| FeatureSet: featuregate.Default, | ||
| } | ||
|
|
||
| if err := v.Validate(req); err != nil { | ||
| t.Errorf("Validate() should allow unchanged immutable field, got error: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hack-side validateWriteMode ---'
ast-grep run --pattern 'func ($V *Validator) validateWriteMode($$$) { $$$ }' --lang go hack/api-codegen/pkg/validation/validator.go
echo
echo '--- gate-aware handling in both validators ---'
rg -n -C 6 'FeatureGateAwareWriteModes' hack/api-codegen/pkg/validation platform-api/pkg/validationRepository: openshift-online/rosa-hyperfleet-api
Length of output: 210
🏁 Script executed:
cat /tmp/coderabbit-shell-logs/shell-output-BS4vjLRepository: openshift-online/rosa-hyperfleet-api
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the validator file structure
echo '=== validator.go structure ==='
ast-grep outline hack/api-codegen/pkg/validation/validator.go
echo
echo '=== All functions and methods in validator.go ==='
rg -n 'func ' hack/api-codegen/pkg/validation/validator.go
echo
echo '=== Searching for WriteMode handling in hack-side validator ==='
rg -n -A 5 'WriteMode' hack/api-codegen/pkg/validation/validator.go | head -50
echo
echo '=== All tests in validator_test.go ==='
rg -n '^func Test' hack/api-codegen/pkg/validation/validator_test.go
echo
echo '=== Searching for FeatureGateAwareWriteModes in both packages ==='
rg -r 'FeatureGateAwareWriteModes' hack/api-codegen/pkg/validation platform-api/pkg/validation 2>/dev/null || echo "Pattern not found or error occurred"Repository: openshift-online/rosa-hyperfleet-api
Length of output: 3896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Full validateWriteMode implementation ==='
sed -n '128,190p' hack/api-codegen/pkg/validation/validator.go
echo
echo '=== TestValidator_Validate_FeatureGates test ==='
sed -n '135,205p' hack/api-codegen/pkg/validation/validator_test.go
echo
echo '=== platform-api field_validator.go structure ==='
ast-grep outline platform-api/pkg/validation/field_validator.go | head -40
echo
echo '=== Search for FeatureGateAwareWriteModes in registry or FieldMeta definition ==='
rg -n 'FeatureGateAwareWriteModes' --type go -A 3 hack/ platform-api/Repository: openshift-online/rosa-hyperfleet-api
Length of output: 17813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Check if gated_writemode_test.go exists in hack-side ==='
ls -la hack/api-codegen/pkg/validation/gated_writemode_test.go 2>/dev/null || echo "File not found"
echo
echo '=== Contents of hack-side gated_writemode_test.go ==='
cat hack/api-codegen/pkg/validation/gated_writemode_test.go 2>/dev/null || echo "File not accessible"
echo
echo '=== Comparison: platform-api field_validator.go validateWriteMode lines 126-175 ==='
sed -n '126,175p' platform-api/pkg/validation/field_validator.go
echo
echo '=== Check how IsFeatureGateEnabled is implemented in hack-side ==='
sed -n '43,51p' hack/api-codegen/pkg/validation/validator.go
echo
echo '=== Check featuregate.IsGateEnabled in platform-api imports and usage ==='
head -30 platform-api/pkg/validation/field_validator.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 9648
Write-mode gate-aware logic is tested, but the two validators use different gate mechanisms.
The hack-side Validator.validateWriteMode (lines 128–152) and platform-api FieldValidator.validateWriteMode both support FeatureGateAwareWriteModes and apply the same precedence rule: check for a specific matching gate first, then fall back to the default (empty gate). However, they obtain enabled gates differently:
- hack-side: Accepts an
EnabledGates []stringslice onRequestand checks membership withreq.IsFeatureGateEnabled(). - platform-api: Accepts a
featuregate.FeatureSetparameter and delegates tofeaturegate.IsGateEnabled().
Tests for hack-side gate-aware write-mode behavior exist in hack/api-codegen/pkg/validation/gated_writemode_test.go and cover the immutable-to-mutable override, service-set restrictions, and multi-gate precedence. The tests in validator_test.go focus on simpler cases (immutable unchanged). To reduce divergence risk, extract the write-mode resolution logic into a shared package, or ensure both validators accept compatible gate parameters.
🤖 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 `@hack/api-codegen/pkg/validation/validator_test.go` around lines 113 - 133,
Align hack-side Validator.validateWriteMode and platform-api
FieldValidator.validateWriteMode on a shared, compatible feature-gate input and
resolution mechanism. Preserve FeatureGateAwareWriteModes precedence: select a
matching specific gate before falling back to the default empty gate, while
retaining each validator’s existing write-mode behavior. Prefer reusing shared
resolution logic rather than maintaining separate EnabledGates and FeatureSet
implementations.
2 participants
Description
Merge order: #175 , #193, then this PR
Field validation on create/update (Phase 2)
Service-set field preservation (Phase 3)
Documentation-only changes
OpenAPI spec alignment (Phase 5)
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit