Use wire-gen injection for all controllers #1733

zhongduo · 2020-09-17T20:33:28Z

Fixes #

Currently some controllers are not using wire-gen. By changing all controller to the same style we can achieve:

consistent coding style for all controllers
easy to support new singleton configuration maps
use same singleton object from the top level.

This PR will be rebased after merge of #1681

Proposed Changes

Use wire-gen to generate all controllers.

zhongduo · 2020-09-21T13:34:03Z

/assign @grantr

zhongduo · 2020-09-21T13:34:18Z

/assign @ian-mi

Harwayne · 2020-09-21T16:42:29Z

pkg/reconciler/trigger/controller.go

+	}
+}
+
+func newController(ctx context.Context, cmw configmap.Watcher, dataresidencyss *dataresidency.Store) *controller.Impl {


Suggested change

func newController(ctx context.Context, cmw configmap.Watcher, dataresidencyss *dataresidency.Store) *controller.Impl {

func newController(ctx context.Context, cmw configmap.Watcher, drs *dataresidency.Store) *controller.Impl {

Harwayne · 2020-09-21T16:42:50Z

pkg/reconciler/broker/controller.go

+	}
+}
+
+func newController(ctx context.Context, cmw configmap.Watcher, dataresidencyss *dataresidency.Store) *controller.Impl {


Suggested change

func newController(ctx context.Context, cmw configmap.Watcher, dataresidencyss *dataresidency.Store) *controller.Impl {

func newController(ctx context.Context, cmw configmap.Watcher, drs *dataresidency.Store) *controller.Impl {

Good catch, I was planning to change the name last Friday but somehow it skipped my mind. Thanks.

Harwayne · 2020-09-21T16:44:04Z

pkg/reconciler/trigger/controller_test.go

@@ -44,7 +44,11 @@ import (
 func TestNew(t *testing.T) {
 	ctx, _ := SetupFakeContext(t)

-	c := NewController(ctx, configmap.NewStaticWatcher(
+	dataresidencySs := &dataresidency.StoreSingleton{}


Inline this, as we don't use it again.

Fixed as suggested.

Harwayne · 2020-09-21T16:44:36Z

pkg/reconciler/broker/controller_test.go

@@ -38,7 +38,11 @@ import (
 func TestNew(t *testing.T) {
 	ctx, _ := SetupFakeContext(t)

-	c := NewController(ctx, configmap.NewStaticWatcher(
+	dataresidencySs := &dataresidency.StoreSingleton{}


Inline this, as we don't use it again.

Fixed as suggested.

ian-mi · 2020-09-21T23:17:39Z

pkg/reconciler/broker/controller_test.go

@@ -38,7 +38,9 @@ import (
 func TestNew(t *testing.T) {
 	ctx, _ := SetupFakeContext(t)

-	c := NewController(ctx, configmap.NewStaticWatcher(
+	ctor := NewConstructor(&dataresidency.StoreSingleton{})


I would inline the constructor as well

Fixed as suggested.

ian-mi · 2020-09-21T23:17:51Z

pkg/reconciler/brokercell/controller_test.go

@@ -48,7 +48,9 @@ func TestNew(t *testing.T) {

 	setReconcilerEnv()

-	c := NewController(ctx, configmap.NewStaticWatcher(
+	ctor := NewConstructor()


Fixed as suggested.

ian-mi · 2020-09-21T23:18:21Z

pkg/reconciler/deployment/controller_test.go

@@ -31,7 +31,9 @@ import (
 func TestNew(t *testing.T) {
 	ctx, _ := SetupFakeContext(t)

-	c := NewController(ctx, configmap.NewStaticWatcher())
+	ctor := NewConstructor()


Fixed as suggested.

ian-mi · 2020-09-21T23:18:42Z

pkg/reconciler/trigger/controller_test.go

@@ -44,7 +44,9 @@ import (
 func TestNew(t *testing.T) {
 	ctx, _ := SetupFakeContext(t)

-	c := NewController(ctx, configmap.NewStaticWatcher(
+	ctor := NewConstructor(&dataresidency.StoreSingleton{})


Fixed as suggested.

…egions in dataresidency configmap

knative-metrics-robot · 2020-09-22T13:31:35Z

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/broker/controller.go	60.6%	67.6%	7.0
pkg/reconciler/brokercell/controller.go	54.3%	56.8%	2.5
pkg/reconciler/deployment/controller.go	81.8%	84.6%	2.8
pkg/reconciler/trigger/controller.go	69.2%	70.0%	0.8

ian-mi · 2020-09-22T16:29:38Z

/lgtm
/approve

knative-prow-robot · 2020-09-22T16:30:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ian-mi, zhongduo

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~~ [ian-mi]

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

zhongduo · 2020-09-22T18:42:54Z

/retest

knative-test-reporter-robot · 2020-09-22T19:45:57Z

The following jobs failed:

Test name	Triggers	Retries
pull-google-knative-gcp-integration-tests	2020-09-22 17:32:25.058 +0000 UTC	1/3
pull-google-knative-gcp-wi-tests	2020-09-22 13:39:54.82 +0000 UTC 2020-09-22 17:38:24.902 +0000 UTC 2020-09-22 19:31:24.923 +0000 UTC 2020-09-22 19:45:55.037 +0000 UTC	3/3

Job pull-google-knative-gcp-wi-tests expended all 3 retries without success.

zhongduo · 2020-09-22T19:46:51Z

/retest

knative-prow-robot added the do-not-merge/work-in-progress label Sep 17, 2020

knative-prow-robot requested review from Harwayne and nachocano September 17, 2020 20:33

knative-prow-robot added the size/XL label Sep 17, 2020

google-cla bot added the cla: yes (override cla status due to multiple authors bug) label Sep 17, 2020

knative-prow-robot added the needs-rebase label Sep 20, 2020

Jimmy Lin added 2 commits September 21, 2020 09:29

Use wire injection for trigger and broker controller

a19699b

Use wiregen for brokercell and deployment controller

335ecab

zhongduo force-pushed the use-injection branch from afff56b to 335ecab Compare September 21, 2020 13:30

knative-prow-robot added size/M and removed needs-rebase size/XL labels Sep 21, 2020

zhongduo changed the title ~~WIP: Use wire-gen injection for all controllers~~ Use wire-gen injection for all controllers Sep 21, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Sep 21, 2020

knative-prow-robot assigned grantr Sep 21, 2020

knative-prow-robot assigned ian-mi Sep 21, 2020

Use NewConstructor in controller test to increase code coverage

8b417f4

knative-prow-robot added size/L and removed size/M labels Sep 21, 2020

Harwayne reviewed Sep 21, 2020

View reviewed changes

Clean up code

9715d89

knative-prow-robot added size/M and removed size/L labels Sep 21, 2020

zhongduo mentioned this pull request Sep 21, 2020

Add data residency validation to webhook #1747

Merged

ian-mi suggested changes Sep 21, 2020

View reviewed changes

Inline constructor to broker and trigger controller test, use empty r…

cbb6a60

…egions in dataresidency configmap

knative-prow-robot added size/L and removed size/M labels Sep 22, 2020

Use inline constructor in deployment and brokercell controller test

4558b82

knative-prow-robot added lgtm approved labels Sep 22, 2020

ian-mi approved these changes Sep 22, 2020

View reviewed changes

knative-prow-robot merged commit b305538 into google:master Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use wire-gen injection for all controllers #1733

Use wire-gen injection for all controllers #1733

zhongduo commented Sep 17, 2020

zhongduo commented Sep 21, 2020

zhongduo commented Sep 21, 2020

Harwayne Sep 21, 2020

Harwayne Sep 21, 2020

zhongduo Sep 21, 2020

Harwayne Sep 21, 2020

zhongduo Sep 21, 2020

Harwayne Sep 21, 2020

zhongduo Sep 21, 2020

ian-mi Sep 21, 2020

zhongduo Sep 22, 2020

ian-mi Sep 21, 2020

zhongduo Sep 22, 2020

ian-mi Sep 21, 2020

zhongduo Sep 22, 2020

ian-mi Sep 21, 2020

zhongduo Sep 22, 2020

knative-metrics-robot commented Sep 22, 2020

ian-mi commented Sep 22, 2020

knative-prow-robot commented Sep 22, 2020

zhongduo commented Sep 22, 2020

knative-test-reporter-robot commented Sep 22, 2020

zhongduo commented Sep 22, 2020

	func newController(ctx context.Context, cmw configmap.Watcher, dataresidencyss dataresidency.Store) controller.Impl {
	func newController(ctx context.Context, cmw configmap.Watcher, drs dataresidency.Store) controller.Impl {

Use wire-gen injection for all controllers #1733

Use wire-gen injection for all controllers #1733

Conversation

zhongduo commented Sep 17, 2020

Proposed Changes

zhongduo commented Sep 21, 2020

zhongduo commented Sep 21, 2020

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

knative-metrics-robot commented Sep 22, 2020

ian-mi commented Sep 22, 2020

knative-prow-robot commented Sep 22, 2020

zhongduo commented Sep 22, 2020

knative-test-reporter-robot commented Sep 22, 2020

zhongduo commented Sep 22, 2020