iframe-proxy

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.

Details

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

 		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

+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

+	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

+
+	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.

Details

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

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


		knativeservingInformer := knativeserving.Get(ctx)

		// TODO: setup additional informers here.

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

trshafer commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cynocracy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houshengbo commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trshafer commented Mar 31, 2020

Uh oh!

Cynocracy commented Mar 31, 2020

Uh oh!

trshafer commented Apr 3, 2020

Uh oh!

houshengbo commented Apr 8, 2020

Uh oh!

knative-prow-robot commented Apr 8, 2020

Uh oh!

houshengbo commented Apr 9, 2020

Uh oh!

trshafer commented Apr 9, 2020

Uh oh!

trshafer commented Apr 9, 2020

Uh oh!

trshafer commented Apr 9, 2020

Uh oh!

lgtm-com Bot commented Apr 9, 2020

Uh oh!

knative-metrics-robot commented Apr 9, 2020

Uh oh!

lgtm-com Bot commented Apr 9, 2020

Uh oh!

houshengbo commented Apr 9, 2020

Uh oh!

knative-prow-robot commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

trshafer commented Mar 27, 2020 •

edited

Loading

houshengbo commented Mar 31, 2020 •

edited

Loading