Merge pull request #20 from jace-ys/jace-ys/fix-renew-flags-validation

Fix `cmctl renew` flags validation

pkg/renew/renew.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
@@ -96,24 +97,37 @@ func NewCmdRenew(ctx context.Context, ioStreams genericclioptions.IOStreams) *co
 
 // Validate validates the provided options
 func (o *Options) Validate(cmd *cobra.Command, args []string) error {
-	if len(o.LabelSelector) > 0 && len(args) > 0 {
-		return errors.New("cannot specify Certificate names in conjunction with label selectors")
+	// The --all, --selector and args are mutually exclusive.
+	//   - --all is equivalent to a match-all label selector
+	//   - --selector filters by label selector
+	//   - args are an explicit list of Certificate names
+	// However, there must always be one of the three specified.
+
+	var flags []string
+	if len(args) > 0 {
+		flags = append(flags, fmt.Sprintf("the Certificate resource names %q", args))
 	}
-
-	if len(o.LabelSelector) > 0 && o.All {
-		return errors.New("cannot specify label selectors in conjunction with --all flag")
+	if o.All {
+		flags = append(flags, "the --all flag")
+	}
+	if len(o.LabelSelector) > 0 {
+		flags = append(flags, "a label selector")
 	}
 
-	if o.All && len(args) > 0 {
-		return errors.New("cannot specify Certificate names in conjunction with --all flag")
+	// Ensure that only one of the three flags are specified
+	if len(flags) > 1 {
+		return fmt.Errorf("cannot specify %s in conjunction with %s", flags[0], strings.Join(flags[1:], " and "))
 	}
 
-	if o.All && cmd.PersistentFlags().Changed("namespace") {
-		return errors.New("cannot specify --namespace flag in conjunction with --all flag")
+	if len(flags) == 0 {
+		return errors.New("please either supply one or more Certificate resource names, label selectors, or use the --all flag to renew all Certificate resources")
 	}
 
-	if !o.All && len(args) == 0 {
-		return errors.New("please supply one or more Certificate resource names or use the --all flag to renew all Certificate resources")
+	// The --all-namespaces flag overrides the --namespace flag.
+	// Additionally, we can only use --all-namespaces when not specifying a list of Certificate names
+
+	if o.AllNamespaces && len(args) > 0 {
+		return errors.New("cannot specify Certificate names in conjunction with --all-namespaces flag")
 	}
 
 	return nil

pkg/renew/renew_test.go

@@ -41,12 +41,18 @@ func TestValidate(t *testing.T) {
 			args:   []string{"abc"},
 			expErr: true,
 		},
+		"If there are arguments, as well as --all-namespaces, error": {
+			options: &Options{
+				AllNamespaces: true,
+			},
+			args:   []string{"abc"},
+			expErr: true,
+		},
 		"If there are all certificates selected, as well as label selector, error": {
 			options: &Options{
 				LabelSelector: "foo=bar",
 				All:           true,
 			},
-			args:   []string{""},
 			expErr: true,
 		},
 		"If there are all certificates selected, as well as arguments, error": {
@@ -63,14 +69,14 @@ func TestValidate(t *testing.T) {
 			},
 			expErr: false,
 		},
-		"If --namespace and --all namespace specified, error": {
+		"If --namespace and --all specified, don't error": {
 			options: &Options{
 				All: true,
 			},
 			setStringFlags: []stringFlag{
 				{name: "namespace", value: "foo"},
 			},
-			expErr: true,
+			expErr: false,
 		},
 		"If --namespace specified without arguments, error": {
 			options: &Options{},
@@ -95,6 +101,29 @@ func TestValidate(t *testing.T) {
 			},
 			expErr: false,
 		},
+		"If --label-selector specified with --all, error": {
+			options: &Options{
+				LabelSelector: "foo=bar",
+				All:           true,
+			},
+			expErr: true,
+		},
+		"If --label-selector specified with --all-namespaces, don't error": {
+			options: &Options{
+				AllNamespaces: true,
+				LabelSelector: "foo=bar",
+			},
+			expErr: false,
+		},
+		"If --label-selector specified with --namespace, don't error": {
+			options: &Options{
+				LabelSelector: "foo=bar",
+			},
+			setStringFlags: []stringFlag{
+				{name: "namespace", value: "foo"},
+			},
+			expErr: false,
+		},
 	}
 
 	for name, test := range tests {