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

Add conversion webhook support #4346

Merged
merged 2 commits into from
May 22, 2021
Merged

Add conversion webhook support #4346

merged 2 commits into from
May 22, 2021

Conversation

varshaprasad96
Copy link
Member

Description of the change:
This PR adds support to scaffold conversion webhook in CSV,
when CRDs include conversion webhookClientConfig.

Follow-up from: #3762

Co-authored-by: Eric Stroczynski ericstroczynski@gmail.com

Motivation for the change:
Support conversion webhook scaffold in SDK

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@varshaprasad96 varshaprasad96 mentioned this pull request Mar 8, 2021
Closed
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 23, 2021
@estroz
Copy link
Member

estroz commented Mar 24, 2021

@varshaprasad96 this is still in-flight, correct?

@varshaprasad96
Copy link
Member Author

Yes, this is still in my to-do list. Will pick this up and continue working on it.

@estroz
Copy link
Member

estroz commented Mar 24, 2021

/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 24, 2021
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2021
@openshift-ci-robot
Copy link

@varshaprasad96: PR needs rebase.

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.

@varshaprasad96 varshaprasad96 force-pushed the add/conv-webhhok-scaffold branch from 5e06a9f to fe91bc1 Compare April 22, 2021 21:05
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2021
@varshaprasad96 varshaprasad96 force-pushed the add/conv-webhhok-scaffold branch from fe91bc1 to c1703c8 Compare May 10, 2021 22:11
@varshaprasad96
Copy link
Member Author

Blocked by kubernetes-sigs/kubebuilder#2176

@varshaprasad96 varshaprasad96 requested a review from estroz May 10, 2021 22:13
@varshaprasad96 varshaprasad96 mentioned this pull request May 12, 2021
Merged
2 tasks
@varshaprasad96 varshaprasad96 force-pushed the add/conv-webhhok-scaffold branch 2 times, most recently from 5fd2045 to 1f9c5f4 Compare May 14, 2021 19:09
estroz
estroz suggested changes May 14, 2021
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

I'd like to see a test case where multiple CRDs use the same conversion webhook server.

@qinggniq
Copy link

key an eye on it, the kubebuilder book cronjob example always fails about the missing conversionReviewVersions when enable webhook.

@estroz
Copy link
Member

estroz commented May 19, 2021

@qinggniq those were added in kubernetes-sigs/kubebuilder#2176 but not to the book tutorials. @varshaprasad96 would you mind doing that when you have a chance?

@varshaprasad96
Copy link
Member Author

@estroz @qinggniq sure, will create a pr to fix it in kb

@qinggniq
Copy link

@estroz Yes, using the latest source code build kubebuilder to create webhook will generate the right webhook inject patch, I think just update kubebuilder binary at https://go.kubebuilder.io/dl/latest/$(go env GOOS)/$(go env GOARCH) could make book tutorial work. And the configs under kubebuilder/doc/book/src/*/testdata/config need to regenerate.

@varshaprasad96
Add conversion webhook definition to csv
8b379e9 
This PR adds the conversion webhook definition to csv. The csv generator includes
the logic to verfiy if the {v1, v1beta1} crd has conversion clinet config defined.
If so, it scaffolds the conversion wh configuration defails in the csv

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
@varshaprasad96 varshaprasad96 force-pushed the add/conv-webhhok-scaffold branch from e29060d to b421be0 Compare May 21, 2021 22:03
@varshaprasad96 varshaprasad96 requested a review from estroz May 21, 2021 22:22
@estroz
Copy link
Member

estroz commented May 21, 2021

@varshaprasad96 you have to make generate with go 1.16

Aside: we may want to consider running generation targets within a golang container so env issues like this do not happen.

@varshaprasad96
add test for multiversion crd
d789200 
Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
@varshaprasad96 varshaprasad96 force-pushed the add/conv-webhhok-scaffold branch from b421be0 to d789200 Compare May 21, 2021 23:57
estroz
estroz approved these changes May 22, 2021
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2021
@varshaprasad96 varshaprasad96 merged commit 03e8c95 into operator-framework:master May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awgreene awgreene awgreene left review comments

@estroz estroz estroz approved these changes

@theishshah theishshah Awaiting requested review from theishshah

Assignees

@estroz estroz

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@varshaprasad96 @openshift-bot @estroz @openshift-ci-robot @qinggniq @awgreene