ROSAENG-64886: fix conversion-gen CRD detection to require TypeMeta/ObjectMeta embeds - #300
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-64886 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 task 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. |
openshift-ci
Bot
added
the
approved
WalkthroughThe generator now records embedded types and identifies top-level CRD resources through ChangesCRD discovery and upgrade policy API
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/pkg/conversion/generator.go`:
- Around line 755-779: Update Generator.isCRDResource to inspect ti.Fields and
require Spec and Status fields whose GoType values match typeName+"Spec" and
typeName+"Status", respectively, in addition to the existing sibling-type and
embedded-metadata checks. Return true only when both matching fields are
present.
🪄 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: 09fb3d83-bc13-4788-90dd-7d042213f07b
📒 Files selected for processing (1)
hack/api-codegen/pkg/conversion/generator.go
| statusName := typeName + "Status" | ||
| _, hasSpec := g.typeInfos[specName] | ||
| _, hasStatus := g.typeInfos[statusName] | ||
| if hasSpec && hasStatus { |
There was a problem hiding this comment.
This was required before and now isn't, is that expected?
There was a problem hiding this comment.
yes, the Spec+Status check is still required and still happens — it's inside isCRDResource(). The isCRDResource() function is a superset: it checks sibling types AND embeds AND (after our latest change) matching Spec/Status fields.
|
/test ci/prow/unit |
|
/retest |
|
/test on-demand-e2e |
a2a35df to
63c3f21
Compare
|
@cdoan1: The following test 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. |
…bjectMeta embeds The naive Spec+Status heuristic incorrectly treated nested sub-types (e.g. ControlPlaneUpgradePolicy) as top-level CRD resources, generating conversion functions that referenced non-existent wrapper types. Now only types with a wrapper struct embedding metav1.TypeMeta and metav1.ObjectMeta are recognized as CRD resources. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review feedback: in addition to checking for sibling FooSpec/FooStatus type definitions and TypeMeta/ObjectMeta embeds, also verify the wrapper struct declares Spec and Status fields whose GoType matches the sibling types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
63c3f21 to
618a4da
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/controlplaneupgradepolicyspec_types.go`:
- Line 35: Update the cron validation Pattern on the control plane upgrade
policy spec to reject zero step values in every \*/... alternative across all
five fields. Replace each step subpattern with an allow-list of valid nonzero
values while preserving the existing accepted cron syntax and ranges.
In `@api/v1alpha1/public/openapi.yaml`:
- Around line 2265-2330: Add a controlPlaneUpgradePolicy status object to the
ClusterStatusInfo schema, including the nextRun date-time field exposed by the
CRD. Match the CRD’s property name, type, and format so OpenAPI-generated
clients can represent the policy status.
In `@hyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yaml`:
- Around line 112-113: Update the enum in the cluster CRD schema to retain the
deprecated ControlPlane and ControlPlaneCVE values alongside UserInitiated and
ServiceInitiated. Do not remove the legacy values until stored Cluster resources
have been migrated in a later release.
🪄 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: 9bf02e13-399b-4807-abb9-0776ce999358
⛔ Files ignored due to path filters (1)
api/v1alpha1/public/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
api/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/openapi.yamlhyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yaml
| controlPlaneUpgradePolicy: | ||
| description: ControlPlaneUpgradePolicy is the control plane upgrade policy defined by the user. | ||
| properties: | ||
| nextRun: | ||
| description: NextRun is the time the upgrade should run for "manual" upgrade policies | ||
| format: date-time | ||
| type: string | ||
| schedule: | ||
| description: |- | ||
| Schedule defines a cron expression that calculates the next automatic upgrade scheduling. | ||
| The cron expression must follow the standard 5-field format: | ||
| ┌───────────── minute (0 - 59) | ||
| │ ┌───────────── hour (0 - 23) | ||
| │ │ ┌───────────── day of month (1 - 31) | ||
| │ │ │ ┌───────────── month (1 - 12) | ||
| │ │ │ │ ┌───────────── day of week (0 - 6) (Sunday to Saturday) | ||
| │ │ │ │ │ | ||
| * * * * * | ||
| maxLength: 256 | ||
| 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])*)$ | ||
| type: string | ||
| scheduleType: | ||
| description: |- | ||
| ScheduleType indicates if the control plane upgrade policy is "manual" and it's executed only one time or | ||
| whether it is "automatic" where an expression will calculate recurrent upgrades. | ||
| enum: | ||
| - Manual | ||
| - Automatic | ||
| type: string | ||
| updateType: | ||
| description: |- | ||
| UpdateType indicates if it is a control plane upgrade policy defined by the user or | ||
| triggered by Red Hat for addressing critical CVEs. | ||
| enum: | ||
| - UserInitiated | ||
| - ServiceInitiated | ||
| type: string | ||
| upgradeScope: | ||
| description: |- | ||
| UpgradeScope indicates if minor version upgrades are allowed for automatic upgrades. | ||
| Manual upgrades always allow it. | ||
| enum: | ||
| - PatchOnly | ||
| - PatchAndMinor | ||
| type: string | ||
| version: | ||
| description: Version is the desired upgrade version on "manual" upgrade policies. | ||
| maxLength: 64 | ||
| type: string | ||
| required: | ||
| - scheduleType | ||
| - updateType | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: version and nextRun are required when scheduleType is Manual | ||
| rule: self.scheduleType != 'Manual' || (has(self.version) && has(self.nextRun)) | ||
| - message: schedule must not be set when scheduleType is Manual | ||
| rule: self.scheduleType != 'Manual' || !has(self.schedule) | ||
| - message: schedule is required when scheduleType is Automatic | ||
| rule: self.scheduleType != 'Automatic' || has(self.schedule) | ||
| - message: version and nextRun must not be set when scheduleType is Automatic | ||
| rule: self.scheduleType != 'Automatic' || (!has(self.version) && !has(self.nextRun)) | ||
| - message: upgradeScope must not be set when scheduleType is Manual | ||
| rule: self.scheduleType != 'Manual' || !has(self.upgradeScope) | ||
| - message: upgradeScope is required when scheduleType is Automatic | ||
| rule: self.scheduleType != 'Automatic' || has(self.upgradeScope) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the policy status to the OpenAPI response schema.
The CRD exposes status.controlPlaneUpgradePolicy.nextRun, but ClusterStatusInfo does not declare this field. OpenAPI-generated clients cannot model the new policy status. Add the matching status object to ClusterStatusInfo.
🤖 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 2265 - 2330, Add a
controlPlaneUpgradePolicy status object to the ClusterStatusInfo schema,
including the nextRun date-time field exposed by the CRD. Match the CRD’s
property name, type, and format so OpenAPI-generated clients can represent the
policy status.
|
/lgtm |
|
[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 |
1225902
into
openshift-online:main
3 participants
The naive Spec+Status heuristic incorrectly treated nested sub-types (e.g. ControlPlaneUpgradePolicy) as top-level CRD resources, generating conversion functions that referenced non-existent wrapper types. Now only types with a wrapper struct embedding metav1.TypeMeta and metav1.ObjectMeta are recognized as CRD resources.
Otherewise, we don't merge this, and top-level CRD need to define TypeMeta/ObjectMeta embeds
The CRD YAML change is small and entirely mechanical — it's a side-effect of regenerating the CRD manifests (make manifests) after updating the Go types.
Here's the causal chain:
constants.go.
UserInitiated/ServiceInitiated. The rest of the new controlPlaneUpgradePolicy block landed in the OpenAPI spec (openapi.yaml), not the CRD bases file, because these are the public API types — the CRD is generated from the internal types in the
operator.
The two-line enum change (ControlPlane → UserInitiated, ControlPlaneCVE → ServiceInitiated) means the internal CRD types already had a ControlPlaneUpgradePolicySpec with an updateType field, but its enum values were renamed to be more
descriptive and consistent with the new public-facing naming convention. The old names described what was being upgraded; the new names describe who initiated the upgrade — which better matches the field's semantic purpose.
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
Bug Fixes