NO-ISSUE: Render OSImageStream in OKD too - #10730
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request explicitly references no jira issue. 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. |
|
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesOSImageStream gating
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/manifests/osimagestream.go (1)
40-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression coverage for SCOS generation.
Because this changes the generation branch, add a unit test proving that an SCOS
InstallConfiggeneratesOSImageStreamwhenFeatureGateOSStreamsis enabled and does not generate it when disabled. This is required for Go code underpkg/.🤖 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 `@pkg/asset/manifests/osimagestream.go` around lines 40 - 48, Add unit-test coverage for OSImageStream.Generate using SCOS InstallConfig fixtures: assert an OSImageStream is generated when FeatureGateOSStreams is enabled and no resource is generated when it is disabled. Reuse the existing test helpers and generation assertions for pkg/asset manifests rather than changing Generate behavior.Source: Coding guidelines
🤖 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 `@pkg/asset/manifests/osimagestream.go`:
- Around line 44-48: Update the comment above the feature-gate check in the
OSImageStream generation flow to describe only that generation is skipped when
FeatureGateOSStreams is disabled. Remove the stale reference to non-OCP targets;
leave the condition and return behavior unchanged.
---
Outside diff comments:
In `@pkg/asset/manifests/osimagestream.go`:
- Around line 40-48: Add unit-test coverage for OSImageStream.Generate using
SCOS InstallConfig fixtures: assert an OSImageStream is generated when
FeatureGateOSStreams is enabled and no resource is generated when it is
disabled. Reuse the existing test helpers and generation assertions for
pkg/asset manifests rather than changing Generate behavior.
🪄 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: c487745d-72d0-408e-9cd2-200632b1ac25
📒 Files selected for processing (1)
pkg/asset/manifests/osimagestream.go
The MCO is already creating the OSImageStream for OKD as we have GAed the feature in OKD and OCP. If not passed by the installer the MCO itself creates the resource but it would be nice to generate it from the installer like we do for OCP to align the behavior with OCP. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
af66e8e to
9dd4c23
Compare
| Name: "cluster", | ||
| }, | ||
| Spec: mcfgv1.OSImageStreamSpec{ | ||
| DefaultStream: string(installConfig.Config.OSImageStream), |
There was a problem hiding this comment.
Looking at https://github.com/openshift/installer/blob/main/pkg/types/installconfig.go#L791 it appears that the OSImageStream that should be used for SCOS should be Centos10. Current code, is adding whatever is provided in https://github.com/openshift/installer/blob/main/data/data/install.openshift.io_installconfigs.yaml#L5062 which seems to allow only 2 values rhel-9 and rhel-10.
I am also checking the OSImageStream validation code to see if that is aligned too.
There was a problem hiding this comment.
https://github.com/openshift/installer/blob/main/pkg/types/validation/installconfig.go#L1820 is how the OSImageStreams is validated.
If OKD is using SCOS OSImageStreamCentos10, then should the enum here https://github.com/openshift/installer/blob/main/data/data/install.openshift.io_installconfigs.yaml#L5062 be augmented?
I understand this request is probably outside the scope of this PR, but would be great to keep everything consistent with the help of this PR.
There was a problem hiding this comment.
Looking at https://github.com/openshift/installer/blob/main/pkg/types/installconfig.go#L791 it appears that the OSImageStream that should be used for SCOS should be Centos10. Current code, is adding whatever is provided in https://github.com/openshift/installer/blob/main/data/data/install.openshift.io_installconfigs.yaml#L5062 which seems to allow only 2 values rhel-9 and rhel-10.
I am also checking the OSImageStream validation code to see if that is aligned too.
@sadasu IIRC that data file is not used or consumed by our validations or defaulting, thus setting the default value is done purely manually here. Do we need to conditionally render that file? Is this the first time we face a misalignment between OCP and OKD values for that file?
The validation is done here based on what types.OSImageStreamValues() returns, that should be centos-10 only for OKD.
About the job, I'll trigger a job that I found for the MCO changes we did. The job fails some assertions but was good enough to know if the installation and the MCO were fine.
There was a problem hiding this comment.
We are aligned on the defaults and validation. With OKD, the user should be allowed to set osImageStreams to centos-10 in the install-config (when not specified, code currently picks the right default as you mentioned) here: https://github.com/openshift/installer/blob/main/data/data/install.openshift.io_installconfigs.yaml#L5062
enum:
- rhel-9
- rhel-10
- centos-10
This install-config field was added as part of commit 1cbe68b where the enum only had values for OCD.
There was a problem hiding this comment.
Is this the first time we face a misalignment between OCP and OKD values for that file?
Yes, it appears so.
Also, during OKD installs, if the osImageStream value is not set, we are able to pick the right defaults. But, if the customer uses this CRD as reference and explicitly sets rhel-10 as the osImageStream, our validations would reject it. Not a real problem, but an inconvenience that the user would need to rectify with updating their install-config.
|
@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. |
|
/payload-job periodic-ci-openshift-release-main-okd-scos-5.0-e2e-aws-ovn |
|
@pablintino: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e51282f0-8c1a-11f1-85c2-518be798ce0c-0 |
3 participants
The MCO is already creating the OSImageStream for OKD as we have GAed the feature in OKD and OCP. If not passed by the installer the MCO itself creates the resource but it would be nice to generate it from the installer like we do for OCP to align the behavior with OCP.
Summary by CodeRabbit