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

Unable to Add Additional Webhooks Without Forcing Re-scaffold #4146

Open
camilamacedo86 opened this issue Sep 9, 2024 · 2 comments · Fixed by #4286
Open

Unable to Add Additional Webhooks Without Forcing Re-scaffold #4146

camilamacedo86 opened this issue Sep 9, 2024 · 2 comments · Fixed by #4286
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Milestone
webhook

Comments

@camilamacedo86
Copy link
Member

What broke? What's expected?

When trying to add an additional webhook (e.g., conversion webhook) to an API where other webhooks (like defaulting or validation) are already scaffolded, Kubebuilder throws an error stating that the webhook files already exist. Currently, the only workaround is to use the --force option, which leads to the entire webhook being re-scaffolded, overwriting any custom implementation added by the user. This behavior results in lost code and makes it difficult to incrementally add webhooks.

Kubebuilder should allow adding additional webhooks without forcing the user to use --force and without overwriting existing implementation. It should:

  • Detect existing webhooks.
  • Ensure appropriate markers are present.
  • Scaffold only the necessary webhook logic (e.g., for conversion, validation, or defaulting) specific to the new request, preserving existing code.

Such as we do in the main.go and for the test-e2e as well. See the WebhookTestUpdater for test-e2e. We need ensure that webhooks follow the same: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/test/e2e/test.go#L58-L103

Reproducing this issue

    1. Scaffold an API with defaulting and validation webhooks:
kubebuilder create api --group batch --version v1 --kind CronJob --resource=true --controller=true
kubebuilder create webhook --group batch --version v1 --kind CronJob --defaulting --programmatic-validation
    1. Try adding a conversion webhook for the same API and kind:
kubebuilder create webhook --group batch --version v1 --kind CronJob --conversion
    1. You’ll encounter an error indicating that the webhook files already exist.
    1. The only option to proceed is to use --force, which will remove the current webhook implementation:
kubebuilder create webhook --group batch --version v1 --kind CronJob --conversion --force
    1. The --force option re-scaffolds the entire webhook, leading to the loss of any custom code previously added to the webhook.

Expected Behavior/Request Kubebuilder should scaffold only the specific webhook (e.g., conversion) requested without overwriting the existing webhook code. It should intelligently detect existing webhooks and add the necessary logic for the new type while keeping the current implementation intact.

KubeBuilder (CLI) Version

4.2.0

PROJECT version

No response

Plugin versions

No response

Other versions

No response

Extra Labels

No response
@camilamacedo86 camilamacedo86 added kind/bug Categorizes issue or PR as related to a bug. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Sep 9, 2024
@fischor
Copy link

fischor commented Sep 16, 2024

/assign
Would like to start once #4150 is merged

@camilamacedo86 camilamacedo86 added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Oct 30, 2024
@camilamacedo86 camilamacedo86 mentioned this issue Nov 4, 2024
Merged
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Nov 9, 2024
@camilamacedo86
revert the changes done in the PR kubernetes-sigs#4286
b1db95f 
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
@camilamacedo86 camilamacedo86 mentioned this issue Nov 9, 2024
Merged
@camilamacedo86 camilamacedo86 reopened this Nov 9, 2024
@camilamacedo86
Copy link
Member Author

We cannot skip.
We will need to create new kubebuilder scaffold markers to fix this issue.

@fischor fischor removed their assignment Nov 9, 2024
@camilamacedo86 camilamacedo86 mentioned this issue Nov 10, 2024
Merged
vtrenton pushed a commit to vtrenton/kubebuilder that referenced this issue Nov 20, 2024
@camilamacedo86 @vtrenton
revert the changes done in the PR kubernetes-sigs#4286
ce3d6d8 
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
@camilamacedo86 camilamacedo86 added this to the webhook milestone Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Milestone
webhook
2 participants
@camilamacedo86 @fischor