ROSAENG-62084: refactor: complete auto generated files and map nodepool namespace to cluster_id - #313
Merged
openshift-merge-bot[bot] merged 15 commits intoAug 11, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces ChangesPublic API and schema changes
Bridge generation and client integration
Generation and SDK validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIAnnotations
participant bridge-gen
participant Clientset
participant Adapter
participant PlatformAPI
APIAnnotations->>bridge-gen: Parse +bridge markers
bridge-gen->>Clientset: Generate platform clients and bridge mappings
Clientset->>Adapter: Select mappings from request resource
Adapter->>PlatformAPI: Send adapted request
PlatformAPI-->>Adapter: Return adapted response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
gdbranco
force-pushed
the
docs/rosaeng-62084-np-parent-mapping
branch
from
August 11, 2026 15:00
d474687 to
44221bd
Compare
gdbranco
changed the title
Contributor
Author
|
/test on-demand-e2e |
gdbranco
force-pushed
the
docs/rosaeng-62084-np-parent-mapping
branch
from
August 11, 2026 15:11
071331a to
81af3e6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yaml (1)
275-280: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestore upper bounds on
nodeLabelsandtaints.Two size limits disappeared in this file:
- Line 275-280:
nodeLabelslostmaxProperties: 100.- Line 1819-1861:
taintslostmaxItems.Both fields are user-writable and both are now unbounded. A single NodePool can carry an arbitrary number of labels and taints. Object size in etcd and the operator reconcile cost both scale with these collections. The sibling field
spec.labelsat line 70-76 still keepsmaxProperties: 100, so the CRD is now inconsistent with itself.The matching schema in
api/v1alpha1/public/openapi.yamlalso has no bound on either field, so no other layer enforces a limit.If the removal came from regenerating against a newer upstream HyperShift, add the bounds back with explicit kubebuilder markers on the HyperFleet-owned passthrough type.
🔍 Verification script
#!/bin/bash # Description: Check whether the size markers still exist on the Go source types. set -euo pipefail # Look for the markers on nodeLabels and taints in the API module. rg -nP -C6 '\b(NodeLabels|Taints)\b' --type=go -g 'api/**' -g '!**/*_test.go' # Confirm which passthrough types still declare MaxProperties/MaxItems. rg -nP 'kubebuilder:validation:(MaxProperties|MaxItems)' --type=go -g 'api/**'🤖 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 `@hyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yaml` around lines 275 - 280, Restore the collection bounds for the HyperFleet-owned passthrough type: add the explicit kubebuilder validation markers limiting NodeLabels to 100 properties and Taints to the intended maximum item count. Regenerate the CRD and public OpenAPI schema so nodeLabels and taints enforce those limits consistently with spec.labels.
🧹 Nitpick comments (4)
docs/api/v2-sdk-initiative.md (1)
171-180: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument resource-specific mapping selection.
The transport layer selects resource-keyed mappings from the request path. These sections describe only wire-to-metadata mappings. State the resource-keyed contract and include the NodePool namespace-to-
cluster_idmapping example. This will help contributors update and verify the correct resource mapping.Also applies to: 336-337
🤖 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 `@docs/api/v2-sdk-initiative.md` around lines 171 - 180, Update the mapping documentation around the bridge-gen markers and the corresponding resource-mapping section to state that transport mappings are selected by resource from the request path. Clarify that mappings are resource-keyed rather than only generic wire-to-metadata mappings, and add the NodePool example mapping its namespace field to cluster_id. Apply the same clarification to the additional mapping reference noted later in the document.api/v1alpha1/public/controlplaneupgradepolicystatus_types.go (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit
+optionalmarker toNextRun.Every other optional field in this package carries an explicit
+optionalmarker.NextRunrelies only on the pointer type andomitempty. Add the marker to keep the generated schema deterministic and consistent with the rest of the package.♻️ Proposed change
// NextRun is the time when the control plane will upgrade. // When the ScheduleType is "manual" it will match with the NextRun defined by the user. // When the ScheduleType is "automatic" it will be calculated from the Schedule cron expression. + // +optional NextRun *metav1.Time `json:"nextRun,omitempty"`🤖 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 `@api/v1alpha1/public/controlplaneupgradepolicystatus_types.go` around lines 11 - 14, Add the explicit +optional marker to the NextRun field in the control plane upgrade policy status type, alongside its existing comments, while preserving the field type and JSON tag.api/v1alpha1/public/openapi.yaml (1)
3377-3396: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRequired passthrough fields are declared as untyped objects.
management(line 3377) andosImageStream(line 3394) are now required, but both are declared as baretype: objectwith noproperties. The CEL rule onmanagementreferencesself.upgradeTypeandself.inPlace, neither of which the schema declares. The CRD inhyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yamldoes declare these properties (lines 136-269 and 286-299).A consumer that generates a client from this document receives an untyped map for a mandatory field. The consumer cannot discover that
management.upgradeTypeis itself required. The same pattern applies toautoNode,dns,release, andsecretEncryptioninHostedClusterSpecPassthrough.Expand at least the required passthrough objects to declare their properties, so the document remains a usable client contract.
As per coding guidelines: "Follow an OpenAPI-first API design."
🤖 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 `@api/v1alpha1/public/openapi.yaml` around lines 3377 - 3396, Expand the required passthrough object schemas in the OpenAPI document instead of leaving them as untyped objects. Update management and osImageStream, and apply the same property definitions to autoNode, dns, release, and secretEncryption in HostedClusterSpecPassthrough, matching the corresponding CRD schemas so required nested fields such as management.upgradeType are discoverable and the existing CEL validation remains valid.Source: Coding guidelines
platform-api/pkg/conversion/v1alpha1/controlplaneupgradepolicy.go (1)
56-59: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe enrichment overlay can clobber user-supplied fields.
conversion.ServiceSetFieldsinplatform-api/pkg/conversion/types.go(lines 11-72) declares noomitemptyon any JSON tag. Marshaling it therefore emits every key, including empty strings, empty slices, and nulls. Line 58 unmarshals that complete blob ontocrdSpec.No
ControlPlaneUpgradePolicySpecfield name collides with aServiceSetFieldskey today, so the current behavior is correct. The pattern is still fragile. If a future spec field is named to match aServiceSetFieldskey, the user value is silently replaced by the platform zero value, because the key is always present in the marshaled output.Consider emitting the overlay only for fields the service actually set, or restricting the overlay to an explicit allow-list per resource in the generator template.
🔍 Verification script
#!/bin/bash # Description: Detect JSON tag collisions between ServiceSetFields and the CRD spec types it overlays. set -euo pipefail # Extract ServiceSetFields JSON keys. rg -nP -o 'json:"([a-zA-Z0-9_]+)' platform-api/pkg/conversion/types.go # Extract JSON keys from every CRD spec type targeted by an Unproject helper. rg -nP -o 'json:"([a-zA-Z0-9_]+)' --type=go -g 'api/v1alpha1/*.go' # List every generated conversion file that uses the same overlay pattern. rg -nP -C6 'ssData, _ := json\.Marshal\(enrichment\)' --type=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 `@platform-api/pkg/conversion/v1alpha1/controlplaneupgradepolicy.go` around lines 56 - 59, Update the enrichment overlay in the conversion function containing the ssData marshal/unmarshal sequence so it applies only explicitly supported ServiceSetFields keys, rather than unmarshaling the complete enrichment object onto crdSpec. Preserve user-supplied spec fields when enrichment values are empty, null, or unrelated, and use the resource’s explicit allow-list or generated mapping if available.
🤖 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 `@api/v1alpha1/public/clusterspec_types.go`:
- Around line 41-43: Add the `+hyperfleet:write-mode=mutable` marker to the
`ControlPlaneUpgradePolicy` field declaration so its `controlPlaneUpgradePolicy`
entry is included in FieldRegistry and validated by FieldValidator.
In `@api/v1alpha1/public/controlplaneupgradepolicyspec_types.go`:
- Around line 34-37: The Schedule validation pattern rejects valid standard cron
range-step expressions and ranges within comma lists. Update the kubebuilder
Pattern on Schedule to accept forms such as 1-5/2 and 0-3,8-11 across applicable
fields, while preserving existing supported syntax and bounds; alternatively,
explicitly document the restricted cron subset in the Schedule field comment.
In `@api/v1alpha1/public/openapi.yaml`:
- Around line 3331-3341: Review the passthrough required-field lists in
api/v1alpha1/public/openapi.yaml at lines 3331-3341 and 3468-3471: remove fields
with server-side defaults from required and declare those defaults, while
confirming caller-supplied fields remain required; specifically reassess
autoNode, fips, pullSecret, services, sshKey, management, and osImageStream. In
hyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yaml:1885, add a
default for spec.nodePool.osImageStream or implement a migration that backfills
existing NodePools before shipping the CRD.
- Around line 2318-2330: Implement equivalent REST-layer validation for
controlPlaneUpgradePolicy in the /api/v0/clusters request path, enforcing all
six scheduleType, version, nextRun, schedule, and upgradeScope constraints shown
in the OpenAPI CEL rules. Reuse the existing validation framework and return the
established client-error response for invalid payloads; add coverage for each
invalid combination. Do not rely solely on OpenAPI or downstream CRD admission.
In `@CLAUDE.md`:
- Line 57: Update the bridge-gen description in CLAUDE.md to replace “wire
generation” with wording that accurately describes “bridge and platform
generation for clientset,” while preserving the existing clientset context.
In `@clientset/docs/architecture.md`:
- Around line 250-252: Update the HyperfleetV1alpha1 architecture example and
its nearby explanation to use the exposed Clientset.V1alpha1Public() method
instead of constructing a client through c.generated.V1alpha1(). Remove all
references to the generated client path in that example.
In `@clientset/transport/bridge_test.go`:
- Around line 86-102: Convert the tests and helper functions in bridge_test.go,
including mustAdaptRequest and mustAdaptResponse and the tests around lines
527–562, from testing.T assertions to Ginkgo specs with Gomega matchers. Replace
*testing.T parameters, Helper calls, Fatalf failures, and test function
structure with the repository’s established Ginkgo/Gomega patterns while
preserving each test’s existing behavior and coverage.
In `@clientset/transport/bridge.go`:
- Around line 204-207: Update the response adaptation branch in the relevant
bridge method so list responses with an empty mappings configuration bypass
adaptList and return the original body unchanged. Preserve existing adaptation
when a resource mapping exists, and add a regression test covering a list
response with empty mappings that verifies no metadata is injected.
In `@platform-api/pkg/conversion/v1alpha1/controlplaneupgradepolicy.go`:
- Around line 31-62: Update the conversion-gen template that generates
projectControlPlaneUpgradePolicySpec, projectControlPlaneUpgradePolicyStatus,
and UnprojectControlPlaneUpgradePolicy so every json.Marshal and json.Unmarshal
error is checked and propagated instead of discarded. Change each helper’s
signature to return its converted value plus an error, preserve nil handling for
UnprojectControlPlaneUpgradePolicy, and propagate errors from both the primary
conversion and service-set enrichment so callers cannot receive partially
populated results.
---
Outside diff comments:
In `@hyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yaml`:
- Around line 275-280: Restore the collection bounds for the HyperFleet-owned
passthrough type: add the explicit kubebuilder validation markers limiting
NodeLabels to 100 properties and Taints to the intended maximum item count.
Regenerate the CRD and public OpenAPI schema so nodeLabels and taints enforce
those limits consistently with spec.labels.
---
Nitpick comments:
In `@api/v1alpha1/public/controlplaneupgradepolicystatus_types.go`:
- Around line 11-14: Add the explicit +optional marker to the NextRun field in
the control plane upgrade policy status type, alongside its existing comments,
while preserving the field type and JSON tag.
In `@api/v1alpha1/public/openapi.yaml`:
- Around line 3377-3396: Expand the required passthrough object schemas in the
OpenAPI document instead of leaving them as untyped objects. Update management
and osImageStream, and apply the same property definitions to autoNode, dns,
release, and secretEncryption in HostedClusterSpecPassthrough, matching the
corresponding CRD schemas so required nested fields such as
management.upgradeType are discoverable and the existing CEL validation remains
valid.
In `@docs/api/v2-sdk-initiative.md`:
- Around line 171-180: Update the mapping documentation around the bridge-gen
markers and the corresponding resource-mapping section to state that transport
mappings are selected by resource from the request path. Clarify that mappings
are resource-keyed rather than only generic wire-to-metadata mappings, and add
the NodePool example mapping its namespace field to cluster_id. Apply the same
clarification to the additional mapping reference noted later in the document.
In `@platform-api/pkg/conversion/v1alpha1/controlplaneupgradepolicy.go`:
- Around line 56-59: Update the enrichment overlay in the conversion function
containing the ssData marshal/unmarshal sequence so it applies only explicitly
supported ServiceSetFields keys, rather than unmarshaling the complete
enrichment object onto crdSpec. Preserve user-supplied spec fields when
enrichment values are empty, null, or unrelated, and use the resource’s explicit
allow-list or generated mapping if available.
🪄 Autofix
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: 3dab1b71-5493-436b-80e1-38f41f0c3291
⛔ Files ignored due to path filters (2)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*api/v1alpha1/zz_generated.passthrough.gois excluded by!**/zz_generated*
📒 Files selected for processing (32)
CLAUDE.mdMakefileapi/v1alpha1/cluster_types.goapi/v1alpha1/nodepool_types.goapi/v1alpha1/public/cluster_types.goapi/v1alpha1/public/clusterspec_types.goapi/v1alpha1/public/clusterstatus_types.goapi/v1alpha1/public/constants.goapi/v1alpha1/public/controlplaneupgradepolicyspec_types.goapi/v1alpha1/public/controlplaneupgradepolicystatus_types.goapi/v1alpha1/public/nodepool_types.goapi/v1alpha1/public/openapi.yamlclientset/docs/architecture.mdclientset/hyperfleet.goclientset/platform/bridge_wrappers_generated.goclientset/platform/options.goclientset/platform/platform_test.goclientset/transport/bridge.goclientset/transport/bridge_mappings_generated.goclientset/transport/bridge_test.goclientset/transport/wire_mappings_generated.godocs/api/v2-sdk-initiative.mdhack/api-codegen/pkg/conversion/generator.gohack/api-codegen/pkg/registry/field_metadata.gohack/api-codegen/pkg/registry/field_metadata.jsonhack/clientset/cmd/bridge-gen/go.modhack/clientset/cmd/bridge-gen/main.gohyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yamlhyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yamlplatform-api/pkg/conversion/types.goplatform-api/pkg/conversion/v1alpha1/controlplaneupgradepolicy.gotest/e2e-sdk/sdk_sanity_test.go
💤 Files with no reviewable changes (4)
- hack/api-codegen/pkg/registry/field_metadata.go
- hack/api-codegen/pkg/registry/field_metadata.json
- clientset/transport/wire_mappings_generated.go
- platform-api/pkg/conversion/types.go
Comment on lines
+41
to
+43
| // ControlPlaneUpgradePolicy is the control plane upgrade policy defined by the user. | ||
| // +optional | ||
| ControlPlaneUpgradePolicy *ControlPlaneUpgradePolicySpec `json:"controlPlaneUpgradePolicy,omitempty"` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine how the codegen interprets a missing write-mode marker.
set -euo pipefail
# Find the marker parsing logic.
rg -nP -C6 'write-mode' --type=go -g '!**/*_test.go'
# Find the default write-mode applied when the marker is absent.
rg -nP -C4 'WriteMode|writeMode' --type=go | head -80Repository: openshift-online/rosa-hyperfleet-api
Length of output: 300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file ---'
cat -n api/v1alpha1/public/clusterspec_types.go | sed -n '1,140p'
printf '%s\n' '--- marker occurrences across the repository ---'
rg -n -C3 'hyperfleet:write-mode|write-mode' . || true
printf '%s\n' '--- write-mode identifiers and likely generator code ---'
rg -n -C4 'WriteMode|writeMode|write mode|write-mode' --glob '!vendor/**' --glob '!**/*_test.go' . || true
printf '%s\n' '--- module and generator-related files ---'
git ls-files | rg -i '(^|/)(go\.mod|Makefile|Dockerfile|hack|cmd|controller-gen|codegen|generator|openapi|crd|types)' | head -200Repository: openshift-online/rosa-hyperfleet-api
Length of output: 50393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ClusterSpec ---'
cat -n api/v1alpha1/public/clusterspec_types.go | sed -n '1,100p'
printf '%s\n' '--- marker scanner validation and parsing ---'
cat -n hack/api-codegen/cmd/marker-scanner/main.go | sed -n '1,220p'
fd -i 'marker' hack/api-codegen --type f -x sh -c 'echo --- "$1"; cat -n "$1"' sh {} \;
printf '%s\n' '--- ControlPlaneUpgradePolicy references ---'
rg -n -C5 'ControlPlaneUpgradePolicy|controlPlaneUpgradePolicy' \
api hack hyperfleet-operator platform-api hyperfleet-db 2>/dev/null | head -240
printf '%s\n' '--- generated registry entries for upgrade-policy fields ---'
rg -n -C3 -i 'upgrade.?policy|control.?plane' hack/api-codegen/pkg/registry/field_metadata.go | head -160Repository: openshift-online/rosa-hyperfleet-api
Length of output: 30098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- marker package files ---'
git ls-files hack/api-codegen/pkg/markers
rg -n -C8 'func .*Validate|type Scanner|write-mode|Parse|marker' hack/api-codegen/pkg/markers
printf '%s\n' '--- codegen inputs and registry generation ---'
rg -n -C5 'marker-scanner|input-dirs|field_metadata|FieldRegistry|api-codegen' Makefile hack scripts .github 2>/dev/null | head -300
printf '%s\n' '--- registry field paths related to ClusterSpec ---'
rg -n -C2 '"(displayName|deleteProtection|expirationTimestamp|properties|tags|hostedCluster|controlPlaneUpgradePolicy)"' \
hack/api-codegen/pkg/registry/field_metadata.go hack/api-codegen/pkg/registry/field_metadata.json 2>/dev/null || true
printf '%s\n' '--- generated CRD mutability or policy metadata ---'
rg -n -C4 'controlPlaneUpgradePolicy|writeMode|write-mode|immutable|mutable' \
hyperfleet-operator/config hyperfleet-operator 2>/dev/null | head -260Repository: openshift-online/rosa-hyperfleet-api
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- registry consumers ---'
rg -n -C5 'registry\.FieldRegistry|FieldRegistry|WriteMode|FeatureGateAwareWriteModes|ServiceSet|Immutable|Mutable' \
hack/api-codegen platform-api hyperfleet-operator api --glob '*.go' \
| rg -v 'field_metadata.go|pkg/markers|_test.go' | head -320
printf '%s\n' '--- conversion and generated REST handling ---'
rg -n -C8 'ControlPlaneUpgradePolicy|write.mode|writeMode|field metadata|registry' \
hack/api-codegen/cmd/conversion-gen hack/api-codegen/pkg platform-api/pkg/conversion \
--glob '*.go' | head -320
printf '%s\n' '--- marker scanner root traversal ---'
sed -n '66,145p' hack/api-codegen/pkg/markers/scanner.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 22501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- field validator request-field loop ---'
sed -n '1,180p' platform-api/pkg/validation/field_validator.go
printf '%s\n' '--- configuration verifier invocation ---'
rg -n -C8 'verify-configuration|ValidateAllFields|missing \+hyperfleet:write-mode' \
Makefile hack .github --glob '*.go' --glob 'Makefile*' --glob '*.yaml' --glob '*.yml' | head -240
printf '%s\n' '--- registry presence for the new field ---'
python3 - <<'PY'
from pathlib import Path
p = Path("hack/api-codegen/pkg/registry/field_metadata.go")
text = p.read_text()
for term in ("controlPlaneUpgradePolicy", "spec.controlPlaneUpgradePolicy"):
print(f"{term}: {term in text}")
PYRepository: openshift-online/rosa-hyperfleet-api
Length of output: 11193
Add +hyperfleet:write-mode=mutable to ControlPlaneUpgradePolicy.
Without the marker, controlPlaneUpgradePolicy is absent from FieldRegistry, and FieldValidator skips it. The marker adds explicit write-mode metadata.
🤖 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 `@api/v1alpha1/public/clusterspec_types.go` around lines 41 - 43, Add the
`+hyperfleet:write-mode=mutable` marker to the `ControlPlaneUpgradePolicy` field
declaration so its `controlPlaneUpgradePolicy` entry is included in
FieldRegistry and validated by FieldValidator.
Comment on lines
+34
to
+37
| // +kubebuilder:validation:MaxLength=256 | ||
| // +kubebuilder:validation:Pattern=`^(\*|([0-9]|1[0-9]|2[0-9]|3[0-9]|4[0-9]|5[0-9])|([0-9]|1[0-9]|2[0-9]|3[0-9]|4[0-9]|5[0-9])-([0-9]|1[0-9]|2[0-9]|3[0-9]|4[0-9]|5[0-9])|\*/([0-9]|1[0-9]|2[0-9]|3[0-9]|4[0-9]|5[0-9])|([0-9]|1[0-9]|2[0-9]|3[0-9]|4[0-9]|5[0-9])(,([0-9]|1[0-9]|2[0-9]|3[0-9]|4[0-9]|5[0-9]))*) (\*|([0-9]|1[0-9]|2[0-3])|([0-9]|1[0-9]|2[0-3])-([0-9]|1[0-9]|2[0-3])|\*/([0-9]|1[0-9]|2[0-3])|([0-9]|1[0-9]|2[0-3])(,([0-9]|1[0-9]|2[0-3]))*) (\*|([1-9]|1[0-9]|2[0-9]|3[0-1])|([1-9]|1[0-9]|2[0-9]|3[0-1])-([1-9]|1[0-9]|2[0-9]|3[0-1])|\*/([1-9]|1[0-9]|2[0-9]|3[0-1])|([1-9]|1[0-9]|2[0-9]|3[0-1])(,([1-9]|1[0-9]|2[0-9]|3[0-1]))*) (\*|([1-9]|1[0-2])|([1-9]|1[0-2])-([1-9]|1[0-2])|\*/([1-9]|1[0-2])|([1-9]|1[0-2])(,([1-9]|1[0-2]))*) (\*|[0-6]|[0-6]-[0-6]|\*/[0-6]|[0-6](,[0-6])*)$` | ||
| // +optional | ||
| Schedule *string `json:"schedule,omitempty"` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The cron pattern rejects valid 5-field cron expressions.
The pattern supports *, single values, simple ranges (1-5), whole-field steps (*/5), and comma lists of single values (1,2,3). It rejects two common standard forms:
- Step over a range:
1-5/2 - Range inside a list:
0-3,8-11
A user who supplies 0 0 1-5/2 * * receives a validation rejection even though the expression is valid cron. Consider relaxing the pattern for these forms, or documenting the restricted subset in the field comment so the limitation is explicit.
🤖 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 `@api/v1alpha1/public/controlplaneupgradepolicyspec_types.go` around lines 34 -
37, The Schedule validation pattern rejects valid standard cron range-step
expressions and ranges within comma lists. Update the kubebuilder Pattern on
Schedule to accept forms such as 1-5/2 and 0-3,8-11 across applicable fields,
while preserving existing supported syntax and bounds; alternatively, explicitly
document the restricted cron subset in the Schedule field comment.
gdbranco
force-pushed
the
docs/rosaeng-62084-np-parent-mapping
branch
2 times, most recently
from
August 11, 2026 15:29
c10f2e7 to
b0dffde
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/api-codegen/pkg/conversion/generator.go (1)
1211-1213: 🗄️ Data Integrity & Integration | 🔵 TrivialVerify regression coverage for the new resource-shape gate.
discoverResourcesnow requires<Resource>Spec,<Resource>Status, and<Resource>. Verify tests for a complete type set, a missingStatustype, and a missing root type inhack/api-codegen/pkg/conversion/generator_test.go. Regenerate outputs and fail verification on generated diffs.Based on learnings: “Run the appropriate build, test, lint, verification, manifest-generation, and code-generation targets before merging changes, and keep generated outputs up to date.”
🤖 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 1211 - 1213, Add regression coverage for discoverResources to verify resources are discovered only when <Resource>Spec, <Resource>Status, and <Resource> types all exist, including separate missing-Status and missing-root cases. Update generator_test.go fixtures/assertions accordingly, then regenerate outputs and run verification so any generated diff causes failure.Source: Learnings
🤖 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.
Nitpick comments:
In `@hack/api-codegen/pkg/conversion/generator.go`:
- Around line 1211-1213: Add regression coverage for discoverResources to verify
resources are discovered only when <Resource>Spec, <Resource>Status, and
<Resource> types all exist, including separate missing-Status and missing-root
cases. Update generator_test.go fixtures/assertions accordingly, then regenerate
outputs and run verification so any generated diff causes failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 360f866c-846c-4362-bdfd-dad89bfd1868
📒 Files selected for processing (1)
hack/api-codegen/pkg/conversion/generator.go
…namespace
Platform-api returns cluster_id in every NodePool response (required field per
OpenAPI schema) but there was no +wire:field mapping for it. The Adapter left it
as an unmapped top-level key that the Kubernetes decoder silently dropped, so
NodePool.Namespace was always empty after any Get/List/Create call.
Add +wire:field=cluster_id,meta=namespace to the NodePool type. wire-gen picks
this up and adds {Wire: "cluster_id", Meta: "namespace"} to defaultMappings, so
adaptItem now lifts the value into metadata.namespace. The mapping is a no-op for
Cluster responses since they carry no cluster_id field.
Add an assertion in the SDK e2e sanity test to catch any regression.
All other e2e targets (test-e2e-api, test-e2e-cli, test-e2e-zoa) pass the platform API URL as E2E_BASE_URL into the test process. The test-e2e-sdk target was using the bare BASE_URL name on both sides. Rename to match.
… api - Rename transport/wire.go → bridge.go, wire_mappings_generated.go → bridge_mappings_generated.go - Rename wrappers/ package to platform/; wire_wrappers_generated.go → bridge_wrappers_generated.go - Rename hack/clientset/cmd/wire-gen/ → bridge-gen/; update go.mod module path - Update bridge-gen modes: mappings → bridge, wrappers → platform - Rename +wire:field/watch/wait markers → +bridge:field/watch/wait in all CRD types - Rename FieldMapping.Wire → FieldMapping.Bridge; update all usages in transport and generator - Update all import paths, Makefile variables, and doc references
…te architecture.md - WIRE_INPUT/OUTPUT_DIR/PKG → BRIDGE_INPUT/OUTPUT_DIR/PKG in Makefile - Fix stale architecture.md references: variable names, file paths (wire_* → bridge_*), package qualifier (wrappers.* → platform.*), directory paths (wrappers/ → platform/)
…gets generate-all: runs manifests, deepcopy, passthrough, conversion, clientset, and openapi in one pass verify-all: fails if any generated output (codegen, conversion, clientset, openapi) is out of date
…marker rename - Replace hostedclusterspec.passthrough.go with zz_generated.passthrough.go (passthrough-gen output) - Regenerate zz_generated.deepcopy.go: Configuration field type changed to v1beta1.ClusterConfiguration - Regenerate field_metadata registry, CRD bases, openapi.yaml, conversion types to reflect +bridge:field/watch/wait marker renames from +wire:
cluster_id (np.Namespace) is a customer resource identifier; logging it may expose customer data in test output.
…n file ControlPlaneUpgradePolicy never existed as a standalone CRD type; only its Spec/Status sub-types are embedded in Cluster. The discoverResources() logic inferred it as a resource from matching Spec+Status names and generated a file referencing a non-existent root type. Fix: require the root type to exist in typeInfos (parsed from the actual Go struct declarations) before treating a Spec+Status pair as a top-level resource. Delete the stale generated file.
- bridge.go: bypass adaptList when mappings is empty so list responses are returned unchanged instead of injecting empty metadata into items - bridge_test.go: regression test covering list with empty mappings - architecture.md: fix HyperfleetV1alpha1 example to use V1alpha1Public() - CLAUDE.md: update bridge-gen description from "wire generation" to "bridge and platform generation"
- loader.go: forward +optional/+required from upstream HyperShift field doc comments into generated passthrough types via an explicit allowlist (upstreamForwardedMarkerPrefixes); fixes autoNode: Required value in tests - crd-variants: add --strip-passthrough-cel mode that auto-detects passthrough subtrees via Go AST and strips x-kubernetes-validations in-place; replaces manual flag; removes emoji from output - Makefile: codegen-registry now depends on codegen-passthrough so any direct invocation of codegen-registry, codegen-conversion, or verify-conversion always regenerates the passthrough file first - Regenerated CRDs and zz_generated.passthrough.go
…ll to generate/verify - generate-deepcopy: what generate was (deepcopy only) - verify-mod: what verify was (go.mod tidiness) - generate: full pipeline (passthrough -> deepcopy -> registry -> manifests -> conversion -> clientset -> openapi) - verify: all generated output checks + verify-mod - generate-all/verify-all removed - generate-clientset now depends on codegen-conversion (reads public types) - generate-openapi now depends on codegen-conversion (reads public types) - Update CLAUDE.md to reflect new target names
….yaml Fields are +optional upstream in HyperShift; now correctly propagated through passthrough-gen and reflected in the generated OpenAPI spec.
…eturn, fix ratelimit test timeout
… before interface comment
gdbranco
force-pushed
the
docs/rosaeng-62084-np-parent-mapping
branch
from
August 11, 2026 19:13
da34c0f to
654041a
Compare
Contributor
Author
|
/test on-demand-e2e |
cdoan1
reviewed
Aug 11, 2026
| # so those are transitively covered; they are listed explicitly here for clarity. | ||
| generate: codegen-registry manifests codegen-conversion generate-clientset generate-openapi | ||
|
|
||
| verify: verify-codegen verify-conversion verify-clientset verify-openapi verify-mod |
cdoan1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Collaborator
|
@gdbranco: This pull request references ROSAENG-62084 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 epic 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. |
Collaborator
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1, gdbranco 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
openshift-merge-bot
Bot
merged commit Aug 11, 2026
93162f5
into
openshift-online:main
14 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
Enhancements