✨ Add scaffolding for the webhook test suite (go/v3-alpha) #1710

prafull01 · 2020-10-06T14:06:15Z

Description
Added the webhook suite generation while creating the webhooks using the envtest.

Motivation:
The webhook envtest suite setup requires setting up quite a things which would be useful for users to be programmatically generated as the envtest has supported webhook testing.

Fixes #1707

k8s-ci-robot · 2020-10-06T14:06:23Z

Hi @prafull01. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

pkg/plugin/v3/api.go

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suite.go

pkg/plugin/v3/webhook.go

camilamacedo86 · 2020-10-06T15:23:02Z

/ok-to-test

prafull01 · 2020-10-06T16:01:51Z

/test pull-kubebuilder-e2e-k8s-1-15-3

pkg/plugin/v3/scaffolds/webhook.go

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suite.go

pkg/plugin/v3/webhook.go

camilamacedo86

Hi @prafull01,

Really thank you for your contribution. It shows OK for me 👍
/approve

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

ellistarn · 2020-10-20T22:41:06Z

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

+	var err error
+	cfg, err = testEnv.Start()
+	Expect(err).ToNot(HaveOccurred())
+	Expect(cfg).ToNot(BeNil())


Why would this be nil? Potentially unnecessary bloat.

The cfg is nill in the failure cases. See: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.6.3/pkg/envtest/server.go#L192

It is such as we have for the controllers. See: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go#L154-L157

Theoretically, the error will not be nil so we would not need to test the cfg. However, it might have a scenario that is not properly handled so, in POV we should not change it and keep it as it is done in the suite_test for the controller.

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

ellistarn · 2020-10-20T22:48:00Z

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

+	[]Reporter{printer.NewlineReporter{}})
+}
+
+var _ = BeforeSuite(func(done Done) {


I might use the form without done given that done isn't being used.

I agree with that. However, the same is applied to https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go#L146. So, I think it might better be addressed in a follow up for both scenarios.

For reference, many of my comments here came from my thoughts when I was "cleaning up" the kb generated code that's setup elsewhere. Of course, everyone is entitled to their own code style and opinions. I thought I'd raise the bits I found to be awkward. I agree with consistent style. If you agree that it's awkward, a followup to address all cases sounds good.

ellistarn · 2020-10-20T22:48:58Z

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

+		},
+	}
+
+	var err error


Rather that mutating the same error every time, it's probably more idiomatic to use the form

foo, err := Func()

1 for that 🥇
However, IMO it needs to be addressed in https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go#L154 as well. So, I think it would be better addressed in a follow up for both.

You are not mutating an error, you are re-assigning it. The current approach only allocates memory once while the other needs to allocate memory for every function so I don't think it is that good of an idea.

HI @Adirio,

The community suggestion was :
Replace

var err error cfg, err = testEnv.Start()

With

cfg, err := testEnv.Start()

In order to keep the simplicity in the code which shows totally fine for me.
However, the most important in my POV is to keep both suite tests for controller and webhook as closer as possible, so, in this way a new issue was tracked to address this nit in both : #1733

So, if you do not agree with the above change could you please add your inputs to #1733?

+1 for not mutating the same error every time.

The proposed changed is more idiomatic but doesn't change the behavior you mentioned. You are still using the same err variable and assigning different values to it. You are not actually mutating an error in any of the two cases. You just declare the variable the first time you use it instead of explicitly declaring the variable. So +1 for the change, but the explanation lead me to thinking the change you suggested was different.

ellistarn · 2020-10-20T22:49:53Z

overall a very useful example to help users get started testing webhooks.

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

ellistarn · 2020-10-21T16:59:12Z

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go

+	// wait for the webhook server to get ready
+	dialer := &net.Dialer{Timeout: time.Second}
+	addrPort := fmt.Sprintf("%s:%s", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
+	Eventually(func() error {


Suggestion:
Consider relying on https://godoc.org/k8s.io/apimachinery/pkg/util/wait#ExponentialBackoff. In my controller, i extracted all this setup logic into a more fully featured "envtest+manager" abstraction. It would be nice to have this logic less reliant on gomega and instead rely on apimachinery. If KB were to ever make an abstraction like this (to avoid this code in every suite_test), you wouldn't want that code to rely on gomega.

Really thank you for your input. I think that the best way for it be addressed would be might via a follow-up PR. So, WDYT about after it gets merged you push a PR with your suggestion? Then, it might clarify better as well as your thoughts.

pkg/plugin/v3/webhook.go

pkg/plugin/v3/api.go

pkg/plugin/v3/scaffolds/webhook.go

camilamacedo86 · 2020-10-27T15:30:06Z

After spoke with @estroz, it might be better we use (go/v3-alpha) instead of (v3+ only).
Could you please update the title/commit

Adirio

/lgtm

k8s-ci-robot · 2020-10-27T16:02:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86, prafull01

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

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

camilamacedo86 · 2020-10-27T20:21:10Z

It was rebased with the master because of this loses the @Adirio lgtm
Doing that again 👍

/lgtm

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 6, 2020

k8s-ci-robot requested review from camilamacedo86 and mengqiy October 6, 2020 14:06

prafull01 force-pushed the webhook-test branch from aee67b1 to 1b7353e Compare October 6, 2020 14:08

camilamacedo86 reviewed Oct 6, 2020

View reviewed changes

pkg/plugin/v3/api.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Oct 6, 2020

View reviewed changes

pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suite.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Oct 6, 2020

View reviewed changes

pkg/plugin/v3/webhook.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2020

prafull01 force-pushed the webhook-test branch from 1b7353e to 880f74a Compare October 6, 2020 15:41

camilamacedo86 reviewed Oct 6, 2020

View reviewed changes

pkg/plugin/v3/scaffolds/webhook.go Show resolved Hide resolved

prafull01 requested a review from camilamacedo86 October 10, 2020 10:58

camilamacedo86 changed the title ~~✨ Add scaffolding for the webhook test suite~~ ✨ Add scaffolding for the webhook test suite (v3+ only) Oct 16, 2020

camilamacedo86 reviewed Oct 16, 2020

View reviewed changes