✨ Add pivoting test based on the e2e test framework #232

namnx228 · 2021-06-28T07:26:07Z

What this PR does / why we need it:
This PR adds the pivoting test based on the e2e test framework.

namnx228 · 2021-06-28T12:52:22Z

/test-v1a5-e2e

namnx228 · 2021-06-29T10:17:35Z

/test-v1a5-e2e

namnx228 · 2021-08-02T08:26:53Z

/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-02T08:36:07Z

/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-02T09:30:28Z

/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-02T12:21:11Z

/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-02T14:14:28Z

/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-03T07:22:24Z

/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-03T09:52:50Z

/test-integration

furkatgofurov7

This is great effort and basement for all other upcoming e2e tests after this, good job @namnx228! I have few comments/suggestions/questions inline:

go.mod

hack/e2e/environment.sh

furkatgofurov7 · 2021-08-03T15:30:58Z

test/e2e/data/cni/calico/calico-centos.yaml

@@ -0,0 +1,3894 @@
+---


Clarification question: there is no way of using single calico manifest for both Ubuntu and Centos tests?

There is a way to do that which is the same as https://github.com/metal3-io/metal3-dev-env/blob/54abb30bb076e237de68b804d3bffe52235ccdb9/vm-setup/roles/v1aX_integration_test/tasks/verify.yml#L75
However, I think we should do it in another patch to avoid adding more complexity to this PR.

test/e2e/pivoting_test.go

furkatgofurov7 · 2021-08-03T15:51:37Z

test/e2e/pivoting_test.go

+		fmt.Sprintf("IRONIC_HOST=%s", ironicHost),
+		fmt.Sprintf("IRONIC_HOST_IP=%s", ironicHost),


what is the diff between these two?

I think they are the same, but it is required by deploy.sh: https://github.com/metal3-io/metal3-dev-env/blob/54abb30bb076e237de68b804d3bffe52235ccdb9/vm-setup/roles/v1aX_integration_test/tasks/move.yml#L122

Strange, in fact they are the same and are coming from m3-dev-env

test/e2e/pivoting_test.go

furkatgofurov7 · 2021-08-03T15:52:36Z

test/e2e/pivoting_test.go

+	err := cmd.Start()
+	Expect(err).To(BeNil(), "Fail to deploy Ironic")
+	err = cmd.Wait()
+	Expect(err).To(BeNil(), "Fail to deploy Ironic")


test/e2e/pivoting_test.go

furkatgofurov7 · 2021-08-03T16:04:28Z

test/e2e/pivoting_test.go

