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.

Sign up for GitHub

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

Jump to bottom

⚠️ (go/v3-alpha) default to v1 CRDs and webhooks #1644

Merged
merged 2 commits into from
Nov 17, 2020
Merged

⚠️ (go/v3-alpha) default to v1 CRDs and webhooks #1644

merged 2 commits into from
Nov 17, 2020

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Aug 19, 2020 •
edited
Loading

This PR bumps controller-tools to v0.4.0, which defaults CRD and webhook config versions to v1. To conform to this change, the default scaffolded patches for either type will be v1, with the option to use v1beta1 via create api --crd-version and create webhook --webhook-version.

Closes #933
Closes #1065
Closes #1734
Closes #1827

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 19, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 19, 2020
@estroz
Copy link
Contributor Author

estroz commented Aug 19, 2020

/cc @DirectXMan12 @vincepri @alvaroaleman @camilamacedo86 @joelanford

@alvaroaleman
Copy link
Member

Sorry, I am not using kubebuilder the CLI at all, hence I can not review changes here.
/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from alvaroaleman August 19, 2020 22:25
@estroz estroz force-pushed the feature/crd-webhook-v1 branch from 5d02748 to 5d98011 Compare August 19, 2020 22:34
@estroz estroz mentioned this pull request Aug 19, 2020
Closed
@estroz estroz force-pushed the feature/crd-webhook-v1 branch from 5d98011 to 16d287f Compare August 19, 2020 22:54
@estroz
Copy link
Contributor Author

estroz commented Aug 19, 2020

Since you can generate multiple CRD/webhook config versions with controller-gen webhook:webhookVersions={v1,v1beta1} crd:crdVersions={v1,v1beta1} I'm wondering if it makes sense to allow multiple versions to be passed to the CLI with --crd-versions and --webhook-versions. To do so will require some scaffolding changes, but is possible. Holding for CLI discussion.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2020
camilamacedo86
camilamacedo86 reviewed Aug 19, 2020
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a nit about the docs and a small question.

@estroz
Copy link
Contributor Author

estroz commented Aug 20, 2020

/retest

@jiachengxu
Copy link

Since you can generate multiple CRD/webhook config versions with controller-gen webhook:webhookVersions={v1,v1beta1} crd:crdVersions={v1,v1beta1} I'm wondering if it makes sense to allow multiple versions to be passed to the CLI with --crd-versions and --webhook-versions. To do so will require some scaffolding changes, but is possible. Holding for CLI discussion.

/hold

@estroz For webhook, there is not a package marker webhook:webhookVersions={v1,v1beta1}, for generating multiple versions of webhook, you need to specify version in the marker, for example:

// +kubebuilder:webhook:webhookVersions=v1;v1beta1,verbs=create;update,path=/mutate-testdata-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=default.cronjob.testdata.kubebuilder.io,sideEffects=None

Fully discussion are here: kubernetes-sigs/controller-tools#469 (comment)

@estroz
Copy link
Contributor Author

estroz commented Aug 21, 2020

@jiachengxu you're correct, I got confused with a POC PR I made recently to version webhooks. My point about flags still stands though.

@estroz
Copy link
Contributor Author

estroz commented Aug 21, 2020

/retest

Adirio
Adirio suggested changes Nov 11, 2020
View reviewed changes
pkg/model/config/config.go Show resolved Hide resolved
pkg/model/config/config.go Show resolved Hide resolved
pkg/model/resource/options.go Show resolved Hide resolved
pkg/plugin/v3/api.go Show resolved Hide resolved
pkg/plugin/v3/scaffolds/init.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/crd/kustomizeconfig.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/crd/kustomizeconfig.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/crd/kustomizeconfig.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/webhook/kustomization.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/webhook/kustomizeconfig.go Outdated Show resolved Hide resolved
@estroz estroz force-pushed the feature/crd-webhook-v1 branch from 373e17c to d5ae8be Compare November 16, 2020 22:49
camilamacedo86
camilamacedo86 approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It shows fine 👍 for me.
/approve

@camilamacedo86 camilamacedo86 mentioned this pull request Nov 16, 2020
Closed
@camilamacedo86
Copy link
Member

/pull-kubebuilder-e2e-k8s-1-16-2
/test pull-kubebuilder-e2e-k8s-1-17-0

@camilamacedo86
Copy link
Member

/pull-kubebuilder-e2e-k8s-1-16-2

1 similar comment
@camilamacedo86
Copy link
Member

/pull-kubebuilder-e2e-k8s-1-16-2

@estroz estroz force-pushed the feature/crd-webhook-v1 branch from d5ae8be to 6e7ffba Compare November 17, 2020 02:09
@estroz
pkg/plugin/v3: default to v1 CRDs and webhooks, with options to
3a971ea 
use v1beta1 via --crd-version and --webhook-version

pkg/plugin/v3: bump controller-tools to the master, which contains
the "webhook:admissionReviewVersions" marker option required for
webhook configs to work with controller-runtime v0.6.3. Tests are
updated to respect multiple tool (controller-gen, kustomize)
versions since e2e tests run for multiple plugin versions.

pkg/model/config: use Config.Resources to hold core resource version info
@estroz
go/v3-alpha: bump ControllerToolsVersion to v0.4.1
239c6d2
@estroz estroz force-pushed the feature/crd-webhook-v1 branch from 6e7ffba to 239c6d2 Compare November 17, 2020 02:46
@estroz
Copy link
Contributor Author

estroz commented Nov 17, 2020

/retest

@estroz estroz requested a review from Adirio November 17, 2020 03:01
@estroz
Copy link
Contributor Author

estroz commented Nov 17, 2020

/retest

1 similar comment
@estroz
Copy link
Contributor Author

estroz commented Nov 17, 2020

/retest

Adirio
Adirio approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86, estroz

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:
  • OWNERS [camilamacedo86,estroz]

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 cd3cb2a into kubernetes-sigs:master Nov 17, 2020
@estroz estroz deleted the feature/crd-webhook-v1 branch November 17, 2020 06:48
This was referenced Nov 17, 2020
Closed
Closed
@camilamacedo86 camilamacedo86 mentioned this pull request Dec 1, 2020
Closed
@estroz estroz mentioned this pull request Dec 2, 2020
Merged
@estroz estroz mentioned this pull request Dec 11, 2020
Closed
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
@zaneb zaneb mentioned this pull request Sep 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@Adirio Adirio Adirio approved these changes

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@droot droot Awaiting requested review from droot

@joelanford joelanford Awaiting requested review from joelanford

@vincepri vincepri Awaiting requested review from vincepri

@hasbro17 hasbro17 Awaiting requested review from hasbro17

Assignees

@Adirio Adirio

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
9 participants
@estroz @alvaroaleman @jiachengxu @DirectXMan12 @camilamacedo86 @prafull01 @k8s-ci-robot @Adirio @hasbro17