Add gatekeeper testing #384

marzianor9 · 2020-12-08T12:55:34Z

No description provided.

openshift-ci-robot · 2020-12-08T12:55:43Z

Hi @marzianor9. Thanks for your PR.

I'm waiting for a openshift-kni member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

functests/utils/dynamic/dynamic.go

functests/gatekeeper/gatekeeper.go

fedepaol

Looks like a good start!
Added a few comments

functests/gatekeeper/resources.go

functests/gatekeeper/gatekeeper.go

SchSeba · 2021-01-05T12:10:53Z

functests/gatekeeper/gatekeeper.go

+
+	// create the dynamic client
+	client, err := dynamicInterface.New("", gvrList...)
+	if err != nil {


lets put this in a BeforeAll and use Expect so we failed the test

i can't create a variable that will be used in execute.BeforeAll, i will need to make it global, should i do it?

SchSeba · 2021-01-05T12:42:49Z

functests/gatekeeper/gatekeeper.go

+		log.Fatalf("Unable to create the dynamic client: %v", err)
+	}
+
+	DescribeTable("should be able to add metadata info(labels/annotations)",


I think the should is not part of a DescribeTable can you rephrase this please

SchSeba · 2021-01-05T12:46:10Z

functests/gatekeeper/gatekeeper.go

+	return entries
+}
+
+func assignMetadataDefinition(amInfo *assignMetadataInfo) *unstructured.Unstructured {


why you use an unstructured.Unstructured object when you always use the AssignMetadata kind?

can't you just use the API struct from the gatekeeper project?

we created the dynamic client because gatekeeper didn't have the API struct we needed in the repo, and we were not sure if they want to add them.

functests/gatekeeper/gatekeeper.go

SchSeba · 2021-01-05T12:48:56Z

functests/gatekeeper/gatekeeper.go

+		func(amInfo assignMetadataInfo, object TestObject) {
+			amObject := assignMetadataDefinition(&amInfo)
+
+			amObject, err := client.Resource("assignmetadata").Create(context.Background(), amObject, metav1.CreateOptions{})


for this reasource why we don't add the schema into a client just like all the other tests?

i didn't want to use 2 clients in the same test suite, and the dynamic client allows for more generic tests.

and i was unable to add the schema for this object to the client

SchSeba · 2021-01-05T13:15:04Z

functests/gatekeeper/gatekeeper.go

+		}()
+
+		By("Creating test-pod-b")
+		testPodB := GetTestPod(TestingNamespace)


I think we already have a create pod function no?

i needed a customized pod for the tests, also i needed it to be unstructured so that tests can be generic.

SchSeba · 2021-01-05T13:19:51Z

functests/gatekeeper/gatekeeper.go

+		Expect(value).To(Equal("0"))
+
+		By("Updating assignMetadata to mutation-version: 1")
+		err = unstructured.SetNestedField(am.Object, "1", "spec", "parameters", "assign", "value")


you know the object type you are working with here so lets use the object and not the unstructured

the gatekeeper repo does not support this.

SchSeba · 2021-01-05T13:20:45Z

functests/gatekeeper/gatekeeper.go

+		Expect(err).ToNot(HaveOccurred())
+		Expect(ok).To(BeFalse())
+
+		By("Deleting test-pod-after-delete")


this is not part of the test should be in the afterEach clean

SchSeba · 2021-01-05T13:25:09Z

functests/gatekeeper/gatekeeper.go

+
+	It("should be able to match by any match category", func() {
+		By("Creating the test namespaces")
+		includedNamepsace := &unstructured.Unstructured{


this is a namespace object just use the namespace schema

do we want to use 2 clients?

functests/gatekeeper/gatekeeper.go

mmirecki · 2021-01-12T09:41:17Z

functests/gatekeeper/gatekeeper.go

+			defer func() {
+				err := client.Resource("assignmetadata").Delete(context.Background(), amDefinition.GetName(), metav1.DeleteOptions{})
+				Expect(err).ToNot(HaveOccurred())
+			}()


We have quite a lot of these defers, so maybe we can do:

func deleteResource(resource string, name string) func() { return func() { err := client.Resource(resource).Delete(context.Background(), name, metav1.DeleteOptions{}) Expect(err).ToNot(HaveOccurred()) } }

and then use it like:
defer deleteResource("assignmetadata", amDefinition.GetName())()()

With 19 defer statements we should save more then 50 lines of code.

functests/gatekeeper/gatekeeper.go

mmirecki · 2021-01-12T11:29:18Z

functests/gatekeeper/gatekeeper.go

+		defer func() {
+			err := client.Resource("assignmetadata").Delete(context.Background(), namespaceSelected.GetName(), metav1.DeleteOptions{})
+			Expect(err).NotTo(HaveOccurred())
+		}()


It should be more readable if we defined the Assigns in one place and then created them in a loop. Something like:

allScopeSelected = {...} namespaceScopeIncluded = {...} namespaceScopeIncluded= {...} ... mutations = { allScopeSelected, namespaceScopeIncluded, namespaceScopeIncluded} for mutation ;= range mutatinos { client.Resource("assignmetadata").Create(context.Background(), namespaceSelected, metav1.CreateOptions{}) defer func(){} }

functests/gatekeeper/gatekeeper.go

oribon

should be more readable not assigning value if it is not used

mmirecki · 2021-01-19T10:52:53Z

functests/gatekeeper/gatekeeper.go

+				Expect(err).ToNot(HaveOccurred())
+				namespaces.WaitForDeletion(client, namespace, time.Minute)
+			}
+		}


If you care about test time, then maybe move the WaitForDeletion after all namespaces are deleted:

for _, namespace := range namespacesUsed { if namespaces.Exists(namespace, client) { err := client.Namespaces().Delete(context.Background(), namespace, metav1.DeleteOptions{}) Expect(err).ToNot(HaveOccurred()) } } for _, namespace := range namespacesUsed { namespaces.WaitForDeletion(client, namespace, time.Minute) }

the namespaces i delete are used only on the last test, all other times we only delete the pods from the TestingNamespace.

If time is not of essence we can skip this

mmirecki · 2021-01-19T12:28:13Z

/lgtm

mmirecki · 2021-01-20T13:32:49Z

functests/gatekeeper/gatekeeper.go

+		}
+
+		for _, namespace := range namespacesUsed {
+			if namespaces.Exists(namespace, client) {


We can skip if namespaces.Exists(namespace, client), this is done in WaitForDeletion

mmirecki · 2021-01-20T14:26:57Z

/lgtm

fedepaol · 2021-01-21T13:54:10Z

/lgtm
/approve

openshift-ci-robot · 2021-01-21T13:54:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fedepaol, marzianor9

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~~ [fedepaol]

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

openshift-ci-robot requested review from slintes and vladikr December 8, 2020 12:55

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 8, 2020

marzianor9 marked this pull request as draft December 8, 2020 12:56

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2020

mmirecki reviewed Dec 8, 2020

View reviewed changes

fedepaol reviewed Dec 9, 2020

View reviewed changes

functests/gatekeeper/resources.go Outdated Show resolved Hide resolved

functests/gatekeeper/gatekeeper.go Outdated Show resolved Hide resolved

marzianor9 marked this pull request as ready for review December 23, 2020 11:49

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2020

marzianor9 requested review from fedepaol and mmirecki December 23, 2020 11:49

SchSeba reviewed Jan 5, 2021

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2021

marzianor9 changed the title ~~Add gatekeeper testing and a dynamic client~~ Add gatekeeper testing Jan 10, 2021

mmirecki reviewed Jan 12, 2021

View reviewed changes

functests/gatekeeper/gatekeeper.go Outdated Show resolved Hide resolved

mmirecki reviewed Jan 12, 2021

View reviewed changes

oribon reviewed Jan 13, 2021

View reviewed changes

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2021

mmirecki reviewed Jan 19, 2021

View reviewed changes

openshift-ci-robot assigned mmirecki Jan 19, 2021

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 19, 2021

mmirecki reviewed Jan 20, 2021

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 20, 2021

Add gatekeeper mutation testing

46aaa2c

openshift-ci-robot assigned fedepaol Jan 21, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2021

openshift-merge-robot merged commit d7a0b95 into openshift-kni:master Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gatekeeper testing #384

Add gatekeeper testing #384

marzianor9 commented Dec 8, 2020

openshift-ci-robot commented Dec 8, 2020

fedepaol left a comment

SchSeba Jan 5, 2021

marzianor9 Jan 6, 2021 •

edited

SchSeba Jan 5, 2021

SchSeba Jan 5, 2021

marzianor9 Jan 6, 2021

SchSeba Jan 5, 2021

marzianor9 Jan 6, 2021

marzianor9 Jan 6, 2021

SchSeba Jan 5, 2021

marzianor9 Jan 6, 2021

SchSeba Jan 5, 2021

marzianor9 Jan 6, 2021

SchSeba Jan 5, 2021

SchSeba Jan 5, 2021

marzianor9 Jan 6, 2021

mmirecki Jan 12, 2021

mmirecki Jan 12, 2021

oribon left a comment

mmirecki Jan 19, 2021 •

edited

marzianor9 Jan 19, 2021

mmirecki Jan 19, 2021

marzianor9 Jan 20, 2021

mmirecki commented Jan 19, 2021

mmirecki Jan 20, 2021

mmirecki commented Jan 20, 2021

fedepaol commented Jan 21, 2021

openshift-ci-robot commented Jan 21, 2021

Add gatekeeper testing #384

Add gatekeeper testing #384

Conversation

marzianor9 commented Dec 8, 2020

openshift-ci-robot commented Dec 8, 2020

fedepaol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marzianor9 Jan 6, 2021 • edited

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

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

oribon left a comment

Choose a reason for hiding this comment

mmirecki Jan 19, 2021 • edited

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

mmirecki commented Jan 19, 2021

Choose a reason for hiding this comment

mmirecki commented Jan 20, 2021

fedepaol commented Jan 21, 2021

openshift-ci-robot commented Jan 21, 2021

marzianor9 Jan 6, 2021 •

edited

mmirecki Jan 19, 2021 •

edited