+}
+func removeIronicDeployment() {
+	deploymentName := os.Getenv("NAMEPREFIX") + "-ironic"
+	ironicNamespace := os.Getenv("IRONIC_NAMESPACE")


I do also suggest we take all vars like these here and in all other funcs so we could define them at the top once and re-use them (if applicable) later, since some of them redefined twice in different funcs, i.e IRONIC_NAMESPACE, BMOPATH

I agree that some variables are redefined some times in this PR. However, I think it is not too bad since these variables are mostly in type string, not big objects.I kind of reluctant to use global variables if we only need them in one test to avoid unnecessary access to these variables from other tests. Therefore, I would use local variables in this case.

I meant to define them in the pivoting_test.go not in e2e_suite_test.go or somewhere else, do you still think it would be accessible by other tests?

namnx228 · 2021-08-04T08:53:08Z

/test-v1a5-centos-e2e
/test-v1a5-e2e
/test-integration

furkatgofurov7 · 2021-08-04T09:01:09Z

go.mod

@@ -3,7 +3,7 @@ module github.com/metal3-io/cluster-api-provider-metal3
 go 1.16

 require (
-	github.com/Microsoft/go-winio v0.5.0 // indirect
+	// github.com/Microsoft/go-winio v0.5.0 // indirect


I think you need to run go mod tidy to get rid of these dependency updates, since just uncommenting did not help.

Oh, ok. I commented to test if the code can work without that module, then I forgot to actually delete it ^^

furkatgofurov7 · 2021-08-04T09:08:54Z

test/e2e/pivoting_test.go

+}
+func removeIronicDeployment() {
+	deploymentName := os.Getenv("NAMEPREFIX") + "-ironic"
+	ironicNamespace := os.Getenv("IRONIC_NAMESPACE")


I meant to define them in the pivoting_test.go not in e2e_suite_test.go or somewhere else, do you still think it would be accessible by other tests?

namnx228 · 2021-08-04T09:23:46Z

I meant to define them in the pivoting_test.go not in e2e_suite_test.go or somewhere else, do you still think it would be accessible by other tests?

@furkatgofurov7 Yes, even when we define the global variables in the pivoting_test.go, other tests in other files in package e2e can still access it.

namnx228 · 2021-08-04T09:25:22Z

/test-v1a5-centos-e2e
/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-04T12:21:49Z

/test-v1a5-centos-e2e
/test-v1a5-e2e
/test-integration

namnx228 · 2021-08-04T14:23:34Z

/test-v1a5-e2e
/test-integration

test/e2e/config/e2e_conf.yaml

Add some check to ensure that pivoting is successful Use go.mod and go.sum from master Fix golint error Fix go dependency More fix on Go dependency Fix Temporary disable log The rest of pivoting test now can run fix Fix `clusterctl init` without checking all deployments running Fix the lint problem Force update on the BMO repo Change order No longer getting stuck Make pivoting become a step Fix golint error Fix Fix fix fix Install BMO in v1alpha5 Fix Add labels to BMO CRDs in v1alpha5 fix fix Pass the CI Remove the use of variables in the config file Fix undeclared variables Fix Fix Fix to pass the unit and lint test Address comments

namnx228 · 2021-08-05T10:25:32Z

/test-v1a5-e2e
/test-integration

furkatgofurov7

I am happy with these changes, we can think of combining calico manifests into a single one and use that for both Ubuntu/CentOS e2e test runs in the future patches.

/approve

metal3-io-bot · 2021-08-05T22:20:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7, Rozzii

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:

~~OWNERS~~ [furkatgofurov7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fmuyassarov · 2021-08-06T07:29:18Z

/lgtm

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 28, 2021

namnx228 changed the title ~~✨ Add some check to ensure that pivoting is successful~~ WIP: ✨ Add some check to ensure that pivoting is successful Jun 28, 2021

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2021

namnx228 changed the title ~~WIP: ✨ Add some check to ensure that pivoting is successful~~ WIP: ✨ Add pivoting test based on the e2e test framework Jun 28, 2021

namnx228 force-pushed the e2e-pivoting-nam branch 2 times, most recently from 1dd185c to 3b3af6e Compare June 28, 2021 12:49

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 28, 2021

namnx228 force-pushed the e2e-pivoting-nam branch 2 times, most recently from 8670126 to d44d27d Compare June 29, 2021 06:51

metal3-io-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 29, 2021

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2021

namnx228 force-pushed the e2e-pivoting-nam branch 3 times, most recently from f1bdf04 to a365924 Compare July 1, 2021 07:41

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2021

namnx228 force-pushed the e2e-pivoting-nam branch 11 times, most recently from 19bb77f to 3b22f17 Compare July 6, 2021 14:37

Rozzii approved these changes Aug 3, 2021

View reviewed changes

furkatgofurov7 reviewed Aug 3, 2021

View reviewed changes

furkatgofurov7 reviewed Aug 4, 2021

View reviewed changes

namnx228 force-pushed the e2e-pivoting-nam branch from 5ae22db to 5ccc097 Compare August 4, 2021 09:25

namnx228 force-pushed the e2e-pivoting-nam branch from 5ccc097 to fe97fe6 Compare August 4, 2021 12:20

namnx228 force-pushed the e2e-pivoting-nam branch from fe97fe6 to 4067f84 Compare August 4, 2021 14:23

furkatgofurov7 reviewed Aug 5, 2021

View reviewed changes

test/e2e/config/e2e_conf.yaml Outdated Show resolved Hide resolved

namnx228 force-pushed the e2e-pivoting-nam branch from 4067f84 to cc3afbe Compare August 5, 2021 10:25

furkatgofurov7 approved these changes Aug 5, 2021

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2021

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2021

metal3-io-bot merged commit 734e98c into metal3-io:master Aug 6, 2021

kashifest deleted the e2e-pivoting-nam branch June 20, 2022 06:32

Rozzii mentioned this pull request Dec 19, 2023

Metal3 for Incubation Nordix/toc#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add pivoting test based on the e2e test framework #232

✨ Add pivoting test based on the e2e test framework #232

namnx228 commented Jun 28, 2021

namnx228 commented Jun 28, 2021

namnx228 commented Jun 29, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 3, 2021

namnx228 commented Aug 3, 2021

furkatgofurov7 left a comment

furkatgofurov7 Aug 3, 2021

namnx228 Aug 4, 2021

furkatgofurov7 Aug 3, 2021

namnx228 Aug 4, 2021

furkatgofurov7 Aug 4, 2021

furkatgofurov7 Aug 3, 2021

furkatgofurov7 Aug 3, 2021

namnx228 Aug 4, 2021

furkatgofurov7 Aug 4, 2021

namnx228 commented Aug 4, 2021

furkatgofurov7 Aug 4, 2021

namnx228 Aug 4, 2021

furkatgofurov7 Aug 4, 2021

namnx228 commented Aug 4, 2021 •

edited

Loading

namnx228 commented Aug 4, 2021

namnx228 commented Aug 4, 2021

namnx228 commented Aug 4, 2021

namnx228 commented Aug 5, 2021

furkatgofurov7 left a comment

metal3-io-bot commented Aug 5, 2021

fmuyassarov commented Aug 6, 2021

		fmt.Sprintf("IRONIC_HOST=%s", ironicHost),
		fmt.Sprintf("IRONIC_HOST_IP=%s", ironicHost),

✨ Add pivoting test based on the e2e test framework #232

✨ Add pivoting test based on the e2e test framework #232

Conversation

namnx228 commented Jun 28, 2021

namnx228 commented Jun 28, 2021

namnx228 commented Jun 29, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 2, 2021

namnx228 commented Aug 3, 2021

namnx228 commented Aug 3, 2021

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namnx228 commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namnx228 commented Aug 4, 2021 • edited Loading

namnx228 commented Aug 4, 2021

namnx228 commented Aug 4, 2021

namnx228 commented Aug 4, 2021

namnx228 commented Aug 5, 2021

furkatgofurov7 left a comment

Choose a reason for hiding this comment

metal3-io-bot commented Aug 5, 2021

fmuyassarov commented Aug 6, 2021

namnx228 commented Aug 4, 2021 •

edited

Loading