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/v3) Add the --force option for the webhook #1903

Merged
merged 1 commit into from
Dec 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions generate_testdata.sh
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ scaffold_test_project() {
$kb create api --group crew --version v1 --kind Admiral --controller=true --resource=true --namespaced=false --make=false
$kb create webhook --group crew --version v1 --kind Admiral --defaulting
$kb create api --group crew --version v1 --kind Laker --controller=true --resource=false --make=false
if [ $project == "project-v3" ]; then
$kb create webhook --group crew --version v1 --kind Captain --defaulting --programmatic-validation --force
fi
elif [[ $project =~ multigroup ]]; then
header_text 'Switching to multigroup layout ...'
$kb edit --multigroup=true
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ type CLI interface {

// cli defines the command line structure and interfaces that are used to
// scaffold kubebuilder project files.
type cli struct {
type cli struct { //nolint:maligned
/* Fields set by Option */

// Root command name. It is injected downstream to provide correct help, usage, examples and errors.
Expand Down
3 changes: 1 addition & 2 deletions pkg/cli/edit.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package cli
package cli // nolint:dupl

import (
"fmt"
Expand Down Expand Up @@ -50,7 +50,6 @@ func (c cli) newEditContext() plugin.Context {
}
}

// nolint:dupl
func (c cli) bindEdit(ctx plugin.Context, cmd *cobra.Command) {
if len(c.resolvedPlugins) == 0 {
cmdErr(cmd, fmt.Errorf(noPluginError))
Expand Down
11 changes: 11 additions & 0 deletions pkg/model/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,17 @@ func (c Config) HasGroup(group string) bool {
return false
}

// HasWebhook returns true if webhook is already present
func (c Config) HasWebhook(resource ResourceData) bool {
for _, r := range c.Resources {
if r.isGVKEqualTo(resource) {
return r.Webhooks != nil
}
}

return false
}

// IsCRDVersionCompatible returns true if crdVersion can be added to the existing set of CRD versions.
func (c Config) IsCRDVersionCompatible(crdVersion string) bool {
return c.resourceAPIVersionCompatible("crd", crdVersion)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ type Webhook struct { // nolint:maligned
Defaulting bool
// If scaffold the validating webhook
Validating bool

Force bool
}

// SetTemplateDefaults implements file.Template
Expand Down Expand Up @@ -69,7 +71,11 @@ func (f *Webhook) SetTemplateDefaults() error {
}
f.TemplateBody = webhookTemplate

f.IfExistsAction = file.Error
if f.Force {
f.IfExistsAction = file.Overwrite
} else {
f.IfExistsAction = file.Error
}

f.GroupDomainWithDash = strings.Replace(f.Resource.Domain, ".", "-", -1)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ var _ file.Template = &ManagerWebhookPatch{}
// ManagerWebhookPatch scaffolds a file that defines the patch that enables webhooks on the manager
type ManagerWebhookPatch struct {
file.TemplateMixin

Force bool
}

// SetTemplateDefaults implements file.Template
Expand All @@ -37,8 +39,12 @@ func (f *ManagerWebhookPatch) SetTemplateDefaults() error {

f.TemplateBody = managerWebhookPatchTemplate

// If file exists (ex. because a webhook was already created), skip creation.
f.IfExistsAction = file.Skip
if f.Force {
f.IfExistsAction = file.Overwrite
} else {
// If file exists (ex. because a webhook was already created), skip creation.
f.IfExistsAction = file.Skip
}

return nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ type Kustomization struct {

// Version of webhook the project was configured with.
WebhookVersion string

Force bool
}

// SetTemplateDefaults implements file.Template
Expand All @@ -40,8 +42,12 @@ func (f *Kustomization) SetTemplateDefaults() error {

f.TemplateBody = kustomizeWebhookTemplate

// If file exists (ex. because a webhook was already created), skip creation.
f.IfExistsAction = file.Skip
if f.Force {
f.IfExistsAction = file.Overwrite
} else {
// If file exists (ex. because a webhook was already created), skip creation.
f.IfExistsAction = file.Skip
}

if f.WebhookVersion == "" {
f.WebhookVersion = "v1"
Expand Down
7 changes: 5 additions & 2 deletions pkg/plugins/golang/v3/scaffolds/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type webhookScaffolder struct {
resource *resource.Resource

// Webhook type options.
defaulting, validation, conversion bool
defaulting, validation, conversion, force bool
}

// NewWebhookScaffolder returns a new Scaffolder for v2 webhook creation operations
Expand All @@ -49,6 +49,7 @@ func NewWebhookScaffolder(
defaulting bool,
validation bool,
conversion bool,
force bool,
) cmdutil.Scaffolder {
return &webhookScaffolder{
config: config,
Expand All @@ -57,6 +58,7 @@ func NewWebhookScaffolder(
defaulting: defaulting,
validation: validation,
conversion: conversion,
force: force,
}
}

Expand Down Expand Up @@ -88,11 +90,12 @@ You need to implement the conversion.Hub and conversion.Convertible interfaces f
WebhookVersion: s.resource.Webhooks.WebhookVersion,
Defaulting: s.defaulting,
Validating: s.validation,
Force: s.force,
},
&templates.MainUpdater{WireWebhook: true},
&kdefault.WebhookCAInjectionPatch{WebhookVersion: s.resource.Webhooks.WebhookVersion},
&kdefault.ManagerWebhookPatch{},
&webhook.Kustomization{WebhookVersion: s.resource.Webhooks.WebhookVersion},
&webhook.Kustomization{WebhookVersion: s.resource.Webhooks.WebhookVersion, Force: s.force},
&webhook.KustomizeConfig{},
&webhook.Service{},
); err != nil {
Expand Down
13 changes: 12 additions & 1 deletion pkg/plugins/golang/v3/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v3

import (
"errors"
"fmt"
"io/ioutil"
"path/filepath"
Expand Down Expand Up @@ -44,6 +45,9 @@ type createWebhookSubcommand struct {
validation bool
conversion bool

// force indicates that the resource should be created even if it already exists
force bool

// runMake indicates whether to run make or not after scaffolding webhooks
runMake bool
}
Expand Down Expand Up @@ -79,6 +83,8 @@ func (p *createWebhookSubcommand) BindFlags(fs *pflag.FlagSet) {
"version of {Mutating,Validating}WebhookConfigurations to scaffold. Options: [v1, v1beta1]")

fs.BoolVar(&p.runMake, "make", true, "if true, run make after generating files")
fs.BoolVar(&p.force, "force", false,
"attempt to create resource even if it already exists")

fs.BoolVar(&p.defaulting, "defaulting", false,
"if set, scaffold the defaulting webhook")
Expand Down Expand Up @@ -112,6 +118,10 @@ func (p *createWebhookSubcommand) Validate() error {
" kind and version provided", p.commandName)
}

if p.config.HasWebhook(p.resource.Data()) && !p.force {
return errors.New("webhook resource already exists")
}

if !p.config.IsWebhookVersionCompatible(p.resource.Webhooks.WebhookVersion) {
return fmt.Errorf("only one webhook version can be used for all resources, cannot add %q",
p.resource.Webhooks.WebhookVersion)
Expand All @@ -129,7 +139,8 @@ func (p *createWebhookSubcommand) GetScaffolder() (cmdutil.Scaffolder, error) {

// Create the actual resource from the resource options
res := p.resource.NewResource(p.config, false)
return scaffolds.NewWebhookScaffolder(p.config, string(bp), res, p.defaulting, p.validation, p.conversion), nil
return scaffolds.NewWebhookScaffolder(p.config, string(bp), res, p.defaulting, p.validation, p.conversion,
p.force), nil
}

func (p *createWebhookSubcommand) PostScaffold() error {
Expand Down
6 changes: 6 additions & 0 deletions testdata/project-v3/api/v1/webhook_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ var _ = BeforeSuite(func() {
err = admissionv1beta1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

err = admissionv1beta1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:scheme

k8sClient, err = client.New(cfg, client.Options{Scheme: scheme})
Expand All @@ -108,6 +111,9 @@ var _ = BeforeSuite(func() {
err = (&Admiral{}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

err = (&Captain{}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:webhook

go func() {
Expand Down
4 changes: 4 additions & 0 deletions testdata/project-v3/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ func main() {
setupLog.Error(err, "unable to create controller", "controller", "Laker")
os.Exit(1)
}
if err = (&crewv1.Captain{}).SetupWebhookWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create webhook", "webhook", "Captain")
os.Exit(1)
}
Comment on lines +125 to +128
Copy link
Member

Choose a reason for hiding this comment

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

/hold

We should not duplicate it since the file will be re-created.
Could we fix it to get it merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 15, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Good catcher. I am ok with we move forward with it and work on the duplication/todo in a follow-up.
/hold cancel

// +kubebuilder:scaffold:builder

if err := mgr.AddHealthzCheck("health", healthz.Ping); err != nil {
Expand Down