-
Notifications
You must be signed in to change notification settings - Fork 98
TRT-2544: normalize job names during variant generation for openshift/release
#3258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@smg247: This pull request explicitly references no 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. |
WalkthroughAdds job-name normalization to the OCP variant registry: a new normalizeJobNameForVariants is applied before computing and storing variants, with tracking of original sources and collision resolution between normalized names; conditional logging reports when names are normalized or collisions are resolved. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smg247 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/variantregistry/ocp.go`:
- Around line 262-266: The current normalizeJobNameForVariants function is too
broad: it uses strings.Contains and ReplaceAll which may affect repos like
"openshift-release-operator" or multiple "-master-" occurrences; update
normalizeJobNameForVariants to parse the jobName into segments (or use a regex)
to explicitly detect the repo segment equal to "openshift/release" (or the token
"openshift-release" as the exact repo segment) and only replace the single
branch token "-master-" for that segment once; ensure you target the branch
portion (not other path parts) and perform a single replacement rather than
ReplaceAll so only the intended branch token is normalized.
- Around line 237-245: The current blind assignment to variantsByJob for key
normalizedJobName is nondeterministic when multiple goroutines collapse names
like "master" and "main"; change the logic so under variantsByJobMu you first
check if an entry already exists and only overwrite it when the new source
should win by a deterministic rule (e.g., create a companion map
variantsByJobSource[string]string or change the stored value to include Source;
on insert check existingSource := variantsByJobSource[normalizedJobName]; if
existingSource != "" then if strings.Contains(existingSource, "master") &&
strings.Contains(jlr.JobName, "main") { overwrite
variantsByJob[normalizedJobName] = variants and set variantsByJobSource[...] =
jlr.JobName } else { keep existing and skip overwrite } else { set
variantsByJob[...] = variants and variantsByJobSource[...] = jlr.JobName }; keep
using normalizeJobNameForVariants, jlr.JobName, normalizedJobName and
v.CalculateVariantsForJob and guard all accesses with variantsByJobMu.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp_test.go (1)
1794-1833: Add a guard case for repos likeopenshift-release-operator.Given the scope requirement (“only openshift/release”), add a test case ensuring a repo name that contains
openshift-releasebut isn’topenshift/releaseremains unchanged.🧪 Suggested test addition
{ name: "other repos with master unchanged", jobName: "periodic-ci-openshift-hive-master-e2e-aws", expected: "periodic-ci-openshift-hive-master-e2e-aws", }, + { + name: "openshift-release-operator should remain unchanged", + jobName: "periodic-ci-openshift-release-operator-master-nightly-4.16-e2e-aws-ovn", + expected: "periodic-ci-openshift-release-operator-master-nightly-4.16-e2e-aws-ovn", + },
|
Scheduling required tests: |
|
@smg247: This pull request explicitly references no 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. |
openshift/releaseopenshift/release
|
@smg247: This pull request references TRT-2544 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 ticket 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. |
|
Scheduling required tests: |
|
/hold we may not need this |
|
@smg247: all tests passed! 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. |
I believe that the variant registry needs to have normalization for job names now that
openshift/releasehas migrated frommastertomain. This solution only affects that repository, and won't normalize others.Currently, we have no regressions in the
Alltab of theRegressedTestsModal, and there are other oddities in CR based on this branch migration.I don't have a good way to confirm that this will solve the issue locally. I would like to merge this and run the job and check the results.
Summary by CodeRabbit
New Features
Tests