-
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
✨introduce test E2E #2884
✨introduce test E2E #2884
Conversation
test/e2e/quick_start_test.go
Outdated
Namespace: namespaceName, | ||
}) | ||
Expect(controlPlane).ToNot(BeNil()) | ||
framework.WaitForOneKubeadmControlPlaneMachineToExist(context.TODO(), framework.WaitForOneKubeadmControlPlaneMachineToExistInput{ |
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 might be better to wait for all machines to be created before the CNI
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.
Is there a particular reason to do so?
AFAIK installing as soon as the control plane is up is legit; it is also a precondition to get the control plane ready...
33ef640
to
bfc95bf
Compare
/assign |
c329dcd
to
9c07d98
Compare
7514e9d
to
e30f0a3
Compare
e30f0a3
to
289bf55
Compare
ccebfad
to
944884e
Compare
feff3db
to
6e4069c
Compare
2d905fd
to
a353b37
Compare
a353b37
to
2595fd1
Compare
2595fd1
to
5319b03
Compare
051c713
to
101cbac
Compare
/test pull-cluster-api-e2e |
Reviewing |
/test pull-cluster-api-e2e |
@@ -71,6 +69,8 @@ func KCPUpgradeSpec(ctx context.Context, inputGetter func() KCPUpgradeSpecInput) | |||
It("Should successfully upgrade Kubernetes, DNS, kube-proxy, and etcd", func() { | |||
|
|||
By("Creating a workload cluster") | |||
Expect(input.E2EConfig.Variables).To(HaveKey(clusterctl.KubernetesVersion)) | |||
Expect(input.E2EConfig.Variables).To(HaveKey(clusterctl.CNIPath)) |
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.
Do we need these checks for every test? We already throw error if they are empty during validation.
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 don't think we should enforce variables to exist during validation... I missed this.
Each spec might require different variables
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.
KubernetesVersion and CNI should exist for all tests? If not we should remove those checks from validation.
/test pull-cluster-api-e2e |
Changes look good to me. Great work @fabriziopandini. |
/test pull-cluster-api-e2e |
/milestone v0.3.4 |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, vincepri 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 |
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 noticed a couple of things regarding contexts. But this is great work! Seems like we even have three e2e tests done!
/test pull-cluster-api-e2e |
@wfernandes thanks for the feedback! |
/test pull-cluster-api-e2e |
fe73407
to
5596ab2
Compare
/test pull-cluster-api-e2e |
@fabriziopandini: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
@wfernandes @sedefsavas for final lgtm |
/lgtm |
What this PR does / why we need it:
This PR introduces an E2e folder that introduces clusterctl based E2E test for quick-start. self-hosted, KCP upgrade.
Most notably:
Which issue(s) this PR fixes:
xref #2753
xref #2637
xref #2636
/assign @vincepri
/assign @sedefsavas