Reclassify spot-check jobs from 'rare' to 'spotcheck-30d' tier#3617
Conversation
Introduce SpotCheckComponent and SpotCheckCapability variants for cpu-partitioning and etcd-scaling jobs, replacing the generic 'rare' tier with the explicit 'spotcheck-30d' tier. Add validation that spot-check jobs must define both component and capability variants. This prepares the variant registry for the upcoming spot-check analysis feature in Component Readiness while keeping BigQuery data compatible via the existing COALESCE fallback SQL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds spot-check classification: new exported variant keys, a classifier that infers component and capability from job names, tier assignment to ChangesSpot-Check Variant Classification
🎯 3 (Moderate) | ⏱️ ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 3 warnings)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
Scheduling required tests: |
![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)
pkg/variantregistry/ocp_test.go (1)
2207-2266: ⚡ Quick winAdd negative tests for the new spot-check validation path.
These additions cover classification, but there’s no direct regression test for invalid spot-check variants (missing
VariantSpotCheckComponent/VariantSpotCheckCapability) and aggregated error behavior inLoadExpectedJobVariants/VariantSnapshot.Identify.As per coding guidelines: “New or modified functionality must include test coverage … bug fixes should include a regression test that fails without the fix.”
🤖 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_test.go` around lines 2207 - 2266, Add negative tests exercising the new spot-check validation path by adding test cases in pkg/variantregistry/ocp_test.go that simulate spot-check job names but omit VariantSpotCheckComponent and/or VariantSpotCheckCapability from the expected map, then assert that LoadExpectedJobVariants (and/or VariantSnapshot.Identify) returns an error and that multiple missing-field errors are aggregated; locate the test table entries near the existing spot-check cases and add corresponding failing-case entries and assertions that check for a non-nil error and that the error message contains both missing-field details (or uses the aggregated error type).Source: Coding guidelines
🤖 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 `@pkg/variantregistry/ocp.go`:
- Around line 263-271: The aggregated validation errors collected into the errs
slice in the loop over variantsByJob (where validateSpotCheckVariants is called)
are nondeterministic because map iteration order varies; sort errs (e.g., using
sort.Strings(errs)) before calling strings.Join so the returned error message is
stable. Make the same change in the analogous aggregation in
pkg/variantregistry/snapshot.go (the errs slice/strings.Join there) to ensure
deterministic ordering across runs.
---
Nitpick comments:
In `@pkg/variantregistry/ocp_test.go`:
- Around line 2207-2266: Add negative tests exercising the new spot-check
validation path by adding test cases in pkg/variantregistry/ocp_test.go that
simulate spot-check job names but omit VariantSpotCheckComponent and/or
VariantSpotCheckCapability from the expected map, then assert that
LoadExpectedJobVariants (and/or VariantSnapshot.Identify) returns an error and
that multiple missing-field errors are aggregated; locate the test table entries
near the existing spot-check cases and add corresponding failing-case entries
and assertions that check for a non-nil error and that the error message
contains both missing-field details (or uses the aggregated error type).
🪄 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: 2f6d2be2-359f-48a0-a4af-bffa14970885
📒 Files selected for processing (4)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.gopkg/variantregistry/snapshot.yaml
Sort the aggregated spot-check validation errors before joining them into the error message, since map iteration order is nondeterministic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
| } | ||
| if len(errs) > 0 { | ||
| sort.Strings(errs) | ||
| return nil, errors.New("variant registry validation failed:\n" + strings.Join(errs, "\n")) |
There was a problem hiding this comment.
nit: You can build a bit friendly error by wrapping the underlying errors instead of appending the errors' strings into a new error's string.
return nil, fmt.Errorf("variant registry validation failed:\n %w", errors.Join(errs...))
| FeatureSet: default | ||
| Installer: ipi | ||
| JobTier: rare | ||
| JobTier: spotcheck-30d |
There was a problem hiding this comment.
Is this yaml file generated programatically?
There was a problem hiding this comment.
Yes, via make update-variants for prs and periodic-prow-auto-sippy-config-generator
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, neisw 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 |
We certainly don't need to hold for my super nit. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
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. |
3 participants
Introduce SpotCheckComponent and SpotCheckCapability variants for
cpu-partitioning and etcd-scaling jobs, replacing the generic 'rare'
tier with the explicit 'spotcheck-30d' tier. Add validation that
spot-check jobs must define both component and capability variants.
This prepares the variant registry for the upcoming spot-check analysis
feature in Component Readiness while keeping BigQuery data compatible
via the existing COALESCE fallback SQL.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Preparation for #3612, will allow for better testing locally before calling for review there.
Summary by CodeRabbit
New Features
Improvements
Tests