AGENT-1522: Graduate InternalReleaseImage from v1alpha1 to v1#2880
AGENT-1522: Graduate InternalReleaseImage from v1alpha1 to v1#2880pawanpinjarkar wants to merge 1 commit 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 <[email protected]>
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 |
📝 WalkthroughWalkthroughThis pull request introduces the 🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
Bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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
| // InternalReleaseImage is used to keep track and manage a set | ||
| // of release bundles (OCP and OLM operators images) that are stored | ||
| // into the control planes nodes. | ||
| // | ||
| // Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer). | ||
| // +openshift:compatibility-gen:level=1 |
There was a problem hiding this comment.
Document the singleton constraint in the type description.
The type has a validation rule enforcing metadata.name == 'cluster', but this singleton nature is not documented in the type's description comment. Users reading the API documentation need to know this is a cluster-scoped singleton resource.
📝 Suggested documentation addition
// InternalReleaseImage is used to keep track and manage a set
// of release bundles (OCP and OLM operators images) that are stored
-// into the control planes nodes.
+// into the control plane nodes.
+// This is a singleton resource; the metadata.name must be 'cluster'.
//
// Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).📝 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.
| // InternalReleaseImage is used to keep track and manage a set | |
| // of release bundles (OCP and OLM operators images) that are stored | |
| // into the control planes nodes. | |
| // | |
| // Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer). | |
| // +openshift:compatibility-gen:level=1 | |
| // InternalReleaseImage is used to keep track and manage a set | |
| // of release bundles (OCP and OLM operators images) that are stored | |
| // into the control plane nodes. | |
| // This is a singleton resource; the metadata.name must be 'cluster'. | |
| // | |
| // Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer). | |
| // +openshift:compatibility-gen:level=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 `@machineconfiguration/v1/types_internalreleaseimage.go` around lines 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.
Source: Coding guidelines
| // conditions represent the observations of the InternalReleaseImage controller current state. | ||
| // Valid types are: Degraded. | ||
| // If Degraded is true, that means something has gone wrong in the controller. | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=20 | ||
| // +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=20 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, the comment should clarify that when the field is present, it must contain between 1 and 20 items, but the field can be omitted entirely.
📝 Suggested documentation fix
// conditions represent the observations of the InternalReleaseImage controller current state.
// Valid types are: Degraded.
// If Degraded is true, that means something has gone wrong in the controller.
+// When present, conditions must contain at least 1 entry and must not exceed 20 entries.
// +listType=map🤖 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 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.
Source: Coding guidelines
| // 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
|
@pawanpinjarkar: 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. |
3 participants
Original PR from @sadasu #2863