MCO-2181: Set OSImageStream default in install-config#10570
Conversation
|
@zaneb: This pull request references MCO-2181 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. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughDefaults for OSImageStream were centralized into InstallConfig defaulting; consumer fallbacks and helpers were removed. Downstream validation, RHCOS stream logic, manifest/bootstrap wiring, baremetal label wiring, utility helpers, and tests were updated to read OSImageStream directly or assert the default. ChangesOSImageStream Default Field Enforcement
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
The "invalid OSImageStream set" test case sets types.SCOS = true during test table initialization (inside an immediately-invoked closure), but the restore function only runs during test execution of that specific case. This means types.SCOS remains true when all the test cases before it are executed. Move the types.SCOS = true assignment into restoreFnFactory, which runs during test execution, so it doesn't leak into other test cases. Assisted-by: Claude Code
zaneb
force-pushed
the
osimagestream-default
branch
from
36e8671 to
f75be8b
Compare
|
I would suggest if we're happy with this approach then we merge this PR and rebase #10481 on top of it (which will allow some extraneous non-ABI-related changes in it to be dropped). |
Set the default value for OSImageStream centrally in SetInstallConfigDefaults() rather than checking for an empty string at every call site that reads it. This eliminates scattered fallback logic and wrapper functions that existed solely to provide the default. Update the feature gate condition for FeatureGateOSStreams to check against DefaultOSImageStream instead of the empty string, since the field now always has a value after defaults are applied. Assisted-by: Claude Code
zaneb
force-pushed
the
osimagestream-default
branch
from
f75be8b to
a58c93f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/types/validation/installconfig_test.go (1)
3088-3088: ⚡ Quick winMake the supported-values assertion build-tag resilient.
This expected error hardcodes
"rhel-9", "rhel-10", which can drift when SCOS/RHCOS defaults differ by build tag. Build the regex from current constants (or assert key substrings) to keep this test stable across variants.🤖 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/types/validation/installconfig_test.go` at line 3088, The test's expectedError string hardcodes supported OS values and should be made build-tag resilient: change the assertion that currently uses expectedError (in the test case within pkg/types/validation/installconfig_test.go) to construct the expected message from the package constants or to match key substrings/regex instead of hardcoding "rhel-9", "rhel-10"; locate the test case where expectedError is defined and either build the supported-values portion dynamically from the current supportedOS constants (e.g., the constants that list supported images/versions used by the validation code) or assert with a regex/Contains on "Unsupported value" + the invalid value and "supported values" so the test remains stable across build-tag variants.
🤖 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.
Nitpick comments:
In `@pkg/types/validation/installconfig_test.go`:
- Line 3088: The test's expectedError string hardcodes supported OS values and
should be made build-tag resilient: change the assertion that currently uses
expectedError (in the test case within
pkg/types/validation/installconfig_test.go) to construct the expected message
from the package constants or to match key substrings/regex instead of
hardcoding "rhel-9", "rhel-10"; locate the test case where expectedError is
defined and either build the supported-values portion dynamically from the
current supportedOS constants (e.g., the constants that list supported
images/versions used by the validation code) or assert with a regex/Contains on
"Unsupported value" + the invalid value and "supported values" so the test
remains stable across build-tag variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee204111-ef64-465d-8f0c-fc60cc454c5e
📒 Files selected for processing (15)
pkg/asset/agent/installconfig_test.gopkg/asset/ignition/bootstrap/common.gopkg/asset/installconfig/installconfig_test.gopkg/asset/machines/baremetal/hosts.gopkg/asset/machines/baremetal/hosts_test.gopkg/asset/machines/baremetal/machines.gopkg/asset/manifests/osimagestream.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.gopkg/types/defaults/installconfig.gopkg/types/defaults/installconfig_test.gopkg/types/validation/featuregates.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.gopkg/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- pkg/types/defaults/installconfig_test.go
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
I would suggest if we're happy with this approach then we merge this PR and rebase #10481 on top of it (which will allow some extraneous non-ABI-related changes in it to be dropped).
yea, sgtm and it will also help with #10533 (comment) :D
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tthvo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test e2e-aws-ovn-rhcos10-techpreview |
|
@zaneb: 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. |
|
/verified by https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_installer/10570/pull-ci-openshift-installer-main-e2e-aws-ovn-rhcos10-techpreview/2059236136831684608 |
openshift-ci-robot
added
the
verified
|
@pablintino: This PR has been marked as verified 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. |
openshift-merge-bot
Bot
merged commit e7cb376
into
openshift:main
This change enables support for RHEL 10 (coreos10-) and RHEL 9 (coreos-) file path differentiation in machine-os-images container and throughout the agent installer workflow. Changes: - Add OSImageStream field to installConfigOverrides in AgentClusterInstall - Add stream-aware helper functions for file paths in releaseextract.go - Update ReleasePayload interface and BaseIso to accept osImageStream parameter - Update GetMetalArtifact to use osImageStream for FetchCoreOSBuild calls - Add GetOSImageStream() method to AgentManifests following existing patterns - Update getHashFromInstaller() to accept and use osImageStream parameter - Fix test to use explicit OSImageStream value instead of empty string - Rely on SetInstallConfigDefaults() to provide default osImageStream value This implements the foundation for RHEL 10 support in the agent installer by allowing the installer to distinguish between RHEL 9 and RHEL 10 boot artifacts based on the osImageStream annotation. Depends on openshift#10570 which sets the default OSImageStream centrally.
This change enables support for RHEL 10 (coreos10-) and RHEL 9 (coreos-) file path differentiation in machine-os-images container and throughout the agent installer workflow. Changes: - Add OSImageStream field to installConfigOverrides in AgentClusterInstall - Add stream-aware helper functions for file paths in releaseextract.go - Update ReleasePayload interface and BaseIso to accept osImageStream parameter - Update GetMetalArtifact to use osImageStream for FetchCoreOSBuild calls - Add GetOSImageStream() method to AgentManifests following existing patterns - Update getHashFromInstaller() to accept and use osImageStream parameter - Fix test to use explicit OSImageStream value instead of empty string - Rely on SetInstallConfigDefaults() to provide default osImageStream value This implements the foundation for RHEL 10 support in the agent installer by allowing the installer to distinguish between RHEL 9 and RHEL 10 boot artifacts based on the osImageStream annotation. Depends on openshift#10570 which sets the default OSImageStream centrally.
This change enables support for RHEL 10 (coreos10-) and RHEL 9 (coreos-) file path differentiation in machine-os-images container and throughout the agent installer workflow. Changes: - Add OSImageStream field to installConfigOverrides in AgentClusterInstall - Add stream-aware helper functions for file paths in releaseextract.go - Update ReleasePayload interface and BaseIso to accept osImageStream parameter - Update GetMetalArtifact to use osImageStream for FetchCoreOSBuild calls - Add GetOSImageStream() method to AgentManifests following existing patterns - Update getHashFromInstaller() to accept and use osImageStream parameter - Fix test to use explicit OSImageStream value instead of empty string - Rely on SetInstallConfigDefaults() to provide default osImageStream value This implements the foundation for RHEL 10 support in the agent installer by allowing the installer to distinguish between RHEL 9 and RHEL 10 boot artifacts based on the osImageStream annotation. Depends on openshift#10570 which sets the default OSImageStream centrally.
4 participants
Set the default value for OSImageStream centrally in
SetInstallConfigDefaults() rather than checking for an empty string at
every call site that reads it. This eliminates scattered fallback logic
and wrapper functions that existed solely to provide the default.
Update the feature gate condition for FeatureGateOSStreams to check
against DefaultOSImageStream instead of the empty string, since the
field now always has a value after defaults are applied.
Summary by CodeRabbit
Refactor
Validation