MCO-2371: Add back "should match os version" test#31301
Open
pablintino wants to merge 1 commit into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-2371 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. |
|
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)
WalkthroughThe "should match os version" CI test in ChangesMCP stream-based OS validation
Sequence Diagram(s)sequenceDiagram
participant Test as should match os version
participant getNodeOSJobType
participant DetectMode as Cluster mode detect
participant MicroShift as validateMicroshiftNodeOS
participant HyperShift as validateHypershiftNodeOS
participant Standalone as validateStandaloneNodeOS
participant fetchMCPStreams
participant getInstallVersion
Test->>getNodeOSJobType: JOB_NAME substring match
getNodeOSJobType-->>Test: NodeOSJobType
Test->>DetectMode: inspect cluster annotations
DetectMode-->>Test: cluster mode
alt MicroShift
Test->>MicroShift: nodeOSJobType
MicroShift-->>Test: Fail (rhcos10/mixed) or Skip
else HyperShift
Test->>HyperShift: nodeOSJobType
HyperShift-->>Test: Fail (rhcos10/mixed) or Skip
else standalone
Test->>getInstallVersion: fetch ClusterVersion history
getInstallVersion-->>Test: base version (for upgrade jobs)
Test->>fetchMCPStreams: list MCPs + OSImageStream singleton
fetchMCPStreams-->>Test: map[MCPName]effectiveStream
Test->>Standalone: nodeOSJobType, mcp streams, base version
Standalone-->>Test: Pass or Fail
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pablintino 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 |
Contributor
Author
|
/payload 5.0 nightly blocking |
Contributor
|
@pablintino: trigger 12 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b5b1b290-689a-11f1-88f4-0052a5b6f9be-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/ci/job_names.go (1)
233-254: 💤 Low valueError message doesn't cover all non-RHEL9 cases.
The condition
nodeOSJobType != RHEL9also includesRHELMixed910, but the error message only mentions "RHCOS10". If a mixed-mode job runs on MicroShift/HyperShift, the failure message will be misleading.Suggested clarification
func validateMicroshiftNodeOS(nodeOSJobType NodeOSJobType, jobName string) { if nodeOSJobType != RHEL9 { - // TODO(muller): Assume we do not have RHCOS10 microshift jobs now. If someone adds a RHCOS10 job, this failure + // TODO(muller): Assume we do not have RHCOS10/mixed microshift jobs now. If someone adds such a job, this failure // should force them to figure out how to detect RHCOS10 in microshift and update this test. - e2e.Failf("TODO: job name %q indicates RHCOS10 which cannot be checked for MicroShift clusters now", jobName) + e2e.Failf("TODO: job name %q indicates RHCOS10 or mixed OS which cannot be checked for MicroShift clusters now", jobName) return } e2eskipper.Skip("Cannot check RHCOS for MicroShift clusters") } func validateHypershiftNodeOS(nodeOSJobType NodeOSJobType, jobName string) { if nodeOSJobType != RHEL9 { - // TODO(muller): Assume we do not have RHCOS10 hypershift jobs now. If someone adds a RHCOS10 job, this failure + // TODO(muller): Assume we do not have RHCOS10/mixed hypershift jobs now. If someone adds such a job, this failure // should force them to figure out how to detect RHCOS10 in hypershift and update this test. - e2e.Failf("TODO: job name %q indicates RHCOS10 which cannot be checked for HyperShift clusters now", jobName) + e2e.Failf("TODO: job name %q indicates RHCOS10 or mixed OS which cannot be checked for HyperShift clusters now", jobName) return }🤖 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/extended/ci/job_names.go` around lines 233 - 254, The condition `nodeOSJobType != RHEL9` in both `validateMicroshiftNodeOS` and `validateHypershiftNodeOS` functions will match any non-RHEL9 type including RHELMixed910, but the error message only mentions RHCOS10, creating a misleading failure message. Update the error messages in both functions to accurately reflect that any non-RHEL9 job type (not just RHCOS10) cannot be checked for MicroShift and HyperShift clusters respectively. This will ensure the failure message is accurate regardless of which specific non-RHEL9 type triggers the condition.
🤖 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 `@test/extended/ci/job_names.go`:
- Around line 233-254: The condition `nodeOSJobType != RHEL9` in both
`validateMicroshiftNodeOS` and `validateHypershiftNodeOS` functions will match
any non-RHEL9 type including RHELMixed910, but the error message only mentions
RHCOS10, creating a misleading failure message. Update the error messages in
both functions to accurately reflect that any non-RHEL9 job type (not just
RHCOS10) cannot be checked for MicroShift and HyperShift clusters respectively.
This will ensure the failure message is accurate regardless of which specific
non-RHEL9 type triggers the condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5dad5b57-b3ba-416a-8f35-1095a31e38ef
📒 Files selected for processing (1)
test/extended/ci/job_names.go
openshift-ci
Bot
added
the
ready-for-human-review
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/payload 5.0 nightly blocking |
Contributor
|
@pablintino: trigger 12 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a4769770-6958-11f1-8c70-40fdd2272de4-0 |
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/extended/ci/job_names.go`:
- Around line 281-292: The current validation in the mixed cluster check only
verifies that both rhel-9 and rhel-10 streams are present, but does not reject
unexpected or invalid stream values. Add validation logic to ensure that every
stream in the mcpsStreams slice is either "rhel-9" or "rhel-10", and fail the
test if any unexpected, empty, or unrelated streams are encountered. This should
be checked after confirming both required streams are present.
🪄 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: 2519ecbf-d801-452c-b534-1c8d7ad91d9e
📒 Files selected for processing (1)
test/extended/ci/job_names.go
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/payload 5.0 nightly blocking |
Contributor
|
@pablintino: trigger 12 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e4748820-6992-11f1-9ee7-41084e8b0190-0 |
Contributor
Author
|
/payload 5.0 nightly blocking |
Contributor
|
@pablintino: trigger 12 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/220fed90-6a16-11f1-830b-e25aaa00ab32-0 |
This change reverts MCO-2370 and restores the test that checks if the OS of the job node's is the expected one based on the job name. This change also introduced a few changes compared to the original implementation: - Assumes that the default OS is now RHEL 10 - Assumes OSImageStreams is now always available - Doesn't use the debug pods to check mixed clusters Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
Contributor
Author
|
/payload 5.0 nightly blocking |
Contributor
|
@pablintino: trigger 12 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d857ede0-6a5d-11f1-900a-041e8c8f0772-0 trigger 69 job(s) of type informing for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d857ede0-6a5d-11f1-900a-041e8c8f0772-1 |
Contributor
|
Scheduling required tests: |
|
Job Failure Risk Analysis for sha: 0e592b9
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change reverts MCO-2370 and restores the test that checks if the OS of the job node's is the expected one based on the job name. This change also introduced a few changes compared to the original implementation:
Summary by CodeRabbit