AGENT-1522: Graduate InternalReleaseImage from v1alpha1 to v1#2880
AGENT-1522: Graduate InternalReleaseImage from v1alpha1 to v1#2880pawanpinjarkar wants to merge 6 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pawanpinjarkar: This pull request references AGENT-1522 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. |
|
Hello @pawanpinjarkar! Some important instructions when contributing to openshift/api: |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughThis pull request introduces the InternalReleaseImage cluster-scoped singleton API: new Go types and validation markers, registration of InternalReleaseImage and InternalReleaseImageList with the machineconfiguration v1 scheme, updated generated CRD payloads to publish v1 schema text and tightened status validation, a YAML test suite covering create/update validation and reconciliation scenarios, and a change to hack/update-payload-crds.sh removing the old v1alpha1 internalreleaseimages glob. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml (1)
1-307: ⚡ Quick winConsider adding test for singleton name constraint.
The CRD has a validation rule enforcing
metadata.name == 'cluster', but there's no onCreate test verifying that a different name (e.g.,metadata.name: not-cluster) is properly rejected. This would help ensure the singleton constraint is working correctly.📋 Suggested test case
Add this test case to the
onCreatesection:- name: Should reject non-cluster name (singleton constraint) initial: | apiVersion: machineconfiguration.openshift.io/v1 kind: InternalReleaseImage metadata: name: not-cluster spec: releases: - name: ocp-release-bundle-4.18.0-x86_64 expectedError: "internalreleaseimage is a singleton, .metadata.name must be 'cluster'"🤖 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 `@machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml` around lines 1 - 307, Add an onCreate test to verify the singleton name constraint for InternalReleaseImage: in the onCreate array add a test (e.g., name "Should reject non-cluster name (singleton constraint)") that creates an InternalReleaseImage with metadata.name: not-cluster and a minimal spec.releases entry, and set expectedError to the singleton rejection message (e.g., "internalreleaseimage is a singleton, .metadata.name must be 'cluster'"). Locate the block handling onCreate tests and the InternalReleaseImage test entries (symbols: onCreate, InternalReleaseImage, metadata.name, spec.releases) and ensure the new test follows the same YAML structure as the other onCreate cases.
🤖 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 `@machineconfiguration/v1/types_internalreleaseimage.go`:
- Around line 19-24: The type comment for InternalReleaseImage lacks
documentation that this is a cluster-scoped singleton; update the top-of-file
doc comment for InternalReleaseImage to state that only one instance is allowed
and its metadata.name must equal "cluster" (i.e., singleton resource enforced by
validation metadata.name == 'cluster'), and mention it is cluster-scoped so
consumers understand it represents a single cluster-wide configuration.
- Around line 103-120: Update the comment for the Conditions field to document
the MinItems/MaxItems validation and optionality: state that when the optional
Conditions slice is present it must contain between 1 and 5 metav1.Condition
entries (inclusive) corresponding to the allowed types (Mounted, Installing,
Available, Removing, Degraded), but the entire field may be omitted when no
status is available; ensure this text is added right above the Conditions
[]metav1.Condition `json:"conditions,omitempty"` declaration so reviewers can
see the +kubebuilder:validation:MinItems=1 and
+kubebuilder:validation:MaxItems=5 constraints are documented.
- Around line 70-78: Update the comment for the Conditions field on the
InternalReleaseImage type to document the +kubebuilder:validation:MinItems=1 and
+kubebuilder:validation:MaxItems=20 constraints and clarify the +optional
behavior: state that the field may be omitted entirely, but if present it must
contain between 1 and 20 metav1.Condition entries (valid types: Degraded), and
note the listType=map/listMapKey behavior as already described; edit the comment
immediately above the Conditions []metav1.Condition declaration to include this
text so reviewers can see the validation rules and optionality.
---
Nitpick comments:
In
`@machineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml`:
- Around line 1-307: Add an onCreate test to verify the singleton name
constraint for InternalReleaseImage: in the onCreate array add a test (e.g.,
name "Should reject non-cluster name (singleton constraint)") that creates an
InternalReleaseImage with metadata.name: not-cluster and a minimal spec.releases
entry, and set expectedError to the singleton rejection message (e.g.,
"internalreleaseimage is a singleton, .metadata.name must be 'cluster'"). Locate
the block handling onCreate tests and the InternalReleaseImage test entries
(symbols: onCreate, InternalReleaseImage, metadata.name, spec.releases) and
ensure the new test follows the same YAML structure as the other onCreate cases.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 662c10ce-ee1a-4b46-8b2b-217c2756384d
⛔ Files ignored due to path filters (6)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
hack/update-payload-crds.shmachineconfiguration/v1/register.gomachineconfiguration/v1/tests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1/types_internalreleaseimage.gopayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yaml
💤 Files with no reviewable changes (1)
- hack/update-payload-crds.sh
|
/assign @yuqi-zhang |
pawanpinjarkar
force-pushed
the
pr-2863-branch
branch
from
21badc0 to
31e0df0
Compare
|
/lgtm Note for the reviewers: this is required for the NoRegistryClusterInstall feature promotion, see #2859 |
|
Scheduling tests matching the |
|
/retest-required |
|
/verified later @pawanpinjarkar |
openshift-ci-robot
added
verified-later
verified
|
@pawanpinjarkar: This PR has been marked to be verified later by 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. |
|
/retest-required |
For this PR I would say let's verify through CI directly. Like what we did for openshift/machine-config-operator#6076 (comment) . We can't merge this by itself, and we shouldn't merge without proving all the component pieces work together with an overall switch The overall API itself looks mostly good, just some minor cleanup needed |
|
@yuqi-zhang: This PR has been marked to be verified later by 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. |
| // conditions represent the observations of an internal release image current state. Valid types are: | ||
| // Mounted, Installing, Available, Removing and Degraded. | ||
| // | ||
| // If Mounted is true, that means that a valid ISO has been discovered and mounted on one of the cluster nodes. |
There was a problem hiding this comment.
Just a question, can multiple conditions be true at the same time? Or can only 1 be true at a time?
There was a problem hiding this comment.
Potentially there could be more conditions true simultaneously. For example, Mounted will be set to True when the ISO will be discovered, and if the user decides to add its content (by editing the IRI resource) also Installing will become true
|
/verified cancel |
|
@yuqi-zhang: The 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. |
|
/verified remove |
openshift-ci-robot
removed
the
verified
Lets try this out. Lets copy the v1alpha1 schema to a manual override in the v1 API so that the v1 version includes both schemas, merge that, and then migrate the rest over the next few days |
Add manual CRD override to include v1alpha1 schema alongside v1 schema for InternalReleaseImage API. This allows both API versions to coexist during the graduation period, enabling gradual migration from v1alpha1 to v1 across dependent repositories. The manual override contains the complete v1alpha1 schema from the featuregated manifest, which will be merged with the v1 schema during code generation to produce a CRD that serves both versions. This addresses the review feedback to support both schemas simultaneously rather than requiring all consumers to update at once. Related: openshift#2880 (comment)
During code generation, the manifest-merge tool combines this override with the v1 schema generated from Go types to produce a final CRD that serves both versions: - v1 (storage: true): New stable API with MaxItems=5 for Conditions - v1alpha1 (storage: false): Original alpha API with MaxItems=20 for Conditions Both versions are served, but v1 is the storage version. Resources submitted via v1alpha1 are automatically converted to v1 for storage in etcd, then converted back to v1alpha1 when read, ensuring backward compatibility without requiring simultaneous updates across all consuming repositories.
|
@JoelSpeed Please review the new changes. |
|
/retest-required |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, bfournie, everettraven 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 |
|
/test minor-e2e-upgrade-minor |
|
/test minor-e2e-upgrade-minor This job seems to have some issue |
|
/verified later |
|
@pawanpinjarkar: 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. |
|
/verified later @pawanpinjarkar |
|
/retest-required |
openshift-ci-robot
added
verified-later
verified
|
@pawanpinjarkar: This PR has been marked to be verified later by 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-aws-ovn-hypershift |
|
@pawanpinjarkar: all tests passed! 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. |
10 participants
Original PR from @sadasu #2863