Skip to content

Restructure the base/controller split.#1198

Merged
google-prow-robot merged 1 commit into
knative:masterfrom
mattmoor:informer-syncing
Jun 14, 2018
Merged

Restructure the base/controller split.#1198
google-prow-robot merged 1 commit into
knative:masterfrom
mattmoor:informer-syncing

Conversation

@mattmoor
Copy link
Copy Markdown
Member

This is the first change of several that will restructure how our controllers process work. This change simply reworks the Base constructor interface to make it harder (I think) to not add an informer to the set we are syncing before we start reconciliation.

This also moves the burden of adding the primary resource's event handler back into the individual controllers (as a prelude to making our other events go through the same workqueue).

WIP until #1195 lands.

@google-prow-robot google-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2018
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/controller/configuration/configuration.go 77.7% 78.3% 0.6
pkg/controller/configuration/configuration_test.go 77.7% 78.3% 0.6
pkg/controller/route/route.go 78.0% 77.9% -0.1
pkg/controller/route/route_test.go 78.0% 77.9% -0.1
pkg/controller/revision/revision.go 80.4% 80.6% 0.1
pkg/controller/revision/revision_test.go 80.4% 80.6% 0.1

*TestCoverage feature is being tested, do not rely on any info here yet

This is the first change of several that will restructure how our controllers process work.  This change simply reworks the Base constructor interface to make it harder (I think) to not add an informer to the set we are syncing before we start reconciliation.

This also moves the burden of adding the primary resource's event handler back into the individual controllers (as a prelude to making our other events go through the same workqueue).
@mattmoor mattmoor changed the title [WIP] Restructure the base/controller split. Restructure the base/controller split. Jun 14, 2018
@google-prow-robot google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2018
@mattmoor
Copy link
Copy Markdown
Member Author

The baseline change is in, so this should be RFAL.

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/controller/configuration/configuration.go 77.7% 78.3% 0.6
pkg/controller/route/route.go 78.0% 78.4% 0.3
pkg/controller/revision/revision.go 80.4% 80.6% 0.1
pkg/controller/revision/revision_test.go 80.4% 80.6% 0.1

*TestCoverage feature is being tested, do not rely on any info here yet

@dprotaso
Copy link
Copy Markdown
Member

@mattmoor
Copy link
Copy Markdown
Member Author

@dprotaso I need to get you the big book of @mattmoor acronyms :)

Ready For A Look

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jun 14, 2018

/lgtm
/approve

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2018
@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas-google

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@google-prow-robot google-prow-robot merged commit e887357 into knative:master Jun 14, 2018
@mattmoor mattmoor deleted the informer-syncing branch June 14, 2018 04:36
Copy link
Copy Markdown
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @mattmoor!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants