MCO-2296: Promote OSImageStreams to v1#2854
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
Hello @pablintino! Some important instructions when contributing to openshift/api: |
|
@pablintino: This pull request references MCO-2296 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. |
|
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 (5)
📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR promotes the OSImageStream API from unstable v1alpha1 to stable v1. The Go type definitions move to the v1 package with updated OpenShift compatibility metadata changing from level 4 (no guarantees) to level 1 (stable within a major release). The v1 types are registered to the scheme, the CRD manifest is updated to expose v1, all test manifests are changed to use the v1 apiVersion consistently, and the build script is cleaned up to remove the osimagestreams CRD glob pattern. 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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/L
|
[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 |
|
@pablintino Rather than moving the files, I need you to create fresh copies of it all. We need a copy in both the v1 and v1alpha1 locations at the same time to merge, and then we can remove the v1alpha1 copies later. Otherwise it will cause issues for those trying to update code already built against v1alpha1 |
pablintino
force-pushed
the
osimagestream-v1
branch
from
0159ec3 to
fe3cc54
Compare
openshift-ci
Bot
added
size/XXL
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Thanks for the explanation @JoelSpeed, appreciated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
payload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml (1)
9-9: 💤 Low valueConsider whether TechPreviewNoUpgrade is appropriate for a v1 API.
The CRD is marked with
release.openshift.io/feature-set: CustomNoUpgrade,DevPreviewNoUpgrade,TechPreviewNoUpgrade, which restricts it to tech preview and custom feature sets. Typically, v1 APIs are considered GA (generally available) and should be in the Default feature set. This might be intentional if you're promoting the API structure to v1 while keeping the feature gated until full GA readiness, but worth confirming this is the intended approach.🤖 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 `@payload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml` at line 9, The CRD for osimagestreams is labeled with release.openshift.io/feature-set: CustomNoUpgrade,DevPreviewNoUpgrade,TechPreviewNoUpgrade which gates the v1 API behind tech-preview; decide whether this v1 API should be GA and, if so, change the feature-set to include the Default feature set (e.g., replace the value with Default or remove TechPreviewNoUpgrade/DevPreviewNoUpgrade entries), otherwise keep the current gating—update the release.openshift.io/feature-set annotation accordingly on the osimagestreams CRD definition to reflect the intended release maturity.
🤖 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_osimagestream.go`:
- Around line 30-33: The comment for the metav1.ObjectMeta field `metadata` is
missing the omission behavior; update the field doc in types_osimagestream.go to
state what happens when callers omit `metadata` (for example: which values are
defaulted/filled by the API server — e.g., name/namespace may be generated or
validated, labels/annotations are empty unless provided, and other optional
metadata fields remain unset) so that the `+optional` annotation for `metadata`
and the type metav1.ObjectMeta clearly document the server-side defaults and
caller expectations.
In `@payload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml`:
- Around line 21-23: Re-add the removed v1alpha1 entry under spec.versions
alongside the new v1 entry so the CRD preserves the old API; ensure
spec.versions contains two items named "v1" and "v1alpha1", set v1 to served:
true and storage: true, set v1alpha1 to served: true and storage: false, and
copy the existing schema block from v1 into the v1alpha1 entry (or keep the
previous v1alpha1 schema) so consumers of the v1alpha1 API continue to work
during the transition.
---
Nitpick comments:
In `@payload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml`:
- Line 9: The CRD for osimagestreams is labeled with
release.openshift.io/feature-set:
CustomNoUpgrade,DevPreviewNoUpgrade,TechPreviewNoUpgrade which gates the v1 API
behind tech-preview; decide whether this v1 API should be GA and, if so, change
the feature-set to include the Default feature set (e.g., replace the value with
Default or remove TechPreviewNoUpgrade/DevPreviewNoUpgrade entries), otherwise
keep the current gating—update the release.openshift.io/feature-set annotation
accordingly on the osimagestreams CRD definition to reflect the intended release
maturity.
🪄 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: 29ce1b49-0a21-4449-bad0-02efbfec0578
⛔ Files ignored due to path filters (5)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams.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/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
hack/update-payload-crds.shmachineconfiguration/v1/register.gomachineconfiguration/v1/tests/osimagestreams.machineconfiguration.openshift.io/OSStreams.yamlmachineconfiguration/v1/types_osimagestream.gopayload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml
💤 Files with no reviewable changes (1)
- hack/update-payload-crds.sh
| // metadata is the standard object's metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` |
There was a problem hiding this comment.
Document the omission behavior for metadata.
Line 32 marks metadata as optional, but the comment does not say what happens when callers omit it. For a new v1 API, that omission behavior should be part of the field docs.
As per coding guidelines, "Documentation for +optional fields must explain the behavior when the field is omitted".
🤖 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_osimagestream.go` around lines 30 - 33, The
comment for the metav1.ObjectMeta field `metadata` is missing the omission
behavior; update the field doc in types_osimagestream.go to state what happens
when callers omit `metadata` (for example: which values are defaulted/filled by
the API server — e.g., name/namespace may be generated or validated,
labels/annotations are empty unless provided, and other optional metadata fields
remain unset) so that the `+optional` annotation for `metadata` and the type
metav1.ObjectMeta clearly document the server-side defaults and caller
expectations.
| versions: | ||
| - name: v1alpha1 | ||
| - name: v1 | ||
| schema: |
There was a problem hiding this comment.
Breaking change: v1alpha1 version completely removed instead of preserved alongside v1.
The CRD now serves only v1, with no v1alpha1 entry in spec.versions. This breaks existing consumers using v1alpha1. Standard Kubernetes API versioning practice is to preserve the old version when promoting:
spec:
versions:
- name: v1
served: true
storage: true
schema: ...
- name: v1alpha1
served: true
storage: false
schema: ...Then deprecate and remove v1alpha1 in a later release after a transition period. This aligns with the reviewer's feedback to "maintain copies in v1alpha1 and add v1 copies now, then remove v1alpha1 later."
🤖 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 `@payload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml`
around lines 21 - 23, Re-add the removed v1alpha1 entry under spec.versions
alongside the new v1 entry so the CRD preserves the old API; ensure
spec.versions contains two items named "v1" and "v1alpha1", set v1 to served:
true and storage: true, set v1alpha1 to served: true and storage: false, and
copy the existing schema block from v1 into the v1alpha1 entry (or keep the
previous v1alpha1 schema) so consumers of the v1alpha1 API continue to work
during the transition.
There was a problem hiding this comment.
This should just be defining what gets shipped in the payload. The alpha API is still available but not directly used in clusters after this change. This shouldn't be a problem assuming we switch over all references (MCO, tests, etc.) altogether
There was a problem hiding this comment.
@yuqi-zhang, thanks for the clarification! That context makes sense — since this is the payload CRD definition and the intent is an atomic switchover of all consumers (MCO, tests, etc.) to v1 simultaneously, preserving v1alpha1 in the payload is unnecessary.
The critical dependency here is that the switchover is truly complete before this lands — any consumer still referencing v1alpha1 in the cluster would break. It may be worth ensuring the PR description or linked Jira explicitly tracks the full list of required reference updates (MCO, tests, and any other tooling) as a checklist to guard against partial switchover.
I'll go ahead and resolve my comment since the concern doesn't apply given this deployment model. Would you like me to do that?
✏️ Learnings added
Learnt from: yuqi-zhang
Repo: openshift/api PR: 2854
File: payload-manifests/crds/0000_80_machine-config_01_osimagestreams.crd.yaml:21-23
Timestamp: 2026-05-21T14:39:01.495Z
Learning: In OpenShift's `payload-manifests/crds/` directory, a CRD version entry can be fully replaced (e.g., v1alpha1 → v1) without keeping the old version alongside the new one, provided all consumers (MCO, tests, tooling) are switched over atomically in the same release. This differs from the standard Kubernetes multi-version deprecation path. The key condition is that no in-cluster consumers reference the removed version after the change lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
pablintino
force-pushed
the
osimagestream-v1
branch
from
fe3cc54 to
15eb0fc
Compare
|
|
||
| // spec contains the desired OSImageStream config configuration. | ||
| // +required | ||
| Spec *OSImageStreamSpec `json:"spec,omitempty"` |
There was a problem hiding this comment.
You shouldn't need a pointer here 🤔
| Spec *OSImageStreamSpec `json:"spec,omitempty"` | |
| Spec OSImageStreamSpec `json:"spec,omitzero"` |
There was a problem hiding this comment.
True, is no longer needed. Fixed.
This change moves OSImageStreams from v1alpha to v1 apiVersion as part of the GA process of the API. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
pablintino
force-pushed
the
osimagestream-v1
branch
from
15eb0fc to
d966b3d
Compare
|
@pablintino: 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. |
4 participants
This change moves OSImageStreams from v1alpha to v1 apiVersion as part of the GA process of the API.