MCO-2116: Generate OSImageStream manifest#10357
MCO-2116: Generate OSImageStream manifest#10357pablintino wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-2116 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 "4.22.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
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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 |
WalkthroughAdds a new OSImageStream writable asset and integrates it into OpenShift manifest generation; introduces DefaultOSImageStream constants for RHCOS and SCOS. The asset emits an OSImageStream CR when the OS Streams feature gate is enabled and the target is not SCOS. Changes
Sequence Diagram(s)sequenceDiagram
participant Generator
participant Deps as dependencies.Get
participant InstallConfig
participant OSIS as OSImageStream
participant Manifests
participant FS as FileWriter
Generator->>Manifests: Generate()
Manifests->>Deps: Request dependencies (InstallConfig, OSImageStream)
Deps->>InstallConfig: Provide InstallConfig
Deps->>OSIS: Provide OSImageStream instance
OSIS->>InstallConfig: Read feature gates, target, stream
alt feature gate enabled and not SCOS
OSIS->>OSIS: Build mcfgv1alpha.OSImageStream
OSIS->>OSIS: Marshal to YAML (99_osimagestream.yaml)
OSIS-->>Manifests: Return Files()
else
OSIS-->>Manifests: Return no files
end
Manifests->>FS: Append files and write outputs
FS-->>Generator: Files written
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@pablintino: This pull request references MCO-2116 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 "4.22.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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/manifests/osimagestream.go`:
- Around line 44-63: The OSImageStream CR is being created even when no stream
was chosen; update the generation logic that currently checks
features.FeatureGateOSStreams and installConfig.Config.IsSCOS to also skip when
installConfig.Config.OSImageStream is empty (or only whitespace). Specifically,
before constructing the mcfgv1alpha.OSImageStream (the osImageStream variable
and its Spec.DefaultStream), add a guard that returns nil if
installConfig.Config.OSImageStream is blank so no 99_osimagestream.yaml is
emitted with an empty spec.defaultStream.
In `@pkg/rhcos/stream_scos.go`:
- Around line 5-6: Add the missing import that provides the OSImageStream type:
import the package that defines OSImageStream and alias it as types (so the file
can reference types.OSImageStream) before declaring the DefaultOSImageStream
constant; update the import block in pkg/rhcos/stream_scos.go accordingly so
DefaultOSImageStream and types.OSImageStream resolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27091357-7587-41b5-bb98-c07bb88b1dff
📒 Files selected for processing (4)
pkg/asset/manifests/openshift.gopkg/asset/manifests/osimagestream.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.go
| // If one of the following are true the OSImageStream CR is not generated | ||
| // 1. The feature is not enabled | ||
| // 2. The target is OKD | ||
| if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() { | ||
| // FG disabled or not OCP | ||
| return nil | ||
| } | ||
|
|
||
| osImageStream := &mcfgv1alpha.OSImageStream{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: mcfgv1alpha.SchemeGroupVersion.String(), | ||
| Kind: "OSImageStream", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "cluster", | ||
| }, | ||
| Spec: &mcfgv1alpha.OSImageStreamSpec{ | ||
| DefaultStream: string(installConfig.Config.OSImageStream), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Skip OSImageStream generation when no stream was selected.
This currently emits 99_osimagestream.yaml whenever the feature gate is on, even if installConfig.Config.OSImageStream is empty. That produces a CR with an empty spec.defaultStream, which does not match the PR objective of generating the manifest only when a stream was chosen.
Suggested fix
- if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
+ if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) ||
+ installConfig.Config.IsSCOS() ||
+ installConfig.Config.OSImageStream == "" {
// FG disabled or not OCP
return nil
}
@@
Spec: &mcfgv1alpha.OSImageStreamSpec{
- DefaultStream: string(installConfig.Config.OSImageStream),
+ DefaultStream: installConfig.Config.OSImageStream,
},
}📝 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.
| // If one of the following are true the OSImageStream CR is not generated | |
| // 1. The feature is not enabled | |
| // 2. The target is OKD | |
| if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() { | |
| // FG disabled or not OCP | |
| return nil | |
| } | |
| osImageStream := &mcfgv1alpha.OSImageStream{ | |
| TypeMeta: metav1.TypeMeta{ | |
| APIVersion: mcfgv1alpha.SchemeGroupVersion.String(), | |
| Kind: "OSImageStream", | |
| }, | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "cluster", | |
| }, | |
| Spec: &mcfgv1alpha.OSImageStreamSpec{ | |
| DefaultStream: string(installConfig.Config.OSImageStream), | |
| }, | |
| } | |
| // If one of the following are true the OSImageStream CR is not generated | |
| // 1. The feature is not enabled | |
| // 2. The target is OKD | |
| if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || | |
| installConfig.Config.IsSCOS() || | |
| installConfig.Config.OSImageStream == "" { | |
| // FG disabled or not OCP | |
| return nil | |
| } | |
| osImageStream := &mcfgv1alpha.OSImageStream{ | |
| TypeMeta: metav1.TypeMeta{ | |
| APIVersion: mcfgv1alpha.SchemeGroupVersion.String(), | |
| Kind: "OSImageStream", | |
| }, | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "cluster", | |
| }, | |
| Spec: &mcfgv1alpha.OSImageStreamSpec{ | |
| DefaultStream: installConfig.Config.OSImageStream, | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/manifests/osimagestream.go` around lines 44 - 63, The OSImageStream
CR is being created even when no stream was chosen; update the generation logic
that currently checks features.FeatureGateOSStreams and
installConfig.Config.IsSCOS to also skip when installConfig.Config.OSImageStream
is empty (or only whitespace). Specifically, before constructing the
mcfgv1alpha.OSImageStream (the osImageStream variable and its
Spec.DefaultStream), add a guard that returns nil if
installConfig.Config.OSImageStream is blank so no 99_osimagestream.yaml is
emitted with an empty spec.defaultStream.
|
@pablintino: This pull request references MCO-2116 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/asset/manifests/osimagestream.go (1)
3-16: Consider grouping imports by origin.Standard library imports (
context,path) are mixed with external and internal packages. Grouping them separately improves readability.♻️ Suggested organization
import ( "context" - "github.com/openshift/installer/pkg/rhcos" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "path" "github.com/openshift/api/features" "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" mcfgv1alpha "github.com/openshift/api/machineconfiguration/v1alpha1" "github.com/openshift/installer/pkg/asset" "github.com/openshift/installer/pkg/asset/installconfig" + "github.com/openshift/installer/pkg/rhcos" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/osimagestream.go` around lines 3 - 16, Reorder the import block in osimagestream.go to group standard library packages together, third-party packages together, and internal repository packages together: place "context" and "path" in the first (standard) group, keep k8s and external libs (metav1, "github.com/openshift/api/features", "github.com/pkg/errors", "sigs.k8s.io/yaml") in the second group, and the project-specific imports ("github.com/openshift/installer/pkg/rhcos", mcfgv1alpha, "github.com/openshift/installer/pkg/asset", "github.com/openshift/installer/pkg/asset/installconfig") in the final group so the import block is clearly separated by origin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/manifests/osimagestream.go`:
- Around line 45-48: Update the misleading comment above the OSImageStream
generation check so it refers to IsSCOS() rather than "OKD": clarify that the
OSImageStream CR is not generated when the feature gate
features.FeatureGateOSStreams is disabled or when installConfig.Config.IsSCOS()
(SCOS/OKD target) is true; keep the logic using
installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams)
and installConfig.Config.IsSCOS() unchanged, only change the comment text to
match the IsSCOS() method name (or explicitly mention both SCOS and OKD for
clarity).
- Around line 54-68: The computed fallback value `stream` is never used when
building the OSImageStream manifest, so DefaultStream is set from
installConfig.Config.OSImageStream (which may be empty); replace the direct use
of installConfig.Config.OSImageStream with the previously computed `stream`
variable when populating the OSImageStream.Spec.DefaultStream so the
rhcos.DefaultOSImageStream fallback is applied; update the construction of the
OSImageStream (OSImageStream, OSImageStreamSpec, DefaultStream) to reference
`stream` instead of installConfig.Config.OSImageStream.
---
Nitpick comments:
In `@pkg/asset/manifests/osimagestream.go`:
- Around line 3-16: Reorder the import block in osimagestream.go to group
standard library packages together, third-party packages together, and internal
repository packages together: place "context" and "path" in the first (standard)
group, keep k8s and external libs (metav1, "github.com/openshift/api/features",
"github.com/pkg/errors", "sigs.k8s.io/yaml") in the second group, and the
project-specific imports ("github.com/openshift/installer/pkg/rhcos",
mcfgv1alpha, "github.com/openshift/installer/pkg/asset",
"github.com/openshift/installer/pkg/asset/installconfig") in the final group so
the import block is clearly separated by origin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a9c47e8-7d7c-45ea-becb-e96948c5349f
📒 Files selected for processing (4)
pkg/asset/manifests/openshift.gopkg/asset/manifests/osimagestream.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.go
| // If one of the following are true the OSImageStream CR is not generated | ||
| // 1. The feature is not enabled | ||
| // 2. The target is OKD | ||
| if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() { |
There was a problem hiding this comment.
Comment mentions "OKD" but code checks IsSCOS().
The comment on line 47 says "The target is OKD" but the code calls IsSCOS(). While SCOS (CentOS Stream CoreOS) is used for OKD builds, the comment should align with the method name for clarity and maintainability.
📝 Suggested clarification
// If one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
- // 2. The target is OKD
+ // 2. The target is SCOS (used by OKD)
if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {📝 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.
| // If one of the following are true the OSImageStream CR is not generated | |
| // 1. The feature is not enabled | |
| // 2. The target is OKD | |
| if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() { | |
| // If one of the following are true the OSImageStream CR is not generated | |
| // 1. The feature is not enabled | |
| // 2. The target is SCOS (used by OKD) | |
| if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/manifests/osimagestream.go` around lines 45 - 48, Update the
misleading comment above the OSImageStream generation check so it refers to
IsSCOS() rather than "OKD": clarify that the OSImageStream CR is not generated
when the feature gate features.FeatureGateOSStreams is disabled or when
installConfig.Config.IsSCOS() (SCOS/OKD target) is true; keep the logic using
installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams)
and installConfig.Config.IsSCOS() unchanged, only change the comment text to
match the IsSCOS() method name (or explicitly mention both SCOS and OKD for
clarity).
| stream := installConfig.Config.OSImageStream | ||
| if stream == "" { | ||
| stream = rhcos.DefaultOSImageStream | ||
| } | ||
|
|
||
| osImageStream := &mcfgv1alpha.OSImageStream{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: mcfgv1alpha.SchemeGroupVersion.String(), | ||
| Kind: "OSImageStream", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "cluster", | ||
| }, | ||
| Spec: &mcfgv1alpha.OSImageStreamSpec{ | ||
| DefaultStream: string(installConfig.Config.OSImageStream), |
There was a problem hiding this comment.
Bug: Computed stream variable is unused; manifest will have empty DefaultStream when no stream is specified.
Lines 54-57 correctly compute stream with a fallback to rhcos.DefaultOSImageStream, but line 68 uses installConfig.Config.OSImageStream directly instead of the computed stream variable. This defeats the purpose of the default logic.
🐛 Proposed fix
Spec: &mcfgv1alpha.OSImageStreamSpec{
- DefaultStream: string(installConfig.Config.OSImageStream),
+ DefaultStream: string(stream),
},📝 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.
| stream := installConfig.Config.OSImageStream | |
| if stream == "" { | |
| stream = rhcos.DefaultOSImageStream | |
| } | |
| osImageStream := &mcfgv1alpha.OSImageStream{ | |
| TypeMeta: metav1.TypeMeta{ | |
| APIVersion: mcfgv1alpha.SchemeGroupVersion.String(), | |
| Kind: "OSImageStream", | |
| }, | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "cluster", | |
| }, | |
| Spec: &mcfgv1alpha.OSImageStreamSpec{ | |
| DefaultStream: string(installConfig.Config.OSImageStream), | |
| stream := installConfig.Config.OSImageStream | |
| if stream == "" { | |
| stream = rhcos.DefaultOSImageStream | |
| } | |
| osImageStream := &mcfgv1alpha.OSImageStream{ | |
| TypeMeta: metav1.TypeMeta{ | |
| APIVersion: mcfgv1alpha.SchemeGroupVersion.String(), | |
| Kind: "OSImageStream", | |
| }, | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "cluster", | |
| }, | |
| Spec: &mcfgv1alpha.OSImageStreamSpec{ | |
| DefaultStream: string(stream), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/manifests/osimagestream.go` around lines 54 - 68, The computed
fallback value `stream` is never used when building the OSImageStream manifest,
so DefaultStream is set from installConfig.Config.OSImageStream (which may be
empty); replace the direct use of installConfig.Config.OSImageStream with the
previously computed `stream` variable when populating the
OSImageStream.Spec.DefaultStream so the rhcos.DefaultOSImageStream fallback is
applied; update the construction of the OSImageStream (OSImageStream,
OSImageStreamSpec, DefaultStream) to reference `stream` instead of
installConfig.Config.OSImageStream.
When the OSStreams feature gate is enabled and generate the OSImageStream CR in the openshift manifests so the cluster can boot with the desired stream. If not stream was selected the default one is picked. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
@pablintino: This pull request references MCO-2116 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 "4.22.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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/asset/manifests/osimagestream.go (1)
53-68:⚠️ Potential issue | 🔴 CriticalUse the computed fallback when populating
spec.defaultStream.Line 68 bypasses the fallback calculated on Lines 54-57, so an unset
OSImageStreamstill emitsspec.defaultStream: ""instead of the intended default.Suggested fix
Spec: &mcfgv1alpha.OSImageStreamSpec{ - DefaultStream: string(installConfig.Config.OSImageStream), + DefaultStream: string(stream), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/osimagestream.go` around lines 53 - 68, The spec.defaultStream is being set from installConfig.Config.OSImageStream instead of the computed fallback stored in the local variable stream; update the OSImageStream creation (Spec.DefaultStream) to use the computed variable stream (convert to string if necessary) so the fallback (rhcos.DefaultOSImageStream) is emitted when the config value is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/asset/manifests/osimagestream.go`:
- Around line 53-68: The spec.defaultStream is being set from
installConfig.Config.OSImageStream instead of the computed fallback stored in
the local variable stream; update the OSImageStream creation
(Spec.DefaultStream) to use the computed variable stream (convert to string if
necessary) so the fallback (rhcos.DefaultOSImageStream) is emitted when the
config value is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ad01fd0-dc57-4b17-88bf-f3f00621e0e2
📒 Files selected for processing (5)
hack/build-coreos-manifest.gopkg/asset/manifests/openshift.gopkg/asset/manifests/osimagestream.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.go
2 participants
When the
OSStreamsfeature gate is enabled and the user has selected an OS image stream, generate theOSImageStreamCR in the openshift manifests so the cluster can boot with the desired stream.Summary by CodeRabbit
New Features
Bug Fixes