-
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 Builders #9346
🌱 Add MachinePool Builders #9346
Conversation
/remove-area testing |
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
Thanks!
LGTM label has been added. Git tree hash: 99275d7fe3e582d3899992e31cd823b464ed8a41
|
cc @chrischdi |
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 one minor comment about the current diff! For blueprint_test.go
, I needed to also create a InfrastructureMachinePoolTemplateBuilder
type as defined here. Would you be able to include this in your PR?
Sure, I adjusted the PR according to your changes. Thx! In my tests I was just simply passing the ouput of InfrastructureMachineTemplate into the MachinePoolClass. Therefore, I'm a bit wondering about the reason behind naming this with |
Lost the overview a bit. Please let me know if there are open questions (ideally open a conversation at the corresponding code location :)) |
@johannesfrey looks like you actually need the kind stuff: https://github.com/kubernetes-sigs/cluster-api/pull/9346/files#r1312902556 |
I went ahead to quickly 😅. Will add another commit. |
Just please make sure you use the right Generic vs Test kind (just compare with the existing pre-existing builders) |
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 label has been added. Git tree hash: 79cd0f9b1fc9143badf0bf41e5e601053ce0bea9
|
Had to do another renaming commit. Should I squash the commits before another lgtm @chrischdi ? |
If you only squash the commits (i.e. without a rebase onto main) lgtm is preserved More precisely lgtm is preserved if the git tree hash doesn't change (it is stored in a comment, e.g.: #9346 (comment), see details) |
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.
Almost there :)
Looks good. Thank you very much!! /lgtm approve pending squash (please just squash the commits without rebasing to main :)) |
LGTM label has been added. Git tree hash: dbae36c15261e617fb5b72109c954b33d03874a7
|
f7d2a63
to
f0ffda0
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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:
This PR adds MachinePool Builders that can be referenced in tests.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Prereq for some of the MachinePool tests tracked in #5991
/area testing
Johannes Frey <johannes.frey@mercedes-benz.com>, Mercedes-Benz Tech Innovation GmbH (Provider Information)