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

🐛 (go/v4) Fix issue with scaffolding multiple webhooks for the same resource #4286

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Nov 4, 2024

Attempting to create multiple webhooks for the same resource with different configurations (e.g., --defaulting and --programmatic-validation) resulted in an error indicating that the webhook already exists.

This update allows multiple webhook types to be scaffolded for a single resource without conflict. Users can scaffold different webhooks for the same GKV afterwards without needing to do so all at once, as expected.

Either we are adding tests to cover the webhook undergo/v4 conversion type.
The tests are scaffolding the conversion webhook after the defaulting and validation were created already.
Therefore, we are validating the fix done in this PR.

Closes: #4146

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2024
@camilamacedo86 camilamacedo86 force-pushed the e2e-test-go-v4-conversion branch 2 times, most recently from 3b9c7d5 to a7fab10 Compare November 4, 2024 06:54
@camilamacedo86 camilamacedo86 changed the title 🌱 (WIP) add e2e tests towards go/v4 to validate conversion webhooks 🐛 Fix issue with scaffolding multiple webhooks for the same resource Nov 4, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 Fix issue with scaffolding multiple webhooks for the same resource 🐛 (go/v4) Fix issue with scaffolding multiple webhooks for the same resource Nov 4, 2024
@camilamacedo86 camilamacedo86 force-pushed the e2e-test-go-v4-conversion branch 7 times, most recently from 182dc8f to 03e167c Compare November 5, 2024 07:50
Previously, attempting to create multiple webhooks for the same resource with different configurations (e.g., `--defaulting` and `--programmatic-validation`) resulted in an error indicating that the webhook already exists.

This update allows multiple webhook types to be scaffolded for a single resource without conflict, providing flexibility for users be able to scaffold different types of webhooks for the same GKV afterwords and without the need to be all by once.
Copy link
Member Author

@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 is a very small bug so I will move forward
I will not make nobody waste time on the reviews here when we have a lot of open PRs that we really need a hand on the reviews.

Anyway, if someone has any suggestion for improvement and etc please fell free to send a PR against. Your help is very welcome and appreciated !

@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2024
@k8s-ci-robot k8s-ci-robot merged commit 5ce0387 into kubernetes-sigs:master Nov 5, 2024
18 checks passed
@camilamacedo86 camilamacedo86 deleted the e2e-test-go-v4-conversion branch November 5, 2024 18:29
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Nov 9, 2024
We are reverting the changes done in the PR kubernetes-sigs#4286.
We are just keeping the e2e tests improvements.
We will not able to solve the issue kubernetes-sigs#4146 without add new markers.

We cannot skip the action to write an webhook since
it will result in incomplete inplementation in the <kind>_webhook.go as in
the <kind>_webhook_test.go
k8s-ci-robot added a commit that referenced this pull request Nov 9, 2024
🐛 revert the changes done in the PR #4286
vtrenton pushed a commit to vtrenton/kubebuilder that referenced this pull request Nov 20, 2024
We are reverting the changes done in the PR kubernetes-sigs#4286.
We are just keeping the e2e tests improvements.
We will not able to solve the issue kubernetes-sigs#4146 without add new markers.

We cannot skip the action to write an webhook since
it will result in incomplete inplementation in the <kind>_webhook.go as in
the <kind>_webhook_test.go
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. release-blocker size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to Add Additional Webhooks Without Forcing Re-scaffold
2 participants