OKD-419: Add missing variant check for SCOS - #6371
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThe OS release package adds the ChangesCentOS Stream CoreOS detection
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Hi @SupremeMortal. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
Pull request overview
Updates host OS detection to treat OKD/SCOS nodes that report ID=centos with VARIANT_ID=coreos as an Enterprise Linux CoreOS variant, so extension-handling logic (e.g., rpm-ostree extensions) applies correctly on newer OKD releases.
Changes:
- Add
centosto the set of recognized OS IDs. - Extend
OperatingSystem.IsEL()to return true forID=centoswhenVARIANT_ID=coreos.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/daemon/osrelease/osrelease.go (1)
121-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression case for CentOS Stream CoreOS.
Add a table-driven case in
pkg/daemon/osrelease/osrelease_test.gowithid=centos,variantID=coreos, andIsEL: true. This locks in the new classification.🤖 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/daemon/osrelease/osrelease.go` at line 121, Add a table-driven regression case in the osrelease tests for a CentOS Stream CoreOS release, using id=centos and variantID=coreos and asserting IsEL is true. Place it alongside the existing classification cases and preserve the current test structure.
🤖 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/daemon/osrelease/osrelease.go`:
- Line 121: Add a table-driven regression case in the osrelease tests for a
CentOS Stream CoreOS release, using id=centos and variantID=coreos and asserting
IsEL is true. Place it alongside the existing classification cases and preserve
the current test structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b53b19a4-93a5-4e11-9e72-4fbd4aef3b43
📒 Files selected for processing (1)
pkg/daemon/osrelease/osrelease.go
2de8163 to
bf247d0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Newer OKD releases use the "centos" ID with the "coreos" variant, similar to RHEL. This resolves issues with extensions not applying when an operator contains them.
bf247d0 to
0ffff0b
Compare
|
/ok-to-test |
|
/retest-required |
isabella-janssen
left a comment
There was a problem hiding this comment.
/lgtm
This seems like a fair change
|
Scheduling tests matching the |
|
/retitle OKD-419: Add missing variant check for SCOS |
|
@SupremeMortal: This pull request references OKD-419 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen, Prashanth684, SupremeMortal 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 |
|
/verified by TestOSRelease, @SupremeMortal The unit test verifies the change and the author provided, they tested themselves: https://kubernetes.slack.com/archives/C06FF6342RG/p1786113645486919?thread_ts=1785935745.819979&cid=C06FF6342RG |
|
@TwoDCube: 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. |
|
@SupremeMortal: 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. |
Newer OKD releases use the
centosID with thecoreosvariant, similar to RHEL. This resolves issues with extensions not applying when an operator MachineConfig contains them.Summary by CodeRabbit