Skip to content

🐛 fix webhook injector#316

Closed
mengqiy wants to merge 3 commits intokubernetes-sigs:masterfrom
mengqiy:fixinjectorforwebhook
Closed

🐛 fix webhook injector#316
mengqiy wants to merge 3 commits intokubernetes-sigs:masterfrom
mengqiy:fixinjectorforwebhook

Conversation

@mengqiy
Copy link
Copy Markdown
Member

@mengqiy mengqiy commented Feb 4, 2019 •
edited
Loading

fixes #309

Based on #300.

use mgr.SetFields() for webhook server and implement Injector interface for webhooks.

@mengqiy mengqiy requested a review from DirectXMan12 February 4, 2019 19:51
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 4, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @pwittrock 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

@k8s-ci-robot k8s-ci-robot requested a review from droot February 4, 2019 19:51
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 4, 2019
@mengqiy
Copy link
Copy Markdown
Member Author

mengqiy commented Feb 4, 2019

cc: @kdada

@DirectXMan12
Copy link
Copy Markdown
Contributor

DirectXMan12 commented Feb 4, 2019

can you provide a bit more detail in the PR description/commit message? Otherwise LGTM

@mengqiy mengqiy force-pushed the fixinjectorforwebhook branch from aa84331 to 859819e Compare February 4, 2019 23:27
@mengqiy
Copy link
Copy Markdown
Member Author

mengqiy commented Feb 4, 2019

can you provide a bit more detail in the PR description/commit message?

Done. Updated both.

droot
droot reviewed Feb 5, 2019
View reviewed changes
Comment thread pkg/webhook/server.go Outdated
s.sMux.Handle(webhook.GetPath(), webhook.Handler())

// Inject dependencies into each webhook
if err := s.manager.SetFields(webhooks[i]); err != nil {
Copy link
Copy Markdown
Contributor

@droot droot Feb 5, 2019

Choose a reason for hiding this comment

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

Interesting... Won't manager call SetField on webhook server and that should call the inject field for each of the webhooks ? because manager knows when all the deps are resolved ?

Copy link
Copy Markdown
Contributor

@DirectXMan12 DirectXMan12 Feb 7, 2019

Choose a reason for hiding this comment

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

yeah, this should be receiving an injectfunc, which it should be using to inject things, as opposed to having a reference to the manager.

@mengqiy mengqiy force-pushed the fixinjectorforwebhook branch from 859819e to c5c7205 Compare February 12, 2019 00:51
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 12, 2019
@mengqiy mengqiy force-pushed the fixinjectorforwebhook branch from c5c7205 to 47a7b28 Compare February 12, 2019 00:56
@mengqiy mengqiy force-pushed the fixinjectorforwebhook branch from 47a7b28 to a4ef929 Compare February 12, 2019 01:08
@mengqiy
Copy link
Copy Markdown
Member Author

mengqiy commented Feb 12, 2019

The first 2 commits are from #300.

@mengqiy mengqiy mentioned this pull request Feb 12, 2019
Merged
droot
droot approved these changes Feb 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

overall looks good to me.

Comment thread example/main.go
WithManager(mgr).
ForType(&corev1.Pod{}).
Handlers(&podAnnotator{}).
Build()
Copy link
Copy Markdown
Contributor

@droot droot Feb 12, 2019

Choose a reason for hiding this comment

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

This looks a lot simpler now :)

mengqiy pushed a commit to mengqiy/controller-runtime that referenced this pull request Feb 13, 2019
Address comments and pull kubernetes-sigs#316 in
52baf75 
drop server name and client
fix injector
@mengqiy
Copy link
Copy Markdown
Member Author

mengqiy commented Feb 13, 2019

Closing since the commit will be in #300.

@mengqiy mengqiy closed this Feb 13, 2019
@mengqiy mengqiy deleted the fixinjectorforwebhook branch February 14, 2019 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

1 more reviewer

@droot droot droot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Webhook: Need more injection Interface in webhook server

4 participants

@mengqiy @k8s-ci-robot @DirectXMan12 @droot