CORS-4542: restrict ClusterAPI machine management to only supported platforms - #10717
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@tthvo: This pull request references CORS-4542 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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 (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughChangesAWS Cluster API machine management
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/cc @mdbooth |
![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
🤖 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/types/validation/installconfig.go`:
- Around line 1699-1706: The AWS branch in the platform management validation
must reject every machine-pool role except MachinePoolComputeRoleName and
MachinePoolEdgeRoleName; update the role check around the existing
platform.Name() switch while preserving the current error behavior. Add an
invalid AWS arbiter-management case in pkg/types/validation/machinepools_test.go
lines 254-293 to verify MachinePoolArbiterRoleName is rejected.
🪄 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: eba34791-5971-454e-98d4-77df57003ed2
📒 Files selected for processing (9)
pkg/types/aws/validation/featuregates.gopkg/types/aws/validation/featuregates_test.gopkg/types/defaults/machinepools.gopkg/types/defaults/machinepools_test.gopkg/types/validation/featuregate_test.gopkg/types/validation/featuregates.gopkg/types/validation/installconfig.gopkg/types/validation/machinepools.gopkg/types/validation/machinepools_test.go
💤 Files with no reviewable changes (1)
- pkg/types/validation/featuregates.go
|
/test e2e-aws-ovn-dualstack-ipv6-primary-techpreview |
|
@tthvo: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a6cd3550-8a4c-11f1-8cdf-adccde30ffe0-0 |
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
Lets keep it simple and close #10659. |
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
Looks fine except for the incorrect (and now duplicated) use of FeatureSets in the tests. Just use the FeatureGate, because that's the only thing you control.
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
@tthvo: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6bde8730-8ab7-11f1-9e31-6a71fec509f0-0 |
|
/cc @sadasu |
|
/test ? |
|
/test e2e-aws-ovn-techpreview |
|
/lgtm I would like to see the e2e-aws-ovn-techpreview job pass. |
|
/test e2e-aws-ovn-dualstack-ipv4-primary-techpreview |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadasu 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
|
I have 1 inline comment about featuregate validation. Can new compute or edge manifests be added day-2 with the CAPI management type? |
|
e2e-aws-ovn-techpreview failed due to a flake, but my testing concern is covered by e2e-aws-ovn-dualstack-ipv6-primary-techpreview, which passed. |
Oh yes, day-2 compute/edge machines can be created by directly creating CAPI machine manifests. The install-config field |
openshift-ci-robot
added
the
verified
|
@tthvo: 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. |
|
@tthvo: The following tests 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. |
|
/tide refresh |
759698c
into
openshift:main
4 participants
Description
The PR introduces the following:
controlPlane.management: ClusterAPIbecause it is not implemented in either installer or CPMS.FeatureGateClusterAPIComputeInstallbyFeatureGateClusterAPIMachineManagementAWSand scope to AWS only, as that is the only platform where it is implementedThe effect of this change is that installer-created worker machinesets will use CAPI as soon as platform support is promoted.
Follow-up for #10659
Summary by CodeRabbit
New Features
managementnow defaults to Cluster API on AWS compute/edge when the required feature gate is enabled.Bug Fixes
Tests