Small improvements on package command (#1038)

Co-authored-by: kensipe <[email protected]>

pkg/kudoctl/cmd/package.go

@@ -8,7 +8,7 @@ import (
 )
 
 const packageDesc = `
-This command consists of multiple sub-commands to interact with KUDO operators.
+This command consists of multiple sub-commands to interact with KUDO packages.
 
 It can be used to package or verify an operator, or list parameters.  When working with parameters it can 
 provide a list of parameters from a remote operator given a url or repository along with the name and version.
@@ -22,7 +22,7 @@ const packageExamples = `  kubectl kudo package create [operator folder]
 // newPackageCmd for operator commands such as packaging an operator or retrieving it's parameters
 func newPackageCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "package [FLAGS] package|params|verify [ARGS]",
+		Use:     "package [SUBCOMMAND] [FLAGS] [ARGS]",
 		Short:   "package an operator, or understand it's content",
 		Long:    packageDesc,
 		Example: packageExamples,

pkg/kudoctl/cmd/package_params.go

@@ -8,9 +8,7 @@ import (
 )
 
 const paramsDesc = `
-This command consists of multiple sub-commands to interact with KUDO parameters when working on creating or modifying an operator.
-
-It can be used to list operator properties
+List operator parameters
 `
 
 const paramsExamples = `  kubectl kudo package params list [operator folder]

pkg/kudoctl/cmd/package_params_list.go

@@ -23,20 +23,27 @@ type paramsListCmd struct {
 	path           string
 	descriptions   bool
 	namesOnly      bool
-	required       bool
+	requiredOnly   bool
 	RepoName       string
 	PackageVersion string
 }
 
+const (
+	pkgParamsExample = `# show parameters from local-folder (where local-folder is a folder in the current directory)
+  kubectl kudo package params list local-folder
+
+  # show parameters from zookeeper (where zookeeper is name of package in KUDO repository)
+  kubectl kudo package params list zookeeper`
+)
+
 func newParamsListCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 	list := &paramsListCmd{fs: fs, out: out}
 
 	cmd := &cobra.Command{
 		Use:     "list [operator]",
 		Short:   "List operator parameters",
-		Example: "  kubectl kudo package params list",
+		Example: pkgParamsExample,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			//list.home = Settings.Home
 			if err := validateOperatorArg(args); err != nil {
 				return err
 			}
@@ -47,20 +54,22 @@ func newParamsListCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 
 	f := cmd.Flags()
 	f.BoolVarP(&list.descriptions, "descriptions", "d", false, "Display descriptions.")
-	f.BoolVarP(&list.required, "required", "r", false, "Restricts list to params which have no defaults but are required.")
-	f.BoolVar(&list.namesOnly, "names-only", false, "Display only names.")
+	f.BoolVarP(&list.requiredOnly, "required", "r", false, "Show only parameters which have no defaults but are required.")
+	f.BoolVar(&list.namesOnly, "names", false, "Display only names.")
 	f.StringVar(&list.RepoName, "repo", "", "Name of repository configuration to use. (default defined by context)")
-	f.StringVar(&list.PackageVersion, "version", "", "A specific package version on the official GitHub repo. (default to the most recent)")
+	f.StringVarP(&list.PackageVersion, "version", "v", "", "A specific package version on the official GitHub repo. (default to the most recent)")
 
 	return cmd
 }
 
 // run provides a table listing the parameters.  There are 3 defined ways to view the table
-// 1. names only using --names-only.  This is based on challenges with other approaches reading really long parameter names
+// 1. names only using --names.  This is based on challenges with other approaches reading really long parameter names
 // 2. name, default and required.  This is the **default**
 // 3. name, default, required, desc.
 func (c *paramsListCmd) run(fs afero.Fs, settings *env.Settings) error {
-
+	if !onlyOneSet(c.requiredOnly, c.namesOnly, c.descriptions) {
+		return fmt.Errorf("only one of the flags 'required', 'names', 'descriptions' can be set")
+	}
 	repository, err := repo.ClientFromSettings(fs, settings.Home, c.RepoName)
 	if err != nil {
 		return errors.WithMessage(err, "could not build operator repository")
@@ -81,7 +90,7 @@ func displayParamsTable(pf *packages.PackageFiles, cmd *paramsListCmd) error {
 	table := uitable.New()
 	tValue := true
 	// required
-	if cmd.required {
+	if cmd.requiredOnly {
 		table.AddRow("Name")
 		found := false
 		for _, p := range pf.Params {
@@ -109,7 +118,6 @@ func displayParamsTable(pf *packages.PackageFiles, cmd *paramsListCmd) error {
 	table.MaxColWidth = 35
 	table.Wrap = true
 	if cmd.descriptions {
-		//table.MaxColWidth = 50
 		table.AddRow("Name", "Default", "Required", "Descriptions")
 
 	} else {
@@ -130,3 +138,11 @@ func displayParamsTable(pf *packages.PackageFiles, cmd *paramsListCmd) error {
 	fmt.Fprintln(cmd.out, table)
 	return nil
 }
+
+func onlyOneSet(b bool, b2 bool, b3 bool) bool {
+	// all false is ok all other combos need to verify only 1
+	if !b && !b2 && !b3 {
+		return true
+	}
+	return (b && !b2 && !b3) || (!b && b2 && !b3) || (!b && !b2 && b3)
+}

pkg/kudoctl/cmd/package_verify.go

@@ -24,7 +24,7 @@ func newPackageVerifyCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 
 	cmd := &cobra.Command{
 		Use:     "verify [package]",
-		Short:   "verify operator parameters",
+		Short:   "verify package parameters",
 		Example: "  kubectl kudo package verify ../zk/operator",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if err := validateOperatorArg(args); err != nil {
@@ -48,25 +48,23 @@ func (c *packageVerifyCmd) run(fs afero.Fs, path string) error {
 	if warnings != nil {
 		printWarnings(c.out, warnings)
 	}
-	if errors != nil {
-		printErrors(c.out, errors)
-		return fmt.Errorf("operator verification errors: %v", len(errors))
-	}
-	if warnings == nil && errors == nil {
-		fmt.Fprintf(c.out, "operator is valid\n")
+	if errors == nil {
+		fmt.Fprintf(c.out, "package is valid\n")
+		return nil
 	}
 
+	printErrors(c.out, errors)
+	return fmt.Errorf("package verification errors: %v", len(errors))
 	//TODO (kensipe): add linting
 	// 2. warning on params not used
 	// 3. error on param in template not defined
-	return nil
 }
 
 func printErrors(out io.Writer, errors verify.ParamErrors) {
 	table := uitable.New()
 	table.AddRow("Errors")
-	for _, warning := range errors {
-		table.AddRow(warning)
+	for _, err := range errors {
+		table.AddRow(err)
 	}
 	fmt.Fprintln(out, table)
 }

pkg/kudoctl/cmd/testdata/invalid-params.golden

@@ -1,3 +1,3 @@
-Errors                                       
-parameter "Cpus" has a duplicate             
-parameter "comma," has a the invalid char ','
+Errors                                           
+parameter "Cpus" has a duplicate                 
+parameter "comma," contains invalid character ','

pkg/kudoctl/cmd/verify/verify.go

@@ -20,7 +20,6 @@ var verifiers = []ParameterVerifier{
 
 // Parameters verifies parameters
 func Parameters(params packages.Params) (warnings ParamWarnings, errors ParamErrors) {
-
 	for _, verifier := range verifiers {
 		w, err := verifier.Verify(params)
 		warnings = append(warnings, w...)
@@ -59,12 +58,11 @@ type InvalidCharVerifier struct {
 }
 
 func (v InvalidCharVerifier) Verify(params packages.Params) (warnings ParamWarnings, errors ParamErrors) {
-
 	for _, param := range params {
 		name := strings.ToLower(param.Name)
 		for _, char := range name {
 			if strings.Contains(v.InvalidChars, strings.ToLower(string(char))) {
-				errors = append(errors, CreateParamError(param, fmt.Sprintf("has a the invalid char %q", char)))
+				errors = append(errors, CreateParamError(param, fmt.Sprintf("contains invalid character %q", char)))
 			}
 		}
 

pkg/kudoctl/cmd/verify/verify_test.go

@@ -8,69 +8,56 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestDuplicateVerifier_NoErrorVerify(t *testing.T) {
-
-	goodParams := []v1beta1.Parameter{
-		{Name: "Foo"},
-		{Name: "Fighters"},
+func TestDuplicateVerifier(t *testing.T) {
+	tests := []struct {
+		name             string
+		params           []v1beta1.Parameter
+		expectedWarnings ParamWarnings
+		expectedErrors   ParamErrors
+	}{
+		{"no warning or error", []v1beta1.Parameter{
+			{Name: "Foo"},
+			{Name: "Fighters"},
+		}, nil, nil},
+		{"duplicate parameter", []v1beta1.Parameter{
+			{Name: "Foo"},
+			{Name: "Foo"},
+		}, nil, []ParamError{ParamError(fmt.Sprintf("parameter \"Foo\" has a duplicate"))}},
+		{"duplicate with different casing", []v1beta1.Parameter{
+			{Name: "Foo"},
+			{Name: "foo"},
+		}, nil, ParamErrors{ParamError(fmt.Sprintf("parameter \"foo\" has a duplicate"))}},
 	}
 
 	verifier := DuplicateVerifier{}
-
-	warnings, errors := verifier.Verify(goodParams)
-	assert.Nil(t, warnings)
-	assert.Nil(t, errors)
-}
-
-func TestDuplicateVerifier_ErrorVerify(t *testing.T) {
-
-	dupParams := []v1beta1.Parameter{
-		{Name: "Foo"},
-		{Name: "Foo"},
+	for _, tt := range tests {
+		warnings, errors := verifier.Verify(tt.params)
+		assert.Equal(t, tt.expectedWarnings, warnings)
+		assert.Equal(t, tt.expectedErrors, errors)
 	}
-
-	verifier := DuplicateVerifier{}
-
-	warnings, errors := verifier.Verify(dupParams)
-	assert.Nil(t, warnings)
-	assert.Equal(t, ParamError(fmt.Sprintf("parameter \"Foo\" has a duplicate")), errors[0])
 }
 
-func TestDuplicateVerifier_CaseErrorVerify(t *testing.T) {
-
-	dupParams := []v1beta1.Parameter{
-		{Name: "Foo"},
-		{Name: "foo"},
-	}
-
-	verifier := DuplicateVerifier{}
-
-	warnings, errors := verifier.Verify(dupParams)
-	assert.Nil(t, warnings)
-	assert.Equal(t, ParamError(fmt.Sprintf("parameter \"foo\" has a duplicate")), errors[0])
-}
-
-func TestInvalidCharVerifier_GoodVerify(t *testing.T) {
-	params := []v1beta1.Parameter{
-		{Name: "Foo"},
-		{Name: "Fighters"},
+func TestInvalidCharVerifier(t *testing.T) {
+	tests := []struct {
+		name             string
+		params           []v1beta1.Parameter
+		expectedWarnings ParamWarnings
+		expectedErrors   ParamErrors
+	}{
+		{"no warning or error", []v1beta1.Parameter{
+			{Name: "Foo"},
+			{Name: "Fighters"},
+		}, nil, nil},
+		{"invalid character", []v1beta1.Parameter{
+			{Name: "Foo:"},
+			{Name: "Fighters,"},
+		}, nil, []ParamError{ParamError("parameter \"Foo:\" contains invalid character ':'"), ParamError("parameter \"Fighters,\" contains invalid character ','")}},
 	}
 
 	verifier := InvalidCharVerifier{InvalidChars: ":,"}
-	warnings, errors := verifier.Verify(params)
-	assert.Nil(t, warnings)
-	assert.Nil(t, errors)
-}
-
-func TestInvalidCharVerifier_BadVerify(t *testing.T) {
-	params := []v1beta1.Parameter{
-		{Name: "Foo:"},
-		{Name: "Fighters,"},
+	for _, tt := range tests {
+		warnings, errors := verifier.Verify(tt.params)
+		assert.Equal(t, tt.expectedWarnings, warnings, tt.name)
+		assert.Equal(t, tt.expectedErrors, errors, tt.name)
 	}
-
-	verifier := InvalidCharVerifier{InvalidChars: ":,"}
-	warnings, errors := verifier.Verify(params)
-	assert.Nil(t, warnings)
-	assert.Equal(t, 2, len(errors))
-	assert.Equal(t, ParamError("parameter \"Foo:\" has a the invalid char ':'"), errors[0])
 }