AGENT-1522: Graduate InternalReleaseImage from v1alpha1 to v1#2880
AGENT-1522: Graduate InternalReleaseImage from v1alpha1 to v1#2880pawanpinjarkar wants to merge 3 commits into
Conversation
- Created machineconfiguration/v1/types_internalreleaseimage.go with v1 API - Updated compatibility level from 4 (alpha) to 1 (stable/GA) - Registered InternalReleaseImage types in v1 scheme - Created v1 integration test suite - Updated payload CRD script to use v1 instead of v1alpha1 - Generated all CRD manifests, deepcopy, and swagger docs Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (2)
📝 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 | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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
| // 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. | ||
| // If Installing is true, that means that a new release bundle is currently being copied on one (or more) cluster nodes, and not yet completed. | ||
| // If Available is true, it means that the release has been previously installed on all the cluster nodes, and it can be used. | ||
| // If Removing is true, it means that a release deletion is in progress on one (or more) cluster nodes, and not yet completed. | ||
| // If Degraded is true, that means something has gone wrong (possibly on one or more cluster nodes). | ||
| // | ||
| // In general, after installing a new release bundle, it is required to wait for the Conditions "Available" to become "True" (and all | ||
| // the other conditions to be equal to "False") before being able to pull its content. | ||
| // | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=5 | ||
| // +optional | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
Document MinItems and MaxItems constraints for conditions field.
The conditions field has +kubebuilder:validation:MinItems=1 and +kubebuilder:validation:MaxItems=5 markers that are not documented in the field comment. Per API review requirements, all validation markers must be documented.
Additionally, since the field is +optional, clarify that when present, it must contain between 1 and 5 items, but can be omitted entirely when the bundle has no status yet.
📝 Suggested documentation fix
// 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.
// If Installing is true, that means that a new release bundle is currently being copied on one (or more) cluster nodes, and not yet completed.
// If Available is true, it means that the release has been previously installed on all the cluster nodes, and it can be used.
// If Removing is true, it means that a release deletion is in progress on one (or more) cluster nodes, and not yet completed.
// If Degraded is true, that means something has gone wrong (possibly on one or more cluster nodes).
//
// In general, after installing a new release bundle, it is required to wait for the Conditions "Available" to become "True" (and all
// the other conditions to be equal to "False") before being able to pull its content.
+// When present, conditions must contain at least 1 entry and must not exceed 5 entries.
//
// +listType=map📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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. | |
| // If Installing is true, that means that a new release bundle is currently being copied on one (or more) cluster nodes, and not yet completed. | |
| // If Available is true, it means that the release has been previously installed on all the cluster nodes, and it can be used. | |
| // If Removing is true, it means that a release deletion is in progress on one (or more) cluster nodes, and not yet completed. | |
| // If Degraded is true, that means something has gone wrong (possibly on one or more cluster nodes). | |
| // | |
| // In general, after installing a new release bundle, it is required to wait for the Conditions "Available" to become "True" (and all | |
| // the other conditions to be equal to "False") before being able to pull its content. | |
| // | |
| // +listType=map | |
| // +listMapKey=type | |
| // +kubebuilder:validation:MinItems=1 | |
| // +kubebuilder:validation:MaxItems=5 | |
| // +optional | |
| Conditions []metav1.Condition `json:"conditions,omitempty"` | |
| // 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. | |
| // If Installing is true, that means that a new release bundle is currently being copied on one (or more) cluster nodes, and not yet completed. | |
| // If Available is true, it means that the release has been previously installed on all the cluster nodes, and it can be used. | |
| // If Removing is true, it means that a release deletion is in progress on one (or more) cluster nodes, and not yet completed. | |
| // If Degraded is true, that means something has gone wrong (possibly on one or more cluster nodes). | |
| // | |
| // In general, after installing a new release bundle, it is required to wait for the Conditions "Available" to become "True" (and all | |
| // the other conditions to be equal to "False") before being able to pull its content. | |
| // When present, conditions must contain at least 1 entry and must not exceed 5 entries. | |
| // | |
| // +listType=map | |
| // +listMapKey=type | |
| // +kubebuilder:validation:MinItems=1 | |
| // +kubebuilder:validation:MaxItems=5 | |
| // +optional | |
| Conditions []metav1.Condition `json:"conditions,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 `@machineconfiguration/v1/types_internalreleaseimage.go` around lines 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.
Source: Coding guidelines
There was a problem hiding this comment.
Let's quickly fix this so it's explicit in the godoc
There was a problem hiding this comment.
@yuqi-zhang, agreed — here's the fix to make the constraints explicit in the godoc.
- Add a sentence documenting the
MinItems=1/MaxItems=5constraint to theConditionsfield comment inInternalReleaseImageBundleStatus
Failed to handle agent chat message. Please try again.
|
/assign @yuqi-zhang |
- Add singleton documentation - Remove omitempty,omitzero from spec - Remove omitempty,omitzero from status - Add conditions list size documentation - Change MaxItems from 20 to 5 - Add struct godoc - Fix grammar: 'will be' changed to 'has been'
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andfasano The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/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
|
@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. |
|
@pawanpinjarkar: 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. |
@yuqi-zhang will the payload testing include the specific (techpreview) agent jobs? Those are the only jobs that are effectively using the new API and that could validate it Update: trying this to see if it works with our TP jobs |
6 participants
Original PR from @sadasu #2863