-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add MachinePool Machine implementation in core CAPI components #8828
Conversation
29bcdf1
to
ae90e1a
Compare
/assign @fabriziopandini I've pulled out the core CAPI implementation. I think this is the implementation that we really want to get merged first so that other providers can get their hands on it when the next release comes. |
/retest |
2 similar comments
/retest |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass
test/infrastructure/docker/exp/api/v1beta1/dockermachinepool_webhook.go
Outdated
Show resolved
Hide resolved
a6d0493
to
f688868
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm aside from the thread about removing the selector
@fabriziopandini @sbueringer I've reverted the changes in the Machine controller and removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor comments. Otherwise looks good to me
changes look good to me pending open comments and a feedback about tests with CAPD |
ed1cb76
to
f60fa9d
Compare
/retest |
lgtm pending golangci-lint:
(No idea about all the errors in: https://github.com/kubernetes-sigs/cluster-api/actions/runs/5492454930/jobs/10037227687, fixing that one finding lead to a green run locally) |
/lgtm |
LGTM label has been added. Git tree hash: fa939587157fc96e970f7ab11bf406347df4a84e
|
/assign @CecileRobertMichon @fabriziopandini |
@fabriziopandini Good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/lgtm Yay! great work! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it: Add implementation for MachinePool Machines based off of this proposal. This only affects Machines and MachinePools, and the DockerMachinePool implementation and fallback machine implementation will come in a follow up.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #4063