Use Genreconciler for KnativeServing #380

trshafer · 2020-03-27T21:14:54Z

Fixes #335

Proposed Changes

Change the reconciler to the generated reconciler

Release Note

NONE

knative-prow-robot

@trshafer: 0 warnings.

In response to this:

Addresses #335

Proposed Changes

Change the reconciler to the generated reconciler

Release Note
NONE

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.

Cynocracy · 2020-03-27T21:24:42Z

pkg/reconciler/knativeserving/knativeserving_controller.go

 	if err != nil {
 		r.Logger.Errorf("invalid resource key: %s", key)
 		return nil
 	}
-	// Get the KnativeServing resource with this namespace/name.
-	original, err := r.knativeServingLister.KnativeServings(namespace).Get(name)


I see build failures because of this line, are the tests stale?

pkg/reconciler/knativeserving/knativeserving_controller.go:256:21: r.knativeServingLister undefined (type *Reconciler has no field or method knativeServingLister) specifically, which maybe is somewhere else I'm missing in the file :)

Cynocracy · 2020-03-30T22:23:26Z

pkg/client/injection/reconciler/serving/v1alpha1/knativeserving/reconciler.go

+limitations under the License.
+*/
+
+// Code generated by injection-gen. DO NOT EDIT.


Any particular reason this isn't in a zz_generated.go file? Is that not standard practice?

I'm not sure. It's the same for knative/serving. Maybe the comment is sufficient? I still find it confusing to understand what line of code actually produced this file. https://github.com/knative/serving/blob/fdfc49f209d6114295968d9d4c7ad8392542a9c0/pkg/client/injection/reconciler/serving/v1/service/reconciler.go.

Cynocracy

A couple of questions about places where I'm confused :)

The tests are passing though, so depending on what I learn this looks reasonable.

Cynocracy · 2020-03-30T22:24:05Z

pkg/client/injection/reconciler/serving/v1alpha1/knativeserving/stub/controller.go

+	v1alpha1knativeserving "knative.dev/serving-operator/pkg/client/injection/reconciler/serving/v1alpha1/knativeserving"
+)
+
+// TODO: PLEASE COPY AND MODIFY THIS FILE AS A STARTING POINT


Delete

Also, is the code generated comment still relevant? If so, we might want to remove this from the generated code :)

In serving, the content under stub is autogenerated and checked in: https://github.com/knative/serving/tree/fdfc49f209d6114295968d9d4c7ad8392542a9c0/pkg/client/injection/reconciler/serving/v1/service/stub

Cynocracy · 2020-03-30T22:24:31Z

pkg/client/injection/reconciler/serving/v1alpha1/knativeserving/stub/controller.go

+
+	knativeservingInformer := knativeserving.Get(ctx)
+
+	// TODO: setup additional informers here.


Do we need more informers on the underlying deployments, for instance?

I am not sure if this line is generated, since this file is under pkg/client/injection?

The content under stub is generated: https://github.com/knative/serving/tree/fdfc49f209d6114295968d9d4c7ad8392542a9c0/pkg/client/injection/reconciler/serving/v1/service/stub

houshengbo · 2020-03-31T13:51:27Z

@trshafer @Cynocracy I have submitted this PR to move over all the serving-operator code: knative/operator#1
I am thinking of whether we FREEZE the operators' repo, to make sure all the code land in there.
After two operators embrace each other in one repo, with correct functionalities, we can move forward with the new repo.
Thoughts?

trshafer · 2020-03-31T17:31:20Z

@houshengbo genreconciler should make it easier to transition from one operator to another as more of the boilerplate code is autogenerated. I think we should finish #335. Thoughts?

Cynocracy · 2020-03-31T19:59:58Z

My opinion is that we should land this and a similar change in eventing to minimize the boilerplate / wiring needed to combine the operators. We have this working for serving, maybe we could prepare a similar change for eventing before we merge this?

trshafer · 2020-04-03T19:50:57Z

@houshengbo thoughts on merging this change? The next change will be to remove rbase:

serving-operator/pkg/reconciler/knativeserving/controller.go

Line 57 in d0e771a

Base: rbase.NewBase(ctx, controllerAgentName, cmw),

houshengbo · 2020-04-08T21:27:12Z

/close
We stopped contributing to this repo, as our new repo for knative operator has set up: knative-sandbox/operator.

knative-prow-robot · 2020-04-08T21:27:15Z

@houshengbo: Closed this PR.

In response to this:

/close
We stopped contributing to this repo, as our new repo for knative operator has set up: knative-sandbox/operator.

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.

houshengbo · 2020-04-09T18:14:46Z

/retest

trshafer · 2020-04-09T18:27:30Z

/hold

Will update with removing rbase.

trshafer · 2020-04-09T18:42:58Z

/cancel hold

trshafer · 2020-04-09T18:43:06Z

/hold cancel

lgtm-com · 2020-04-09T18:45:24Z

This pull request introduces 1 alert and fixes 1 when merging b8f656b into 940a4ca - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

fixed alerts:

