TRT-2506: Add OS version validation to prow job name test#30826
TRT-2506: Add OS version validation to prow job name test#30826petr-muller wants to merge 1 commit intoopenshift:mainfrom
Conversation
Ensure CI job names reflect the node OS version: RHCOS 10 clusters must have "rhcos10" in the job name, while RHCOS 9 clusters must have no "rhcos*" fragment or exactly "rhcos9". Export IsRHEL10 from machine_config helpers (accepting the Interface type) so it can be called from the ci package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@petr-muller: This pull request references TRT-2506 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 "4.22.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. |
|
@petr-muller: This pull request references TRT-2506 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 "4.22.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. |
WalkthroughThe changes introduce a new test case for validating OS version matching in job names and update the public API signature of the IsRHEL10 helper function to accept an interface type instead of a concrete pointer type for improved abstraction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test ? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller 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
|
@petr-muller: This pull request references TRT-2506 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 "4.22.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. |
![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
🧹 Nitpick comments (1)
test/extended/machine_config/helpers.go (1)
167-168: Consider aligningskipOnRHEL10BeforeMar11with the same interface type.This helper still takes
*machineconfigclient.Clientset; usingmachineconfigclient.Interfacehere would keep the helper APIs consistent.♻️ Suggested consistency refactor
-func skipOnRHEL10BeforeMar11(machineConfigClient *machineconfigclient.Clientset, mcpName string) { +func skipOnRHEL10BeforeMar11(machineConfigClient machineconfigclient.Interface, mcpName string) { if IsRHEL10(machineConfigClient, mcpName) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/machine_config/helpers.go` around lines 167 - 168, The helper skipOnRHEL10BeforeMar11 currently accepts a concrete *machineconfigclient.Clientset; change its first parameter to the interface machineconfigclient.Interface to match the other helpers and keep the API consistent, and update any calls passing a Clientset (which already implements machineconfigclient.Interface) to continue to work; ensure the IsRHEL10 function signature is compatible (it accepts machineconfigclient.Interface) and adjust imports if needed so skipOnRHEL10BeforeMar11(machineconfigclient.Interface, mcpName string) compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/ci/job_names.go`:
- Around line 186-188: The current substring check on jobName is too permissive;
update the logic in the validation block (the code that reads jobName and
originalJobName) to split jobName into tokens (e.g., strings.Split(jobName,
"-")) and then enforce that no token begins with "rhcos" unless that token is
exactly "rhcos9"; if any token startsWith "rhcos" and != "rhcos9" call e2e.Failf
with the same message using originalJobName. This ensures only an exact "rhcos9"
token is allowed and mixed/extended fragments are rejected.
---
Nitpick comments:
In `@test/extended/machine_config/helpers.go`:
- Around line 167-168: The helper skipOnRHEL10BeforeMar11 currently accepts a
concrete *machineconfigclient.Clientset; change its first parameter to the
interface machineconfigclient.Interface to match the other helpers and keep the
API consistent, and update any calls passing a Clientset (which already
implements machineconfigclient.Interface) to continue to work; ensure the
IsRHEL10 function signature is compatible (it accepts
machineconfigclient.Interface) and adjust imports if needed so
skipOnRHEL10BeforeMar11(machineconfigclient.Interface, mcpName string) compiles.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
test/extended/ci/job_names.gotest/extended/machine_config/helpers.go
| // RHCOS 9: must have no rhcos fragment at all, or exactly rhcos9 | ||
| if strings.Contains(jobName, "rhcos") && !strings.Contains(jobName, "rhcos9") { | ||
| e2e.Failf("job name %q contains an unexpected rhcos fragment for a cluster running RHCOS 9", originalJobName) |
There was a problem hiding this comment.
RHCOS 9 validation is too permissive and can miss invalid names.
The current substring logic allows invalid cases (for example mixed or extended fragments) while the requirement is “no rhcos* fragment or exactly rhcos9.”
🐛 Suggested fix to enforce exact RHCOS 9 token semantics
isRHCOS10 := machine_config.IsRHEL10(oc.MachineConfigurationClient(), "worker")
+ rhcosTokens := make([]string, 0, 1)
+ for _, token := range strings.Split(jobName, "-") {
+ if strings.HasPrefix(token, "rhcos") {
+ rhcosTokens = append(rhcosTokens, token)
+ }
+ }
if isRHCOS10 {
if !strings.Contains(jobName, "rhcos10") {
e2e.Failf("job name %q does not contain rhcos10 but cluster is running RHCOS 10", originalJobName)
}
} else {
- // RHCOS 9: must have no rhcos fragment at all, or exactly rhcos9
- if strings.Contains(jobName, "rhcos") && !strings.Contains(jobName, "rhcos9") {
+ // RHCOS 9: must have no rhcos fragment, or exactly one rhcos9 fragment.
+ if len(rhcosTokens) > 1 || (len(rhcosTokens) == 1 && rhcosTokens[0] != "rhcos9") {
e2e.Failf("job name %q contains an unexpected rhcos fragment for a cluster running RHCOS 9", originalJobName)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/ci/job_names.go` around lines 186 - 188, The current substring
check on jobName is too permissive; update the logic in the validation block
(the code that reads jobName and originalJobName) to split jobName into tokens
(e.g., strings.Split(jobName, "-")) and then enforce that no token begins with
"rhcos" unless that token is exactly "rhcos9"; if any token startsWith "rhcos"
and != "rhcos9" call e2e.Failf with the same message using originalJobName. This
ensures only an exact "rhcos9" token is allowed and mixed/extended fragments are
rejected.
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-azure-ovn-rhcos10-techpreview-serial |
|
@petr-muller: 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/9ed573a0-16fa-11f1-9002-4371b3a38667-0 |
|
/test images |
|
@petr-muller: 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. |
2 participants
Ensure CI job names reflect the node OS version: RHCOS 10 clusters must have "rhcos10" in the job name, while RHCOS 9 clusters must have no "rhcos*" fragment or exactly "rhcos9".
Export IsRHEL10 from machine_config helpers (accepting the Interface type) so it can be called from the ci package.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit