Make component/capability variants more generic#3696
Conversation
This shouldn't be tied to being a spot check job. Many spot check jobs will start out running quite often as part of their feature gate promotion logic.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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)
WalkthroughVariant component and capability keys replace spot-check keys in variant generation, validation, tests, and snapshot data. ChangesComponent/capability variant migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 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: dgoodwin 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp.go (1)
763-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep ownership and spot-check tier patterns in one place.
The
-cpu-partitioning/-etcd-scalingsubstrings are now duplicated betweensetComponentAndCapabilityandsetJobTier. If a future pattern is added to only one list, validation can failspotcheck-*jobs or silently miss component ownership. Consider a shared pattern table withcomponent,capability, and optionaljobTier.Also applies to: 807-810
🤖 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/variantregistry/ocp.go` around lines 763 - 782, The pattern definitions for ownership/capability matching are duplicated between setComponentAndCapability and setJobTier, which can drift and break spotcheck job handling. Refactor the existing pattern handling in those functions to use one shared table keyed by jobNameLower substrings, and include component, capability, and an optional jobTier so both VariantComponent/VariantCapability and tier assignment stay in sync. Make sure the shared symbols used by both paths are updated consistently instead of maintaining separate substring lists.
🤖 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/variantregistry/ocp.go`:
- Around line 763-782: The pattern definitions for ownership/capability matching
are duplicated between setComponentAndCapability and setJobTier, which can drift
and break spotcheck job handling. Refactor the existing pattern handling in
those functions to use one shared table keyed by jobNameLower substrings, and
include component, capability, and an optional jobTier so both
VariantComponent/VariantCapability and tier assignment stay in sync. Make sure
the shared symbols used by both paths are updated consistently instead of
maintaining separate substring lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f041e435-89e5-4db1-858d-f3cf5ccaed66
📒 Files selected for processing (4)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.gopkg/variantregistry/snapshot.yaml
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
Scheduling required tests: |
|
@dgoodwin: 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. |
| if strings.HasPrefix(variants[VariantJobTier], "spotcheck-") { | ||
| if variants[VariantSpotCheckComponent] == "" || variants[VariantSpotCheckCapability] == "" { | ||
| return fmt.Errorf("job %q has JobTier=%s but is missing SpotCheckComponent or SpotCheckCapability", jobName, variants[VariantJobTier]) | ||
| if variants[VariantComponent] == "" || variants[VariantCapability] == "" { |
There was a problem hiding this comment.
to @stbenjam point on slack, Capability should probably obsolete Procedure. I don't want to play with that just yet as it's used in views, thus part of the tracked regression data we have triaged.
I'd like to do a followup replacing Procedure in the sippy postgres whitelist with Component + Capability. I can't think of a reason we need Procedure in sippy postgres/classic right now. This would help limit the combo explosion, though I'm unclear how relevant that is now with Matthew's work.
mstaeble
left a comment
mstaeble
left a comment
There was a problem hiding this comment.
Overall the changes look good to me. I just have one clarifying question to help me understand better.
| VariantLayeredProduct = "LayeredProduct" | ||
| VariantOS = "OS" | ||
| VariantComponent = "Component" // jobs with an owning component, used to flag regressions if a tailored job is not passing | ||
| VariantCapability = "Capability" // jobs with an owning capability, used to flag regressions if a tailored job is not passing |
There was a problem hiding this comment.
Is this capability the same as the CR capability? Or are we overloading this term?
Is the long-term expectation that many/most jobs will have the component and capability set, even in the absence of the spot-check job tier for the job?
| // Check componentCapabilityPatterns first for tier overrides (e.g. spotcheck). | ||
| for _, p := range componentCapabilityPatterns { | ||
| if p.jobTier != "" && allSubstringsMatch(jobNameLower, p.substrings) { | ||
| variants[VariantJobTier] = p.jobTier | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Since setComponentAndCapability already ran and set VariantComponent/VariantCapability, this could skip the redundant substring matching and use the already-computed result:
if variants[VariantComponent] != "" {
for _, p := range componentCapabilityPatterns {
if p.jobTier != "" && p.component == variants[VariantComponent] && p.capability == variants[VariantCapability] {
variants[VariantJobTier] = p.jobTier
return
}
}
}
This makes the dependency on setter ordering explicit. It's already the case, just not visible in the code.
2 participants
This shouldn't be tied to being a spot check job. Many spot check jobs
will start out running quite often as part of their feature gate
promotion logic.
Summary by CodeRabbit
ComponentandCapabilitylabels.spotcheck-*jobs to require both component and capability variant information.