CLOUDP-81150: Replace operator-sdk test with envtest #315

alyacb · 2021-02-03T11:22:42Z

All Submissions:

Have you opened an Issue before filing this PR?
Have you signed our CLA?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).

This PR removes operator-sdk test from our e2e tests. It switches to using go test to run the test and envtest to configure the environment. Most changes to individual e2e tests are just to refactor them to rely on globals/ a new main entry function refactored to test/e2e/client.go.

The downside is that test cleanup no longer happens after tests, which will not affect evergreen runs but will make post-test cleanup a manual process when developing locally.

alyacb · 2021-02-03T11:23:40Z

deploy/e2e/role.yaml

+  - update
+  - watch
+- apiGroups:
+  - rbac.authorization.k8s.io


Needed a new role to add these additional permissions so the test could create the operator role. Furthermore, this had to be a cluster role for the clusterwide test.

alyacb · 2021-02-03T11:24:28Z

go.mod

 	github.com/imdario/mergo v0.3.9
 	github.com/klauspost/compress v1.9.8 // indirect
-	github.com/konsorten/go-windows-terminal-sequences v1.0.3 // indirect
-	github.com/operator-framework/operator-sdk v0.19.4


It looks like the operator-sdk test code was the only dependency on operator-sdk.

alyacb · 2021-02-03T11:28:34Z

scripts/dev/e2e.py

                    ],
                    "command": [
-                        "/bin/operator-sdk",
+                        "go",


Key change here: go test instead of operator-sdk test

alyacb · 2021-02-03T11:29:51Z

scripts/dev/e2e.py

    print("Creating Role")
    k8s_conditions.ignore_if_already_exists(
-        lambda: rbacv1.create_namespaced_role(dev_config.namespace, _load_test_role())
+        lambda: rbacv1.create_cluster_role(_load_test_role())


Needed to use a cluster role to be able to create a cluster role for the clusterwide test.

alyacb · 2021-02-03T11:30:42Z

test/e2e/client.go

@@ -0,0 +1,74 @@
+package e2eutil
+
+import (


This file essentially replaces f.MainEntry, f.Context, and f.Global.

alyacb · 2021-02-03T11:31:39Z

test/e2e/replica_set_cross_namespace_deploy/replica_set_cross_namespace_deploy_test.go

 	postfix, err := generate.RandomValidDNS1123Label(5)
 	if err != nil {
-		t.Fatal()
+		t.Fatal(err)


Flyby fix: I neglected to report errors in this file when I first committed it.

bznein · 2021-02-04T11:51:05Z

Can you also make sure that the parts about e2e tests here: https://github.com/mongodb/mongodb-kubernetes-operator/blob/master/docs/contributing.md are up-to-date? Maybe they already are but I remember writing them some time ago with references to operator-sdk test so maybe they need some tweaking

alyacb · 2021-02-04T11:58:35Z

Can you also make sure that the parts about e2e tests here: https://github.com/mongodb/mongodb-kubernetes-operator/blob/master/docs/contributing.md are up-to-date? Maybe they already are but I remember writing them some time ago with references to operator-sdk test so maybe they need some tweaking

Done in previous commit! I removed mention of operator-sdk and added envtest.

chatton

This is looking great, massive work so far! Just had a few questions.

chatton · 2021-02-04T16:54:22Z

deploy/e2e/operator.yaml

@@ -0,0 +1,40 @@
+apiVersion: apps/v1


[q] do we need to introduce another yaml definition? We would get more benefit out of testing deploy/operator.yaml in our tests. What do you think?

Makes sense! The e2e role.yaml has additional permissions so it can create the test environment. I needed a new operator.yaml and serviceaccount.yaml because I changed the name of the role from mongodb-kubernetes-operator, however I'll look into whether we can just have role and role binding in this directory while reusing deploy/operator.yaml.

Update: I removed operator.yaml from the new e2e directory, since we weren't using it anyway (the test deploys using deploy/operator). We do, however, need a new cluster role, cluster rolebinding, and serviceaccount to create the operator role and role binding in the test itself for the deploy. We also need these to be able to create a new namespace in the cross-namespace test.

chatton · 2021-02-04T16:54:58Z

deploy/e2e/operator.yaml

+      labels:
+        name: mongodb-kubernetes-operator
+    spec:
+      serviceAccountName: e2e-test


is this the only difference in this deployment?

I believe so

test/e2e/client.go

chatton · 2021-02-04T17:07:58Z

test/e2e/client.go

+	code := m.Run()
+
+	err = testEnv.Stop()
+	if err != nil {


[nit] same comment as above

Sorry, I'm not sure which comment this refers to. If it's the exit code one, I also refactored this function to return an error code for the test to handle instead.

ah I think this comment was regarding preferring

if err := fn(); err != nil { return err }

over

err := fn() if err != nil { return err }

Definitely not a blocker.

test/e2e/client.go

test/e2e/e2eutil.go

test/e2e/setup/setup.go

alyacb · 2021-02-05T16:54:12Z

I implemented cleanup logic so that the --preform-cleanup flag works with the new changes. My last commit also added comments/ reorganised setup.go slightly.

chatton

LGTM! Thanks for all the great work with this refactor 🥇

chatton · 2021-02-08T10:05:04Z

test/e2e/client.go

+	code := m.Run()
+
+	err = testEnv.Stop()
+	if err != nil {


ah I think this comment was regarding preferring

if err := fn(); err != nil { return err }

over

err := fn() if err != nil { return err }

Definitely not a blocker.

alyacb added the wip label Feb 3, 2021

alyacb commented Feb 3, 2021

View reviewed changes

scripts/dev/e2e.py

],

"command": [

"/bin/operator-sdk",

"go",

Copy link

Contributor Author

alyacb Feb 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key change here: go test instead of operator-sdk test

alyacb commented Feb 3, 2021

View reviewed changes

alyacb force-pushed the CLOUDP-81150_switch_to_envtest branch from 561027e to a4ac714 Compare February 4, 2021 11:10

alyacb marked this pull request as ready for review February 4, 2021 11:42

alyacb requested a review from chatton February 4, 2021 11:42

alyacb removed the wip label Feb 4, 2021

alyacb force-pushed the CLOUDP-81150_switch_to_envtest branch from 4fd082d to 1429588 Compare February 4, 2021 16:35

chatton reviewed Feb 4, 2021

View reviewed changes

alyacb requested a review from chatton February 5, 2021 16:54

chatton approved these changes Feb 8, 2021

View reviewed changes

CLOUDP-81150: Replace operator-sdk test with envtest

630009e

alyacb force-pushed the CLOUDP-81150_switch_to_envtest branch from dc49ca3 to 630009e Compare February 8, 2021 10:58

alyacb merged commit c4b20bb into master Feb 8, 2021

alyacb deleted the CLOUDP-81150_switch_to_envtest branch February 8, 2021 12:08

CLOUDP-81150: Replace operator-sdk test with envtest #315

CLOUDP-81150: Replace operator-sdk test with envtest #315

Uh oh!

Conversation

alyacb commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bznein commented Feb 4, 2021

Uh oh!

alyacb commented Feb 4, 2021

Uh oh!

chatton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyacb commented Feb 5, 2021

Uh oh!

chatton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alyacb commented Feb 3, 2021 •

edited

Loading