-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
✨ Move the webhookcainjection_patch.yaml file creation from init project to webhook creation #1728
✨ Move the webhookcainjection_patch.yaml file creation from init project to webhook creation #1728
Conversation
Hi @prafull01. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/cc @camilamacedo86 |
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
*/ | |||
|
|||
package kdefault | |||
package webhook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file will still be scaffolded in config/default
, so should remain in the config/kdefault
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done..
@@ -37,7 +37,8 @@ func (f *InjectCAPatch) SetTemplateDefaults() error { | |||
|
|||
f.TemplateBody = injectCAPatchTemplate | |||
|
|||
f.IfExistsAction = file.Error | |||
// If file webhookcainjection_patch.yaml exist, skip its creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the v3 plugin should be changed, so revert changes in pkg/plugin/v2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO would be fine do it for v2+ since it shows a bug introduced instead of a RFE. However, I would be ok with change only for v3+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, scaffolding this patch during init
is intentional because it is directly referenced in config/default/kustomization.yaml
. @DirectXMan12 is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @estroz,
I do not think that it was intentional. Note that, it may be hurting concepts such as encapsulation, single responsibility principle and almost for sure cohesion that may cause unexpected sides effects such difficulty to read, maintain/improve and extend, to mention a few. Also, IMO:
As @pwittrock spoke well another day, it is very hard for users to know and maintain so many config files, in this way, allow the init create/delete/update webhooks manifests not make harder ONLY for us(contributors) but also for users since it means more unnecessary config files for their scenarios. So, a use case that it hurts, for example, would be I am an operator dev would like to see specific manifests which are required/useful for the webhooks only/if my project has webhooks
Also, note that when we introduce the plugin design we start to allow the plugins to be consumed for the other projects. Then, it means that as a developer I can decide to consume only the init and the create the plugin as well which means that my projects would NOT support webhooks then, would not make sense have webhooks flags or scaffolded manifests in the init command/plugin. It shows hurt the domains of responsibility as described in #1644.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify what I mean: the choice to scaffold this file in init
may have been sub-optimal, but it was a conscious (intentional) one. Regardless the right move is to move this to create webhook
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're only making these changes to support the outcome of #1644's discussion about flags, we should hold off on reviewing this PR until that discussion is finalized. If we decide to go with create webhook --webhook-version
over init --webhook-version
, then this PR can go ahead for review.
/hold
19a7bd7
to
ab3f59d
Compare
ab3f59d
to
39bf637
Compare
/ok-to-test |
I think we can : based on the discussion made in the bug triage meeting. |
pkg/plugin/v3/scaffolds/webhook.go
Outdated
@@ -19,6 +19,8 @@ package scaffolds | |||
import ( | |||
"fmt" | |||
|
|||
"sigs.k8s.io/kubebuilder/pkg/plugin/v3/scaffolds/internal/templates/config/kdefault" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Sort the import. It needs to be at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -37,7 +37,8 @@ func (f *InjectCAPatch) SetTemplateDefaults() error { | |||
|
|||
f.TemplateBody = injectCAPatchTemplate | |||
|
|||
f.IfExistsAction = file.Error | |||
// If file webhookcainjection_patch.yaml exist, skip its creation | |||
f.IfExistsAction = file.Skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked that 👍
Great work
&webhook.Kustomization{}, | ||
&webhook.KustomizeConfigWebhook{}, | ||
&webhook.Service{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I think we need also move these ones to the webhook plugin as well since they are specific to the webhook plugin and are not useful to projects which as not in the webhooks. I mean, it shows to be the same scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also thinks the same.. Should I move all the webhook related scaffolding in this PR itself? Or should we wait for discussion to be finalized on #1644 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ct to webhook creation
39bf637
to
54194e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: estroz, prafull01 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 |
I don't think this is a bug. @prafull01 can you remove /hold |
Since @estroz /lgtm this one and just hold until the author be able to update its title with ✨ then, shows that all is done to get it merged. IMO: We will still require a follow up for https://github.com/kubernetes-sigs/kubebuilder/pull/1728/files#r508778432. To address that the issue #1734 was raised. /hold cancel |
The PR will merge with the wrong commit message if you remove the hold @camilamacedo86 /hold |
Ah didn’t see the title was updated. /hold cancel |
Description of the Change:
webhookcainjection_patch.yaml file is created at the init of the project, I have changed that to be created at the time of webhook creation.
Motivation:
I am a consumer of the kubebuilder plugins would like that each plugin only scaffold manifests which are related to its domain so that, I can decide what plugins I'd like to consume or not.
Fixes: #1727