CNTRLPLANE-1364: feat: enable global pull secret for AWS - #6736
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/test unit |
|
/test verify |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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
approved
jparrill
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-1364 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
|
/jira refresh |
|
@jparrill: This pull request references CNTRLPLANE-1364 which is a valid 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. |
|
/lgtm |
|
/test e2e-aws |
|
/test e2e-aks |
Test Resultse2e-aws
Failed TestsTotal failed tests: 21
... and 16 more failed tests e2e-aks
Failed TestsTotal failed tests: 13
... and 8 more failed tests |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@jparrill: This pull request references CNTRLPLANE-1364 which is a valid 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/util/util.go (1)
1718-1720: Broadened gate is correct; make skip message more informative and precise.Pass/fail behavior is fine. Use Skipf to include the actual platform and avoid “ARO” as a platform name; prefer “Azure (ARO) or AWS”.
- if entryHostedCluster.Spec.Platform.Type != hyperv1.AzurePlatform && entryHostedCluster.Spec.Platform.Type != hyperv1.AWSPlatform { - t.Skip("test only supported on platform ARO or AWS") - } + if pt := entryHostedCluster.Spec.Platform.Type; pt != hyperv1.AzurePlatform && pt != hyperv1.AWSPlatform { + t.Skipf("test only supported on Azure (ARO) or AWS; got %s", pt) + }control-plane-operator/hostedclusterconfigoperator/cmd.go (1)
285-287: Registering globalps on AWS looks good; prefer type-safe switch over string comparisons.This keeps behavior but avoids stringly-typed checks and reads cleaner.
- if o.platformType == string(hyperv1.AzurePlatform) || o.platformType == string(hyperv1.AWSPlatform) { - controllersToRun[globalps.ControllerName] = globalps.Setup - } + switch hyperv1.PlatformType(o.platformType) { + case hyperv1.AzurePlatform, hyperv1.AWSPlatform: + controllersToRun[globalps.ControllerName] = globalps.Setup + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/cmd.go(1 hunks)test/e2e/util/util.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*.pb).go
📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)
**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...
Files:
control-plane-operator/hostedclusterconfigoperator/cmd.gotest/e2e/util/util.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)
Use
make lint-fixafter writing Go code to automatically fix most linting issuesFollow the rules defined in @100-go-mistakes.mdc for Go code
Files:
control-plane-operator/hostedclusterconfigoperator/cmd.gotest/e2e/util/util.go
🧠 Learnings (3)
📚 Learning: 2025-08-29T18:49:17.570Z
Learnt from: CR
PR: openshift/hypershift#0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T18:49:17.570Z
Learning: Applies to {hypershift-operator,control-plane-operator}/controllers/** : Place platform-specific implementations within their respective controller subdirectories to keep platform logic isolated
Applied to files:
control-plane-operator/hostedclusterconfigoperator/cmd.go
📚 Learning: 2025-08-29T18:49:17.570Z
Learnt from: CR
PR: openshift/hypershift#0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T18:49:17.570Z
Learning: Applies to hypershift-operator/controllers/**/*.go : Place operator controller implementations under hypershift-operator/controllers/
Applied to files:
control-plane-operator/hostedclusterconfigoperator/cmd.go
📚 Learning: 2025-08-29T18:49:17.570Z
Learnt from: CR
PR: openshift/hypershift#0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T18:49:17.570Z
Learning: Applies to control-plane-operator/controllers/**/*.go : Place control-plane controller implementations under control-plane-operator/controllers/
Applied to files:
control-plane-operator/hostedclusterconfigoperator/cmd.go
🧬 Code graph analysis (2)
control-plane-operator/hostedclusterconfigoperator/cmd.go (2)
api/hypershift/v1beta1/hostedcluster_types.go (2)
AzurePlatform(1155-1155)AWSPlatform(1140-1140)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go (2)
AzurePlatform(1155-1155)AWSPlatform(1140-1140)
test/e2e/util/util.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (2)
AzurePlatform(1155-1155)AWSPlatform(1140-1140)
|
/retest |
|
/lgtm |
|
/test e2e-aws-4-20 |
|
Hey @gaol do you mind verify this? |
|
I have done the pre-merge testing using my own CPO image with this pr, and it looks good to me. The test also passed in the draft auto test ci: openshift/release#68899 |
|
/verified by @gaol |
openshift-ci-robot
added
the
verified
|
@gaol: This PR has been marked as verified by 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. |
|
/test e2e-aws-upgrade-hypershift-operator |
2 similar comments
|
/retest |
|
failures of |
|
/retest |
Yeah, that's weird, right now the condition to execute this is explicitly set in the code:
|
|
@jparrill: 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. |
|
/test e2e-aws-4-20 |
6 participants
What this PR does / why we need it
Which issue(s) this PR fixes:
Summary by CodeRabbit
New Features
Tests