MCO-2257: make rhel-10 the default for new installs#5999
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughDefault RHEL CoreOS image tag changed to ChangesRHEL CoreOS tag + build/test rewrites
Test: MCP cleanup naming and shadow password-hash checks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi 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
cheesesashimi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@cheesesashimi: This pull request references MCO-2257 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
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-aws-ovn e2e-aws-ovn-upgrade |
|
/jira refresh |
|
@cheesesashimi: This pull request references MCO-2257 which is a valid 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. |
cheesesashimi
force-pushed
the
zzlotnik/rhel10-by-default
branch
from
7f8dd4f to
411e4be
Compare
|
/test e2e-aws-ovn e2e-aws-ovn-upgrade |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
33-35: 💤 Low valueThe else branch safely rewrites
osimageurl.yamltoday, but consider anchoring the sed to YAML keys for resilience against future edits.The sed
s/rhel-coreos/rhel-coreos-10/gis currently safe—the target file contains onlyrhel-coreosandrhel-coreos-extensions, with no pre-existingrhel-coreos-10substring. However, the pattern is non-idempotent: a future manual edit introducing a literalrhel-coreos-10in the file would cause it to rewrite torhel-coreos-10-10and silently break image resolution. Consider anchoring the rewrite to specific YAML keys for safety:♻️ Optional: anchor the rewrite to specific YAML keys for idempotency
- sed -i 's/rhel-coreos/rhel-coreos-10/g' /manifests/0000_80_machine-config_05_osimageurl.yaml; fi && \ + sed -i -e 's|^\(\s*baseOSContainerImage:\s*\)rhel-coreos$|\1rhel-coreos-10|' \ + -e 's|^\(\s*baseOSExtensionsContainerImage:\s*\)rhel-coreos-extensions$|\1rhel-coreos-10-extensions|' \ + /manifests/0000_80_machine-config_05_osimageurl.yaml; fi && \Note: The else branch now makes empty/unknown
TAGSvalues default torhel-coreos-10(previously untouched). This is intentional behavior — any out-of-band builds with non-fcos/scosTAGSwill see manifests rewritten accordingly.🤖 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 `@Dockerfile` around lines 33 - 35, The sed replacement in the else branch (the sed -i 's/rhel-coreos/rhel-coreos-10/g' invocation targeting /manifests/0000_80_machine-config_05_osimageurl.yaml) is not idempotent; change it to only rewrite the specific YAML key(s) that carry the OS image identifier (e.g., the osImageURL or image fields in that manifest) so that literal occurrences elsewhere are not mangled. Update the Dockerfile command to match and replace the value on the YAML key (anchor the pattern to the key name and its value) instead of a global s///, ensuring the rewrite is safe to run multiple times and will not produce rhel-coreos-10-10 if the file already contains rhel-coreos-10.
🤖 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 `@Dockerfile`:
- Around line 33-35: The sed replacement in the else branch (the sed -i
's/rhel-coreos/rhel-coreos-10/g' invocation targeting
/manifests/0000_80_machine-config_05_osimageurl.yaml) is not idempotent; change
it to only rewrite the specific YAML key(s) that carry the OS image identifier
(e.g., the osImageURL or image fields in that manifest) so that literal
occurrences elsewhere are not mangled. Update the Dockerfile command to match
and replace the value on the YAML key (anchor the pattern to the key name and
its value) instead of a global s///, ensuring the rewrite is safe to run
multiple times and will not produce rhel-coreos-10-10 if the file already
contains rhel-coreos-10.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5aee0aad-7c70-434c-bd88-069e411d76fb
📒 Files selected for processing (4)
DockerfileDockerfile.rhel7cmd/machine-config-operator/bootstrap.goinstall/image-references
|
/test e2e-gcp-op-part1 e2e-gcp-op-part2 e2e-gcp-op-single-node |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-ocl-shared/helpers.go`:
- Around line 875-877: The test now pulls quay.io/centos/centos:stream10 but
ConvertFilesFromContainerImageToBytesMap contains hardcoded repo rewrite logic
that forces repo names to "9-stream"; update the rewrite logic in
ConvertFilesFromContainerImageToBytesMap to derive the stream version from the
centosPullspec (or from the repo file contents) instead of always substituting
"9-stream" — specifically detect tags like "stream10" and rewrite to "10-stream"
(or preserve the original stream token) so repo entries produced for
centosPullspec match the pulled image; ensure the change touches the rewrite
branch that references "9-stream" and keeps tests using centosPullspec
unchanged.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e671115b-5ac5-450a-9bd0-7862f453d91d
📒 Files selected for processing (4)
test/e2e-ocl-shared/Containerfile.cowsaytest/e2e-ocl-shared/helpers.gotest/extended-priv/mco_ocb.gotest/helpers/utils.go
| centosPullspec := "quay.io/centos/centos:stream10" | ||
| yumReposContents := ConvertFilesFromContainerImageToBytesMap(t, centosPullspec, "/etc/yum.repos.d/") | ||
| rpmGpgContents := ConvertFilesFromContainerImageToBytesMap(t, centosPullspec, "/etc/pki/rpm-gpg/") |
There was a problem hiding this comment.
Stream10 pullspec now conflicts with hardcoded 9-stream rewrite logic
After switching to quay.io/centos/centos:stream10 (Line 875), the extracted repo content is still rewritten to 9-stream in ConvertFilesFromContainerImageToBytesMap. That mismatch can point builds at the wrong repos and break package resolution.
Suggested fix
func ConvertFilesFromContainerImageToBytesMap(t *testing.T, pullspec, containerFilepath string) map[string][]byte {
@@
- isCentosImage := strings.Contains(pullspec, "centos")
+ isCentosImage := strings.Contains(pullspec, "centos")
+ streamValue := "10-stream"
+ if strings.Contains(pullspec, "stream9") {
+ streamValue = "9-stream"
+ }
@@
- if isCentosImage {
- contents = bytes.ReplaceAll(contents, []byte("$stream"), []byte("9-stream"))
- }
-
- // Replace $stream with 9-stream in any of the Centos repo content we pulled.
+ if isCentosImage {
+ contents = bytes.ReplaceAll(contents, []byte("$stream"), []byte(streamValue))
+ }
+
+ // Replace $stream with the matching stream value in any of the CentOS repo content.
out[filepath.Base(path)] = contents🤖 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 `@test/e2e-ocl-shared/helpers.go` around lines 875 - 877, The test now pulls
quay.io/centos/centos:stream10 but ConvertFilesFromContainerImageToBytesMap
contains hardcoded repo rewrite logic that forces repo names to "9-stream";
update the rewrite logic in ConvertFilesFromContainerImageToBytesMap to derive
the stream version from the centosPullspec (or from the repo file contents)
instead of always substituting "9-stream" — specifically detect tags like
"stream10" and rewrite to "10-stream" (or preserve the original stream token) so
repo entries produced for centosPullspec match the pulled image; ensure the
change touches the rewrite branch that references "9-stream" and keeps tests
using centosPullspec unchanged.
|
/retest-required |
|
/test e2e-aws-ovn e2e-aws-ovn-upgrade e2e-gcp-op-part1 e2e-gcp-op-part2 e2e-gcp-op-single-node e2e-gcp-op-ocl-part1 e2e-gcp-op-ocl-part2 |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.22-arm64-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@cheesesashimi: 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/7b2a0d00-4e11-11f1-9785-b0e986132b42-0 |
|
@cheesesashimi: 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. |
Golang defines a built-in delete() function which is used to remove items from maps. These tests overwrite this built-in function by assigning a different function to it. Using a different name for our cleanup functions is preferred due to the potential for side-effects.
When usermod -P is used, the last password change field is changed. When the MachineConfig is rolled back, this value remains the same even though the actual password value has been changed back to its previous value. Consequently, the test should only use the password hash to determine whether it was successful or not. Assisted-By: Claude Opus 4.6
|
/test e2e-gcp-op-part1 |
1 similar comment
|
/test e2e-gcp-op-part1 |
|
/test unit |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@cheesesashimi: 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/62101d20-4e41-11f1-9aa1-f596df5ceec0-0 |
The -K flag signals that curl should read its arguments from a config file. However, no config file is provided. This comment also deduplicates the package name, making future changes easier.
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
/test e2e-gcp-op-part1 |
|
@cheesesashimi: 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/9ecc63d0-4edd-11f1-8511-92668b88bb23-0 |
2 participants
- What I did
This updates the MCO's Dockerfiles to set the RHEL 10 image as the default OS image for new installs.
- How to verify it
Bring up a new cluster with this PR and it should be running RHEL 10.
- Description for the changelog
Use RHEL 10 by default
Summary by CodeRabbit
Bug Fixes
Tests
Chores