Skip to content
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

✨ scaffold the webhook builder #816

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Jun 25, 2019

Add a new command kubebuilder create webhook for user to scaffold webhook.
Kubebuilder now dynamically register commands based on metadata in PROJECT.
If PROJECT doesn't exist or PROJECT exists with version=2, register
create webhook, but no alpha webhook.
If PROJECT exists and version=1, register alpha webhook but no create webhook

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 25, 2019
@mengqiy mengqiy requested review from droot and removed request for Liujingfang1 June 25, 2019 21:26
@mengqiy mengqiy added this to Code Awaiting Review in 2.0.0 cross-project tracking Jun 25, 2019
cmd/api.go Outdated Show resolved Hide resolved
cmd/api.go Outdated
@@ -48,6 +48,9 @@ func (o *apiOptions) bindCmdFlags(cmd *cobra.Command) {
cmd.Flags().BoolVar(&o.apiScaffolder.DoController, "controller", true,
"if set, generate the controller without prompting the user")
o.controllerFlag = cmd.Flag("controller")
cmd.Flags().BoolVar(&o.apiScaffolder.DoWebhook, "webhook", false,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to differentiate from "manual" webhooks (i.e. not our special defaulting/validating)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed with Solly offline. We can have something like kubebuilder create webhook --low-level-defaulting --low-level-programmatic-validation for "manual" webhooks if we want to do it in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed tracking issue #833

@DirectXMan12
Copy link
Contributor

discussed this a bit more -- we probably want a separate subcommand for enabling this

Copy link
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.

Change looks good except that we want to have a separate subcommand to enable webhooks.

@mengqiy mengqiy force-pushed the webhookbuilder branch 2 times, most recently from 09126a1 to 2438fc5 Compare June 26, 2019 21:05
Copy link
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.

Mostly looks good. I have a few questions/comments.

cmd/webhook_v2.go Show resolved Hide resolved
cmd/webhook_v1.go Outdated Show resolved Hide resolved
cmd/webhook_v2.go Outdated Show resolved Hide resolved
cmd/webhook_v2.go Outdated Show resolved Hide resolved
cmd/webhook_v2.go Show resolved Hide resolved
cmd/webhook_v2.go Outdated Show resolved Hide resolved
cmd/webhook_v2.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
testdata/project-v2/main.go Show resolved Hide resolved
cmd/webhook_v1.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
@mengqiy mengqiy changed the title ✨ scaffold the webhook builder [WIP] ✨ scaffold the webhook builder Jun 28, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2019
pkg/scaffold/v2/gomod.go Outdated Show resolved Hide resolved
@mengqiy
Copy link
Member Author

mengqiy commented Jun 28, 2019

PTAL

@mengqiy mengqiy changed the title [WIP] ✨ scaffold the webhook builder ✨ scaffold the webhook builder Jun 28, 2019
@k8s-ci-robot k8s-ci-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 28, 2019
Copy link
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.

Looks good, have a minor comments.

Thanks for updating the e2e tests.

cmd/create.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/webhook_v2.go Show resolved Hide resolved
@droot droot mentioned this pull request Jul 1, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jul 1, 2019

PTAL

Copy link
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.

Looks good. A couple of minor nits.

cmd/create.go Show resolved Hide resolved
cmd/webhook_v2.go Show resolved Hide resolved
@mengqiy
Copy link
Member Author

mengqiy commented Jul 2, 2019

Need to update example to add --conversion flag.

I didn't add it in the existing example intentionally, since from our earlier offline discussion. --conversion is for advanced use case and flag description should be good enough.
I can still add a separate example for conversion flag if you believe we need it :)

I don't know why GH doesn't allow me to reply under it. :/

@mengqiy
Copy link
Member Author

mengqiy commented Jul 2, 2019

@droot @DirectXMan12 PTAL

@mengqiy
Copy link
Member Author

mengqiy commented Jul 2, 2019

Travis still has a lot of backlog now: https://www.traviscistatus.com/incidents/g9xm3l02dknv

Copy link
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.

Looks good to me.

cmd/webhook_v2.go Outdated Show resolved Hide resolved
cmd/webhook_v2.go Outdated Show resolved Hide resolved
Mengqi Yu added 2 commits July 3, 2019 11:32
add a new command `kubebuilder create webhook` to scaffold the webhooks.
--defaulting, --imperative-validation and --converison can be used to
scaffold mutating, validating and conversion webhooks.
If PROJECT doesn't exist or PROJECT exists with version=2, register
`create webhook`, but no `alpha webhook`.
If PROJECT exists and version=1, register `alpha webhook` but no `create
webhook`
@mengqiy
Copy link
Member Author

mengqiy commented Jul 3, 2019

@droot Should be ready to ship now.

@droot
Copy link
Contributor

droot commented Jul 3, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2019
@droot
Copy link
Contributor

droot commented Jul 3, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droot, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 285b0bc into kubernetes-sigs:master Jul 3, 2019
2.0.0 cross-project tracking automation moved this from Code Awaiting Review to Done Jul 3, 2019
@mengqiy mengqiy deleted the webhookbuilder branch July 3, 2019 19:28
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants