-
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 E2E test for MachineSet Preflight checks #8698
🌱 add E2E test for MachineSet Preflight checks #8698
Conversation
@killianmuldoon When you have some time, can you do a sanity check on the changes to the test/e2e folder in this PR? |
ed534eb
to
418b52c
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.
Just a few nits + golangci-lint has some findings
f052aca
to
df358cb
Compare
/test pull-cluster-api-e2e-main |
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.
Last round of nits from my side
Lgtm pending underlying PRs are merged + squash/rebase |
This PR looks like it includes actual implementation of these checks and not just E2E, should we retitle? |
Those commits will be dropped. We have to merge underlying PRs first which is also the reason why the PR is on hold. xref #8698 (comment) |
@jackfrancis If you want to review, everything in test/ will stay in this PR. |
// should be blocked because the preflight checks should not pass (kubeadm version skew preflight check should fail). | ||
func machineSetPreflightChecksTestHandler(ctx context.Context, c client.Client, clusterRef types.NamespacedName) { | ||
// Verify that the hook is called and the topology reconciliation is blocked. | ||
hookName := "AfterControlPlaneUpgrade" |
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.
I almost wonder if this check for the AfterControlPlaneUpgrade
hook should be generalized as part of every control plane upgrade flow, and not incorporated into this MachineDeployment validation. Is that a good idea, and viable?
(I'm assuming that this hook is always called after a control plane upgrade, as its name suggests.)
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.
It is only called if there is a Runtime Extension deployed. This test is the only one who uses Runtime Extensions.
To be honest, I"m not sure I understand what you mean with "generalized as part of every control plane upgrade flow"
@ykakarap Just merged the underlying PR, can you please rebase this PR? |
Will do. |
340c23a
to
574fda7
Compare
Rebased. |
/lgtm /assign @fabriziopandini |
LGTM label has been added. Git tree hash: 1e7e06df9d05131473eb3039f30a2742e8c362c0
|
574fda7
to
9aafe17
Compare
/lgtm |
LGTM label has been added. Git tree hash: 4ffe9094da9a4579c932021f4597605f0969a664
|
/assign @fabriziopandini |
/hold cancel |
[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 |
/area e2e-testing |
What this PR does / why we need it:
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 #