check-cluster-profiles-config: Workround to handle Cluster Profile Sets#5282
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthrough
check-cluster-profiles-config
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, psalajova 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.
Actionable comments posted: 2
🤖 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 `@cmd/check-cluster-profiles-config/main.go`:
- Around line 178-185: The profile-set bypass in the validator is hardcoded to
specific names, which will drift from the real source of truth. Update the logic
in main.go around the validator.profiles loop to derive skip behavior from the
resolved cluster profile-set metadata (or profile details) instead of matching
profileName against a fixed set, so new valid sets are handled without code
changes.
- Around line 190-192: The error handling in the config-resolver branch drops
the original failure, so the aggregate error loses the real cause. Update the
`check-cluster-profiles-config` flow where `profileName` details are appended to
`errs` to wrap `err` with `fmt.Errorf("...: %w", err)` just like the secret
lookup branch, keeping the message lowercase and without trailing punctuation.
🪄 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: d48a3fb3-9ce1-4dd8-b5e4-5011e885534f
📒 Files selected for processing (1)
cmd/check-cluster-profiles-config/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
| // FIXME: temporary workaround, we should read this information from a config file. | ||
| clusterProfileSets := sets.New("openshift-org-aws", "openshift-org-azure", "openshift-org-gcp") | ||
| errs := make([]error, 0) | ||
|
|
||
| for p := range validator.profiles { | ||
| profileDetails, err := server.NewResolverClient(configResolverAddress).ClusterProfile(p.Name()) | ||
| profileName := p.Name() | ||
| if clusterProfileSets.Has(profileName) { | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don't hardcode the profile-set bypass.
This validator gates openshift/release, and cluster profile sets are already a first-class concept in adjacent config/jobs. A fixed name list here will drift and start rejecting valid new sets until this binary is patched again. Please drive the skip from the source-of-truth profile-set metadata (or the resolved profile details) instead of profile names.
🤖 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 `@cmd/check-cluster-profiles-config/main.go` around lines 178 - 185, The
profile-set bypass in the validator is hardcoded to specific names, which will
drift from the real source of truth. Update the logic in main.go around the
validator.profiles loop to derive skip behavior from the resolved cluster
profile-set metadata (or profile details) instead of matching profileName
against a fixed set, so new valid sets are handled without code changes.
Source: Linked repositories
| if err != nil { | ||
| return fmt.Errorf("failed to retrieve details from config resolver for '%s' cluster profile", p.Name()) | ||
| errs = append(errs, fmt.Errorf("failed to retrieve details from config resolver for '%s' cluster profile", profileName)) | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve the resolver failure cause.
This branch drops err, so the aggregate loses the actual config-resolver failure. Wrap it the same way as the secret lookup branch.
Suggested fix
- errs = append(errs, fmt.Errorf("failed to retrieve details from config resolver for '%s' cluster profile", profileName))
+ errs = append(errs, fmt.Errorf("failed to retrieve details from config resolver for %q cluster profile: %w", profileName, err))As per path instructions, **/*.go: Wrap errors with fmt.Errorf("context: %w", err), lowercase messages, no trailing punctuation; and coding guidelines, Errors should be handled correctly - determine whether to ignore, log, wrap and raise up.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| return fmt.Errorf("failed to retrieve details from config resolver for '%s' cluster profile", p.Name()) | |
| errs = append(errs, fmt.Errorf("failed to retrieve details from config resolver for '%s' cluster profile", profileName)) | |
| continue | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("failed to retrieve details from config resolver for %q cluster profile: %w", profileName, err)) | |
| continue |
🤖 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 `@cmd/check-cluster-profiles-config/main.go` around lines 190 - 192, The error
handling in the config-resolver branch drops the original failure, so the
aggregate error loses the real cause. Update the `check-cluster-profiles-config`
flow where `profileName` details are appended to `errs` to wrap `err` with
`fmt.Errorf("...: %w", err)` just like the secret lookup branch, keeping the
message lowercase and without trailing punctuation.
Sources: Coding guidelines, Path instructions
|
/override ci/prow/integration |
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/integration 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 kubernetes-sigs/prow repository. |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
/override ci/prow/images |
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/images 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 kubernetes-sigs/prow repository. |
|
@danilo-gemoli: 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
Cluster Profile Sets are special, they don't have a secret associated with them. This PR introduces a workaround to skip that check in case of cluster profile sets.
I'll come up with a solid solution once the cluster profile migration is done.
check-cluster-profiles-confignow treats Cluster Profile Sets as a special case and skips the secret lookup for those profiles, since they do not have associated secrets yet. For the remaining profiles, it now checks all referenced secrets and reports any lookup failures together in one aggregated error instead of stopping at the first problem. This is a temporary workaround to keep CI config validation working until the cluster profile migration is complete.