1 for Useless assignment to local variable

Delete base reconciler.

knative-metrics-robot · 2020-04-09T18:49:30Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/knativeserving_lifecycle.go	57.9%	52.9%	-5.0

lgtm-com · 2020-04-09T18:54:08Z

This pull request fixes 1 alert when merging 28b217e into 940a4ca - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

houshengbo · 2020-04-09T20:07:46Z

This PR is equivalent to the corresponding changes we had in knative-sandbox/operator, in terms of refactoring the reconciler.
/lgtm
/approve

knative-prow-robot · 2020-04-09T20:08:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, trshafer

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~~ [houshengbo,trshafer]

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

googlebot added the cla: yes Author(s) signed a CLA. label Mar 27, 2020

knative-prow-robot reviewed Mar 27, 2020

View reviewed changes

knative-prow-robot requested review from Cynocracy and jcrossley3 March 27, 2020 21:15

knative-prow-robot added size/XL approved labels Mar 27, 2020

Cynocracy reviewed Mar 27, 2020

View reviewed changes

trshafer changed the title ~~Genreconciler~~ [WIP] Genreconciler Mar 27, 2020

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

trshafer force-pushed the genreconciler branch 2 times, most recently from a73a2cf to 2dc4a44 Compare March 30, 2020 21:13

trshafer changed the title ~~[WIP] Genreconciler~~ Genreconciler Mar 30, 2020

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

trshafer changed the title ~~Genreconciler~~ Use Genreconciler for KnativeServing Mar 30, 2020

trshafer force-pushed the genreconciler branch from 2dc4a44 to 94dc513 Compare March 30, 2020 21:34

knative-prow-robot added size/XXL and removed size/XL labels Mar 30, 2020

trshafer force-pushed the genreconciler branch from 94dc513 to cbf9470 Compare March 30, 2020 21:38

Cynocracy reviewed Mar 30, 2020

View reviewed changes

knative-prow-robot closed this Apr 8, 2020

houshengbo reopened this Apr 9, 2020

knative-prow-robot added the do-not-merge/hold label Apr 9, 2020

Generate reconciler for knativeserving

4395c59

trshafer force-pushed the genreconciler branch 2 times, most recently from 3a3f836 to b8f656b Compare April 9, 2020 18:38

knative-prow-robot removed the do-not-merge/hold label Apr 9, 2020

Replace reconciler with genreconciler.

28b217e

Delete base reconciler.

trshafer force-pushed the genreconciler branch from b8f656b to 28b217e Compare April 9, 2020 18:47

knative-prow-robot assigned houshengbo Apr 9, 2020

knative-prow-robot added the lgtm label Apr 9, 2020

knative-prow-robot merged commit 7624ab5 into knative:master Apr 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Genreconciler for KnativeServing #380

Use Genreconciler for KnativeServing #380

trshafer commented Mar 27, 2020 •

edited

Loading

knative-prow-robot left a comment

Cynocracy Mar 27, 2020

Cynocracy Mar 27, 2020

Cynocracy Mar 30, 2020

trshafer Apr 3, 2020

Cynocracy left a comment

Cynocracy Mar 30, 2020

trshafer Apr 3, 2020

Cynocracy Mar 30, 2020

houshengbo Mar 31, 2020

trshafer Apr 3, 2020

houshengbo commented Mar 31, 2020 •

edited

Loading

trshafer commented Mar 31, 2020

Cynocracy commented Mar 31, 2020

trshafer commented Apr 3, 2020

houshengbo commented Apr 8, 2020

knative-prow-robot commented Apr 8, 2020

houshengbo commented Apr 9, 2020

trshafer commented Apr 9, 2020

trshafer commented Apr 9, 2020

trshafer commented Apr 9, 2020

lgtm-com bot commented Apr 9, 2020

knative-metrics-robot commented Apr 9, 2020

lgtm-com bot commented Apr 9, 2020

houshengbo commented Apr 9, 2020

knative-prow-robot commented Apr 9, 2020


		knativeservingInformer := knativeserving.Get(ctx)

		// TODO: setup additional informers here.

Use Genreconciler for KnativeServing #380

Use Genreconciler for KnativeServing #380

Conversation

trshafer commented Mar 27, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

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

Cynocracy left a 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

houshengbo commented Mar 31, 2020 • edited Loading

trshafer commented Mar 31, 2020

Cynocracy commented Mar 31, 2020

trshafer commented Apr 3, 2020

houshengbo commented Apr 8, 2020

knative-prow-robot commented Apr 8, 2020

houshengbo commented Apr 9, 2020

trshafer commented Apr 9, 2020

trshafer commented Apr 9, 2020

trshafer commented Apr 9, 2020

lgtm-com bot commented Apr 9, 2020

knative-metrics-robot commented Apr 9, 2020

lgtm-com bot commented Apr 9, 2020

houshengbo commented Apr 9, 2020

knative-prow-robot commented Apr 9, 2020

trshafer commented Mar 27, 2020 •

edited

Loading

houshengbo commented Mar 31, 2020 •

edited

Loading