Skip to content

Reconcile with context#315

Merged
knative-prow-robot merged 2 commits into
knative:masterfrom
grantr:reconcile-with-context
Aug 14, 2018
Merged

Reconcile with context#315
knative-prow-robot merged 2 commits into
knative:masterfrom
grantr:reconcile-with-context

Conversation

@grantr
Copy link
Copy Markdown
Contributor

@grantr grantr commented Aug 3, 2018 •
edited
Loading

This is a WIP update of the feed controller that adds context.Context parameters everywhere in preparation for #303. [EDIT: and #303 is in, yay!]

Still unclear is how the logger will get from main to the reconciler (possibly via ProvideController).

/cc @n3wscott

@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2018
@google-prow-robot
Copy link
Copy Markdown

google-prow-robot commented Aug 3, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: grantr
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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

@grantr grantr force-pushed the reconcile-with-context branch from 26f9511 to ab7623f Compare August 3, 2018 00:10
@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Aug 6, 2018

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

File Old Coverage New Coverage Delta
pkg/controller/feed/reconcile.go 71.7% 71.8% 0.1

@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Aug 6, 2018

Yeah, it is not so bad having ctx passed around.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2018
@grantr grantr changed the title [WIP] Reconcile with context Reconcile with context Aug 6, 2018
@knative-prow-robot knative-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 Aug 6, 2018
@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented Aug 7, 2018

/assign @evankanderson

evankanderson
evankanderson reviewed Aug 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/approve

Thanks! Sorry I missed this earlier, but it looks like it has no merge conflicts, amazingly.

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Aug 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, grantr

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2018
@knative-prow-robot knative-prow-robot merged commit ef81799 into knative:master Aug 14, 2018
@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented Aug 14, 2018

it looks like it has no merge conflicts, amazingly.

I am similarly flabbergasted!

@grantr grantr deleted the reconcile-with-context branch August 14, 2018 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evankanderson evankanderson evankanderson left review comments

@n3wscott n3wscott Awaiting requested review from n3wscott

Assignees

@evankanderson evankanderson

@n3wscott n3wscott

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@grantr @google-prow-robot @knative-metrics-robot @n3wscott @knative-prow-robot @evankanderson