✨ (go/v3) Add the --force option for the webhook #1903
Conversation
k8s-ci-robot
added
the
size/M
k8s-ci-robot
added
the
cncf-cla: yes
|
/cc @camilamacedo86 |
prafull01
changed the title
prafull01
changed the title
k8s-ci-robot
added
the
approved
| if err = (&crewv1.Captain{}).SetupWebhookWithManager(mgr); err != nil { | ||
| setupLog.Error(err, "unable to create webhook", "webhook", "Captain") | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
/hold
We should not duplicate it since the file will be re-created.
Could we fix it to get it merged?
There was a problem hiding this comment.
Hi @camilamacedo86 With current behavior we can not avoid duplicating that. We can not force overwrite the main.go as the functionality is to overwrite the entire file, not the section. Also, main.go is edited by other apis as well and hence we can not do that.
To remove the such duplicate scaffolding we need to fix this
There was a problem hiding this comment.
would not possible if we use the --force option then not update the main.go? wdyt?
Also, wdyt about we raise an issue in the repo to address this todo? It might give more visibility.
There was a problem hiding this comment.
Let me raise the issue and will try to work on it.
We can not skip the update on main.go because if user runs --force for the first time in webhook create command. It will not setup the webhook manager in the main.go.
There was a problem hiding this comment.
Good catcher. I am ok with we move forward with it and work on the duplication/todo in a follow-up.
/hold cancel
k8s-ci-robot
added
do-not-merge/hold
pkg/model/config/config.go
Outdated
| func (c Config) HasWebhook(kind string) bool { | ||
| for _, r := range c.Resources { | ||
| if r.Kind == kind { | ||
| return r.WebhookVersion != "" |
There was a problem hiding this comment.
We need to rebase it with masyter change it to r.Webhook != nil && r.Webhook.WebhookVersion != "". More; #1899
pkg/model/config/config.go
Outdated
| // HasWebhook returns true if webhook is already present | ||
| func (c Config) HasWebhook(kind string) bool { | ||
| for _, r := range c.Resources { | ||
| if r.Kind == kind { |
There was a problem hiding this comment.
we need to compare here gkv becuase we can have the same Kind for diff apis/gkv.
/hold
k8s-ci-robot
added
do-not-merge/hold
prafull01
force-pushed
the
force-webhook
branch
from
21a17cb to
e604838
Compare
k8s-ci-robot
removed
the
needs-rebase
There was a problem hiding this comment.
it shows fine for me. 👍
shows the same scenario of #1847 discussed in the bug triage meeting.
/lgtm
/hold cancel
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, 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 |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Description
Add the --force option to overwrite already generated webhook code.
Motivation
Currently, user do not have a way to overwrite already scaffold code other than deleting the scaffold code manually.