Skip to content

Add check for nil StatsReporter in webhook package#518

Merged
knative-prow-robot merged 4 commits into
knative:masterfrom
anniefu:fixWebhookNil
Jul 10, 2019
Merged

Add check for nil StatsReporter in webhook package#518
knative-prow-robot merged 4 commits into
knative:masterfrom
anniefu:fixWebhookNil

Conversation

@anniefu
Copy link
Copy Markdown
Contributor

@anniefu anniefu commented Jul 9, 2019

Fixes #517

Also add constructors for consumers to use.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 9, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2019
@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

/assign @grantr
/assign @evankanderson

@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

I validated that the fix passes the e2e tests for knative/serving master branch after updating the knative.dev/pkg version to my personal fork with the fix.

@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

/assign @n3wscott

@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Jul 9, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2019
vagababov
vagababov reviewed Jul 9, 2019
View reviewed changes
Comment thread webhook/webhook.go Outdated
Comment thread webhook/webhook.go Outdated
evankanderson
evankanderson reviewed Jul 9, 2019
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

/hold
In case you want to change the return type of NewAdmissionController

Comment thread webhook/webhook.go Outdated
@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jul 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jul 9, 2019
@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

/retest

@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2019
@vagababov
Copy link
Copy Markdown
Contributor

vagababov commented Jul 9, 2019

/approve

/hold
In case you want to change the return type of NewAdmissionController

I didn't suggest that, since it's a NBC change and requires changes to the downstream consumers, I presume.

@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

/approve
/hold
In case you want to change the return type of NewAdmissionController

I didn't suggest that, since it's a NBC change and requires changes to the downstream consumers, I presume.

What does NBC stand for? The constructor is introduced in this PR, so there are no downstream consumers that need to change.

@vagababov
Copy link
Copy Markdown
Contributor

vagababov commented Jul 9, 2019

not backwards compatible :-)

but if it's new then it doesn't matter -- returning error is a preferred method.

evankanderson
evankanderson reviewed Jul 9, 2019
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.

One more API suggestion, having read this again without the panic...

Comment thread webhook/webhook.go Outdated
evankanderson
evankanderson reviewed Jul 9, 2019
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.

/lgtm

/hold
To consider the change in API, but I don't want to completely block fixing eventing.

@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 9, 2019
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019
@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 9, 2019

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jul 9, 2019

@anniefu: you cannot LGTM your own PR.

Details

In response to this:

/lgtm

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.

@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Jul 10, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2019
@knative-prow-robot knative-prow-robot merged commit a68e009 into knative:master Jul 10, 2019
chengjingtao pushed a commit to katanomi/knative-pkg that referenced this pull request Oct 22, 2024
@l-qing
chore: adjust catalog owner (knative#518)
6f76a0e
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

@grantr grantr Awaiting requested review from grantr

@mattmoor mattmoor Awaiting requested review from mattmoor

+1 more reviewer

@vagababov vagababov vagababov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@grantr grantr

@evankanderson evankanderson

@n3wscott n3wscott

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. 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.

Stats Reporter causes a NPE. StatsReporter is new and nil in all repos.

7 participants

@anniefu @n3wscott @knative-prow-robot @vagababov @evankanderson @grantr @googlebot