-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚠️ split the webhook builder out as a separate builder. #497
Conversation
Need to split out conversion registration as well (in a separate PR) @droot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A few minor nits.
pkg/builder/webhook.go
Outdated
} | ||
} | ||
|
||
err = conversion.CheckConvertibility(blder.mgr.GetScheme(), blder.apiType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make a separate PR on top of this to enable conversion only when type implements convertible.
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Minor nits (I am ok with those being done in a follow up PRs as well).
|
||
// TODO(droot): update the GoDoc for conversion. | ||
|
||
// For takes a runtime.Object which should be a CR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily this PR, but we should consider adding an example for this that shows in godoc. They are super useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL |
Controller builder now only build controller; while webhook builder only build webhook.
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: mengqiy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
build.go
=>controller.go
+webhook.go
build_test.go
=>controller_test.go
+webhook_test.go
TODO: figure out if
For
method in webhook builder should be variadic?