[WIP] 🌱 Enable webhook envtests #5722

sbueringer · 2021-11-23T14:41:38Z

What this PR does / why we need it:

WIP => currently mostly a PoC to discuss if we want to address the issue this way.

DOES NOT WORK RIGHT NOW

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5721

Signed-off-by: Stefan Büringer [email protected]

k8s-ci-robot · 2021-11-23T14:41:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cecilerobertmichon after the PR has been reviewed.
You can assign the PR to them by writing /assign @cecilerobertmichon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

sbueringer · 2021-11-23T15:33:53Z

api/v1beta1/webhooks.go

+)
+
+// SetupWebhooksWithManager sets up all webhooks of the current package.
+func SetupWebhooksWithManager(mgr ctrl.Manager) error {


We don't necessarily need the package level functions to avoid the circular dependencies, but they would avoid some repetition (otherwise we would have to inline those funcs, aka we would need the full list of resources in all those cases)

sbueringer · 2021-11-23T15:36:44Z

bootstrap/kubeadm/main.go

@@ -216,18 +216,10 @@ func setupWebhooks(mgr ctrl.Manager) {
 		setupLog.Error(err, "unable to create webhook", "webhook", "KubeadmConfig")
 		os.Exit(1)
 	}


We don't have to register those webhooks (we don't register any other List webhooks)

sbueringer · 2021-11-23T15:37:39Z

internal/envtest/environment.go

@@ -69,6 +68,9 @@ func init() {
 	klog.SetOutput(ginkgo.GinkgoWriter)

 	// Calculate the scheme.
+	// FIXME(tbd): if we want to enable envtests in those packages too we


sbueringer · 2021-11-23T15:38:27Z

internal/envtest/setup/webhook.go

+)
+
+// AllWebhooksWithManager sets up all webhooks.
+func AllWebhooksWithManager(mgr ctrl.Manager) {


In theory we also don't need this helper to avoid circular dependencies, but the alternative would be to inline it and have the list of webhooks redundantly

sbueringer · 2021-11-23T15:38:49Z

internal/webhooks/suite_test.go

+	ctx = ctrl.SetupSignalHandler()
+)
+
+// FIXME(sbueringer): just part of this PR to prove that it works.


Just to prove the concept works, should be removed before merge

sbueringer · 2021-11-23T15:39:42Z

webhooks/webhooks.go

-	return ctrl.NewWebhookManagedBy(mgr).
-		For(r).
-		Complete()
+// SetupWebhooksWithManager sets up all webhooks of the current package.


Note: that github diff is really weird (it's just a new file)

k8s-ci-robot · 2021-11-23T16:05:30Z

@sbueringer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`3cff8d2`	link	false	`/test pull-cluster-api-apidiff-main`

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.

sbueringer · 2021-11-23T16:28:44Z

@killianmuldoon @fabriziopandini This PR is meant as a starting point for the discussion on how the solve the issue.

tl;dr is

we can't just register all webhooks in the envtest package because then envtest won't be usable in webhook envtests
- => so we have to move them out of there
naive solution would be to just register all webhooks in the suite_test.go files.
- => To avoid the redundancy I added two layers of helper, which both are not strictly necessary to avoid the circular dependency but avoid a lot of repetition.
  - Tests which require all webhooks can just use the setup func
  - webhook envtests have to individually register the required webhooks (because they cannot use the helper func because this would introduce a circular dependency)

P.S. apidiff would be fixed via deprecation if we want

sbueringer · 2021-11-23T18:23:29Z

I've discussed the issue with Fabrizio and we would simply implement the webhooks integration test in a package like internal/webhooks/integration instead of refactoring envtest like proposed in this PR.

The problem with this PR is that it distributes the webhook setup all over the place vs. currently webhooks will just work out of the box when using envtest.

Let's continue the discussion on the issue

sbueringer · 2021-11-23T18:23:49Z

/close

k8s-ci-robot · 2021-11-23T18:24:01Z

@sbueringer: Closed this PR.

In response to this:

/close

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.

sbueringer added 2 commits November 23, 2021 15:29

Initial trial

4e03710

Signed-off-by: Stefan Büringer [email protected]

second try

8fc9721

Signed-off-by: Stefan Büringer [email protected]

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 23, 2021

k8s-ci-robot requested review from JoelSpeed and stmcginnis November 23, 2021 14:41

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 23, 2021

sbueringer commented Nov 23, 2021

View reviewed changes

sbueringer force-pushed the pr-enable-webhook-envtests branch from dd48432 to 7bec4f2 Compare November 23, 2021 15:35

sbueringer commented Nov 23, 2021

View reviewed changes

sbueringer force-pushed the pr-enable-webhook-envtests branch from 7bec4f2 to 8600de1 Compare November 23, 2021 15:43

should work again (hopefully)

3cff8d2

sbueringer force-pushed the pr-enable-webhook-envtests branch from 8600de1 to 3cff8d2 Compare November 23, 2021 16:04

k8s-ci-robot closed this Nov 23, 2021

sbueringer mentioned this pull request Nov 23, 2021

Enable envtest-based unit tests for webhooks #5721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] 🌱 Enable webhook envtests #5722

[WIP] 🌱 Enable webhook envtests #5722

sbueringer commented Nov 23, 2021 •

edited

Loading

k8s-ci-robot commented Nov 23, 2021

sbueringer Nov 23, 2021 •

edited

Loading

sbueringer Nov 23, 2021

sbueringer Nov 23, 2021

sbueringer Nov 23, 2021

sbueringer Nov 23, 2021

sbueringer Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021 •

edited

Loading

sbueringer commented Nov 23, 2021

sbueringer commented Nov 23, 2021 •

edited

Loading

sbueringer commented Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021

[WIP] 🌱 Enable webhook envtests #5722

[WIP] 🌱 Enable webhook envtests #5722

Conversation

sbueringer commented Nov 23, 2021 • edited Loading

k8s-ci-robot commented Nov 23, 2021

sbueringer Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

sbueringer Nov 23, 2021

Choose a reason for hiding this comment

sbueringer Nov 23, 2021

Choose a reason for hiding this comment

sbueringer Nov 23, 2021

Choose a reason for hiding this comment

sbueringer Nov 23, 2021

Choose a reason for hiding this comment

sbueringer Nov 23, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 23, 2021 • edited Loading

sbueringer commented Nov 23, 2021

sbueringer commented Nov 23, 2021 • edited Loading

sbueringer commented Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021

sbueringer commented Nov 23, 2021 •

edited

Loading

sbueringer Nov 23, 2021 •

edited

Loading

k8s-ci-robot commented Nov 23, 2021 •

edited

Loading

sbueringer commented Nov 23, 2021 •

edited

Loading