From 4213b7b593c158b2ebe90d006670fc48f82d888c Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 5 Aug 2020 15:23:12 +0200 Subject: [PATCH] Move logic out of the Configuration container - It shouldn't be necessary to figure out the namespace if we just pass --version flag - other small improvements - The version will now be printed as Info at startup, to prevent confusion in pipelines. Signed-off-by: Chris --- cfg/types.go | 11 ------- cmd/common.go | 6 ++++ cmd/configmaps.go | 43 ++++++++++++------------- cmd/history.go | 23 +++++++------- cmd/images.go | 6 ++-- cmd/images_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++++ cmd/orphans.go | 24 +++++++------- cmd/orphans_test.go | 73 ++---------------------------------------- cmd/root.go | 30 +++++++++++++----- cmd/secrets.go | 42 +++++++++++++------------ 10 files changed, 179 insertions(+), 156 deletions(-) create mode 100644 cmd/images_test.go diff --git a/cfg/types.go b/cfg/types.go index 21c7cec..8693ac1 100644 --- a/cfg/types.go +++ b/cfg/types.go @@ -1,10 +1,5 @@ package cfg -import ( - "github.com/appuio/seiso/pkg/kubernetes" - log "github.com/sirupsen/logrus" -) - type ( // Configuration holds a strongly-typed tree of the configuration Configuration struct { @@ -47,13 +42,7 @@ type ( // NewDefaultConfig retrieves the hardcoded configs with sane defaults func NewDefaultConfig() *Configuration { - namespace, err := kubernetes.Namespace() - if err != nil { - log.Warning("Unable to determine default namespace. Falling back to: default") - namespace = "default" - } return &Configuration{ - Namespace: namespace, Git: GitConfig{ CommitLimit: 0, RepoPath: ".", diff --git a/cmd/common.go b/cmd/common.go index aaa8d14..d3750ac 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -58,6 +58,12 @@ func toListOptions(labels []string) metav1.ListOptions { } } +func showUsageOnError(cmd *cobra.Command, err error) { + if err != nil { + cmd.Usage() + } +} + func missingLabelSelectorError(namespace, resource string) error { return fmt.Errorf("label selector with --label expected. You can print out available labels with \"kubectl -n %s get %s --show-labels\"", namespace, resource) } diff --git a/cmd/configmaps.go b/cmd/configmaps.go index 69b2f29..83248ca 100644 --- a/cmd/configmaps.go +++ b/cmd/configmaps.go @@ -7,6 +7,7 @@ import ( "github.com/appuio/seiso/pkg/kubernetes" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" + "strings" ) const ( @@ -22,24 +23,8 @@ var ( Aliases: []string{"configmap", "cm"}, Args: cobra.MaximumNArgs(1), SilenceUsage: true, - RunE: func(cmd *cobra.Command, args []string) error { - log.SetFormatter(&log.TextFormatter{DisableTimestamp: true}) - if err := validateConfigMapCommandInput(); err != nil { - cmd.Usage() - return err - } - - coreClient, err := kubernetes.NewCoreV1Client() - if err != nil { - return fmt.Errorf("cannot initiate kubernetes core client") - } - - configMapService := configmap.NewConfigMapsService( - coreClient.ConfigMaps(config.Namespace), - kubernetes.New(), - configmap.ServiceConfiguration{Batch: config.Log.Batch}) - return executeConfigMapCleanupCommand(configMapService) - }, + PreRunE: validateConfigMapCommandInput, + RunE: executeConfigMapCleanupCommand, } ) @@ -48,26 +33,42 @@ func init() { defaults := cfg.NewDefaultConfig() configMapCmd.PersistentFlags().BoolP("delete", "d", defaults.Delete, "Effectively delete ConfigMaps found") - configMapCmd.PersistentFlags().StringSliceP("label", "l", defaults.Resource.Labels, "Identify the ConfigMap by these labels") + configMapCmd.PersistentFlags().StringSliceP("label", "l", defaults.Resource.Labels, + "Identify the ConfigMap by these \"key=value\" labels") configMapCmd.PersistentFlags().IntP("keep", "k", defaults.History.Keep, "Keep most current ConfigMaps; does not include currently used ConfigMaps (if detected)") configMapCmd.PersistentFlags().String("older-than", defaults.Resource.OlderThan, "Delete ConfigMaps that are older than the duration, e.g. [1y2mo3w4d5h6m7s]") } -func validateConfigMapCommandInput() error { +func validateConfigMapCommandInput(cmd *cobra.Command, args []string) (returnErr error) { + defer showUsageOnError(cmd, returnErr) if len(config.Resource.Labels) == 0 { return missingLabelSelectorError(config.Namespace, "configmaps") } + for _, label := range config.Resource.Labels { + if !strings.Contains(label, "=") { + return fmt.Errorf("incorrect label format does not match expected \"key=value\" format: %s", label) + } + } if _, err := parseCutOffDateTime(config.Resource.OlderThan); err != nil { return fmt.Errorf("could not parse older-than flag: %w", err) } return nil } -func executeConfigMapCleanupCommand(service configmap.Service) error { +func executeConfigMapCleanupCommand(cmd *cobra.Command, args []string) error { + coreClient, err := kubernetes.NewCoreV1Client() + if err != nil { + return fmt.Errorf("cannot initiate kubernetes client: %w", err) + } + c := config.Resource namespace := config.Namespace + service := configmap.NewConfigMapsService( + coreClient.ConfigMaps(namespace), + kubernetes.New(), + configmap.ServiceConfiguration{Batch: config.Log.Batch}) log.WithField("namespace", namespace).Debug("Getting ConfigMaps") foundConfigMaps, err := service.List(toListOptions(c.Labels)) diff --git a/cmd/history.go b/cmd/history.go index aa69ef5..e139f72 100644 --- a/cmd/history.go +++ b/cmd/history.go @@ -2,7 +2,6 @@ package cmd import ( "fmt" - "github.com/appuio/seiso/cfg" "github.com/appuio/seiso/pkg/cleanup" "github.com/appuio/seiso/pkg/git" @@ -19,13 +18,8 @@ var ( Long: `Clean up excessive image tags matching the commit hashes (prefix) of the git repository`, Args: cobra.MaximumNArgs(1), SilenceUsage: true, - RunE: func(cmd *cobra.Command, args []string) error { - if err := validateHistoryCommandInput(args); err != nil { - cmd.Usage() - return err - } - return ExecuteHistoryCleanupCommand(args) - }, + PreRunE: validateHistoryCommandInput, + RunE: ExecuteHistoryCleanupCommand, } ) @@ -39,21 +33,28 @@ func init() { } -func validateHistoryCommandInput(args []string) error { +func validateHistoryCommandInput(cmd *cobra.Command, args []string) (returnErr error) { + defer showUsageOnError(cmd, returnErr) if len(args) == 0 { return missingImageNameError(config.Namespace) } - if _, _, err := splitNamespaceAndImagestream(args[0]); err != nil { + namespace, image, err := splitNamespaceAndImagestream(args[0]) + if err != nil { return fmt.Errorf("could not parse image name: %w", err) } if config.Git.Tag && !git.IsValidSortValue(config.Git.SortCriteria) { return fmt.Errorf("invalid sort flag provided: %v", config.Git.SortCriteria) } + log.WithFields(log.Fields{ + "namespace": namespace, + "image": image, + }).Debug("Using image config") + config.Namespace = namespace return nil } // ExecuteHistoryCleanupCommand executes the history cleanup command -func ExecuteHistoryCleanupCommand(args []string) error { +func ExecuteHistoryCleanupCommand(cmd *cobra.Command, args []string) error { c := config.History namespace, imageName, _ := splitNamespaceAndImagestream(args[0]) diff --git a/cmd/images.go b/cmd/images.go index 71c870d..445f1c0 100644 --- a/cmd/images.go +++ b/cmd/images.go @@ -24,10 +24,10 @@ func splitNamespaceAndImagestream(repo string) (namespace string, image string, } else { paths := strings.SplitAfter(repo, "/") if len(paths) >= 3 { - namespace = paths[1] + namespace = strings.TrimSuffix(paths[1], "/") image = paths[2] } else { - namespace = paths[0] + namespace = strings.TrimSuffix(paths[0], "/") image = paths[1] } } @@ -37,5 +37,5 @@ func splitNamespaceAndImagestream(repo string) (namespace string, image string, if image == "" { return "", "", errors.New("missing or invalid image name") } - return strings.TrimSuffix(namespace, "/"), image, nil + return namespace, image, nil } diff --git a/cmd/images_test.go b/cmd/images_test.go new file mode 100644 index 0000000..27e5a0a --- /dev/null +++ b/cmd/images_test.go @@ -0,0 +1,77 @@ +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_SplitNamespaceAndImagestream(t *testing.T) { + type args struct { + repo string + } + tests := []struct { + name string + args args + expectedNamespace string + expectedImage string + wantErr bool + }{ + { + name: "ShouldSplit_NamespaceAndImageName", + args: args{ + repo: "namespace/image", + }, + expectedNamespace: "namespace", + expectedImage: "image", + }, + { + name: "ShouldReturnActiveNamespace_IfRepoDoesNotContainNamespace", + args: args{ + repo: "image", + }, + expectedNamespace: "currently-active-ns", + expectedImage: "image", + }, + { + name: "ShouldThrowError_IfRepoDoesNotContainImage", + args: args{ + repo: "namespace/", + }, + wantErr: true, + }, + { + name: "ShouldThrowError_IfRepoIsInvalid", + args: args{ + repo: "/", + }, + wantErr: true, + }, + { + name: "ShouldThrowError_IfRepoIsEmpty", + args: args{}, + wantErr: true, + }, + { + name: "ShouldIgnore_Registry", + args: args{ + repo: "docker.io/namespace/image", + }, + expectedNamespace: "namespace", + expectedImage: "image", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config.Namespace = "currently-active-ns" + namespace, image, err := splitNamespaceAndImagestream(tt.args.repo) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, namespace, tt.expectedNamespace) + assert.Equal(t, image, tt.expectedImage) + }) + } +} diff --git a/cmd/orphans.go b/cmd/orphans.go index 685c94c..0873d38 100644 --- a/cmd/orphans.go +++ b/cmd/orphans.go @@ -31,13 +31,8 @@ var ( Aliases: []string{"orph", "orphan"}, Args: cobra.MaximumNArgs(1), SilenceUsage: true, - RunE: func(cmd *cobra.Command, args []string) error { - if err := validateOrphanCommandInput(args); err != nil { - cmd.Usage() - return err - } - return ExecuteOrphanCleanupCommand(args) - }, + PreRunE: validateOrphanCommandInput, + RunE: ExecuteOrphanCleanupCommand, } ) @@ -52,13 +47,15 @@ func init() { "Delete images that match the regex, defaults to matching Git SHA commits") } -func validateOrphanCommandInput(args []string) error { +func validateOrphanCommandInput(cmd *cobra.Command, args []string) (returnErr error) { + defer showUsageOnError(cmd, returnErr) if len(args) == 0 { return missingImageNameError(config.Namespace) } c := config.Orphan - if _, _, err := splitNamespaceAndImagestream(args[0]); err != nil { - return err + namespace, image, err := splitNamespaceAndImagestream(args[0]) + if err != nil { + return fmt.Errorf("could not parse image name: %w", err) } if _, err := parseOrphanDeletionRegex(c.OrphanDeletionRegex); err != nil { return fmt.Errorf("could not parse orphan deletion pattern: %w", err) @@ -71,11 +68,16 @@ func validateOrphanCommandInput(args []string) error { if config.Git.Tag && !git.IsValidSortValue(config.Git.SortCriteria) { return fmt.Errorf("invalid sort flag provided: %v", config.Git.SortCriteria) } + log.WithFields(log.Fields{ + "namespace": namespace, + "image": image, + }).Debug("Using image config") + config.Namespace = namespace return nil } // ExecuteOrphanCleanupCommand executes the orphan cleanup command -func ExecuteOrphanCleanupCommand(args []string) error { +func ExecuteOrphanCleanupCommand(cmd *cobra.Command, args []string) error { c := config.Orphan namespace, imageName, _ := splitNamespaceAndImagestream(args[0]) diff --git a/cmd/orphans_test.go b/cmd/orphans_test.go index eac493e..02e1178 100644 --- a/cmd/orphans_test.go +++ b/cmd/orphans_test.go @@ -1,6 +1,7 @@ package cmd import ( + "github.com/spf13/cobra" "regexp" "testing" @@ -8,76 +9,6 @@ import ( "github.com/stretchr/testify/assert" ) -func Test_splitNamespaceAndImagestream(t *testing.T) { - type args struct { - repo string - } - tests := []struct { - name string - args args - expectedNamespace string - expectedImage string - wantErr bool - }{ - { - name: "ShouldSplit_NamespaceAndImageName", - args: args{ - repo: "namespace/image", - }, - expectedNamespace: "namespace", - expectedImage: "image", - }, - { - name: "ShouldReturnActiveNamespace_IfRepoDoesNotContainNamespace", - args: args{ - repo: "image", - }, - expectedNamespace: "currently-active-ns", - expectedImage: "image", - }, - { - name: "ShouldThrowError_IfRepoDoesNotContainImage", - args: args{ - repo: "namespace/", - }, - wantErr: true, - }, - { - name: "ShouldThrowError_IfRepoIsInvalid", - args: args{ - repo: "/", - }, - wantErr: true, - }, - { - name: "ShouldThrowError_IfRepoIsEmpty", - args: args{}, - wantErr: true, - }, - { - name: "ShouldIgnore_Registry", - args: args{ - repo: "docker.io/namespace/image", - }, - expectedNamespace: "namespace", - expectedImage: "image", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config.Namespace = "currently-active-ns" - namespace, image, err := splitNamespaceAndImagestream(tt.args.repo) - if tt.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - assert.Equal(t, namespace, tt.expectedNamespace) - assert.Equal(t, image, tt.expectedImage) - }) - } -} - func Test_parseOrphanDeletionRegex(t *testing.T) { type args struct { orphanIncludeRegex string @@ -173,7 +104,7 @@ func Test_validateOrphanCommandInput(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { config = &tt.input.config - err := validateOrphanCommandInput(tt.input.args) + err := validateOrphanCommandInput(&cobra.Command{}, tt.input.args) if tt.wantErr { assert.Error(t, err) return diff --git a/cmd/root.go b/cmd/root.go index 935135a..b3277f6 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -1,6 +1,8 @@ package cmd import ( + "fmt" + "github.com/appuio/seiso/pkg/kubernetes" "os" "strings" @@ -16,12 +18,13 @@ import ( var ( // rootCmd represents the base command when called without any subcommands rootCmd = &cobra.Command{ - Use: "seiso", - Short: "Keeps your Kubernetes projects clean", - PersistentPreRun: parseConfig, + Use: "seiso", + Short: "Keeps your Kubernetes projects clean", + PersistentPreRunE: parseConfig, } config = cfg.NewDefaultConfig() koanfInstance = koanf.New(".") + version = "undefined" ) // Execute is the main entrypoint of the CLI, it executes child commands as given by the user-defined flags and arguments. @@ -32,7 +35,7 @@ func Execute() error { func init() { rootCmd.PersistentFlags().StringP("namespace", "n", config.Namespace, "Cluster namespace of current context") rootCmd.PersistentFlags().String("log.level", config.Log.LogLevel, "Log level, one of [debug info warn error fatal]") - rootCmd.PersistentFlags().BoolP("log.verbose", "v", config.Log.Verbose, "Shorthand for --log.level debug") + rootCmd.PersistentFlags().BoolP("log.verbose", "v", config.Log.Verbose, "Shorthand for \"--log.level debug\"") rootCmd.PersistentFlags().BoolP("log.batch", "b", config.Log.Batch, "Use Batch mode (Prints error to StdErr, StdOut is used to just print resource names, useful for piping)") cobra.OnInitialize(initRootConfig) @@ -43,13 +46,13 @@ func initRootConfig() { } // parseConfig reads the flags and ENV vars -func parseConfig(cmd *cobra.Command, args []string) { +func parseConfig(cmd *cobra.Command, args []string) error { loadEnvironmentVariables() bindFlags(cmd.PersistentFlags()) if err := koanfInstance.Unmarshal("", &config); err != nil { - log.WithError(err).Fatal("Could not read config") + return fmt.Errorf("could not read config: %w", err) } log.SetFormatter(&log.TextFormatter{ @@ -72,6 +75,14 @@ func parseConfig(cmd *cobra.Command, args []string) { } else { log.SetLevel(level) } + if config.Namespace == "" { + namespace, err := kubernetes.Namespace() + if err != nil { + return fmt.Errorf("unable to determine default namespace from Kubeconfig and --namespace not given: %w", err) + } + config.Namespace = namespace + } + log.Infof("Seiso %s", version) log.WithFields(log.Fields{ "namespace": config.Namespace, "git": config.Git, @@ -80,6 +91,7 @@ func parseConfig(cmd *cobra.Command, args []string) { "orphan": config.Orphan, "resource": config.Resource, }).Debug("Using config") + return nil } func loadEnvironmentVariables() { @@ -108,6 +120,8 @@ func bindFlags(flagSet *pflag.FlagSet) { } // SetVersion sets the version string in the help messages -func SetVersion(version string) { - rootCmd.Version = version +func SetVersion(v string) { + // We need to set both properties in order to break an initialization loop + rootCmd.Version = v + version = v } diff --git a/cmd/secrets.go b/cmd/secrets.go index 5da1a6e..a9abde6 100644 --- a/cmd/secrets.go +++ b/cmd/secrets.go @@ -7,6 +7,7 @@ import ( "github.com/appuio/seiso/pkg/secret" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" + "strings" ) const ( @@ -22,23 +23,8 @@ var ( Aliases: []string{"secret"}, Args: cobra.MaximumNArgs(1), SilenceUsage: true, - RunE: func(cmd *cobra.Command, args []string) error { - if err := validateSecretCommandInput(); err != nil { - cmd.Usage() - return err - } - - coreClient, err := kubernetes.NewCoreV1Client() - if err != nil { - return fmt.Errorf("cannot initiate kubernetes client") - } - - secretService := secret.NewSecretsService( - coreClient.Secrets(config.Namespace), - kubernetes.New(), - secret.ServiceConfiguration{Batch: config.Log.Batch}) - return executeSecretCleanupCommand(secretService) - }, + PreRunE: validateSecretCommandInput, + RunE: executeSecretCleanupCommand, } ) @@ -47,26 +33,42 @@ func init() { defaults := cfg.NewDefaultConfig() secretCmd.PersistentFlags().BoolP("delete", "d", defaults.Delete, "Effectively delete Secrets found") - secretCmd.PersistentFlags().StringSliceP("label", "l", defaults.Resource.Labels, "Identify the Secrets by these labels") + secretCmd.PersistentFlags().StringSliceP("label", "l", defaults.Resource.Labels, + "Identify the Secrets by these \"key=value\" labels") secretCmd.PersistentFlags().IntP("keep", "k", defaults.History.Keep, "Keep most current Secrets; does not include currently used secret (if detected)") secretCmd.PersistentFlags().String("older-than", defaults.Resource.OlderThan, "Delete Secrets that are older than the duration, e.g. [1y2mo3w4d5h6m7s]") } -func validateSecretCommandInput() error { +func validateSecretCommandInput(cmd *cobra.Command, args []string) (returnErr error) { + defer showUsageOnError(cmd, returnErr) if len(config.Resource.Labels) == 0 { return missingLabelSelectorError(config.Namespace, "secrets") } + for _, label := range config.Resource.Labels { + if !strings.Contains(label, "=") { + return fmt.Errorf("incorrect label format does not match expected \"key=value\" format: %s", label) + } + } if _, err := parseCutOffDateTime(config.Resource.OlderThan); err != nil { return fmt.Errorf("could not parse older-than flag: %w", err) } return nil } -func executeSecretCleanupCommand(service secret.Service) error { +func executeSecretCleanupCommand(cmd *cobra.Command, args []string) error { + coreClient, err := kubernetes.NewCoreV1Client() + if err != nil { + return fmt.Errorf("cannot initiate kubernetes client: %w", err) + } + c := config.Resource namespace := config.Namespace + service := secret.NewSecretsService( + coreClient.Secrets(namespace), + kubernetes.New(), + secret.ServiceConfiguration{Batch: config.Log.Batch}) log.WithField("namespace", namespace).Debug("Getting Secrets") foundSecrets, err := service.List(toListOptions(c.Labels))