UX Improvements: fixes and improvements (#190)

* add default to project list

* project list: improve implementation, add testing

* minor improvements to code

* fix linting

* change keyring testing

* address comments in PR

* address PR comments

* generate docs

* Update internal/pkg/args/args.go

Co-authored-by: João Palet <joao.palet@outlook.com>

* fix linting

---------

Co-authored-by: João Palet <joao.palet@outlook.com>

Author: DiogoFerrao

docs/stackit_project_list.md

@@ -13,7 +13,7 @@ stackit project list [flags]
 ### Examples
 
 ```
-  List all STACKIT projects that authenticated user is a member of
+  List all STACKIT projects that the authenticated user or service account is a member of
   $ stackit project list
 
   List all STACKIT projects that are children of a specific parent

internal/cmd/dns/record-set/describe/describe.go

@@ -107,7 +107,7 @@ func outputResult(cmd *cobra.Command, outputFormat string, recordSet *dns.Record
 		for _, r := range *recordSet.Records {
 			recordsData = append(recordsData, *r.Content)
 		}
-		recordsDataJoin := strings.Join(recordsData, ",")
+		recordsDataJoin := strings.Join(recordsData, ", ")
 
 		table := tables.NewTable()
 		table.AddRow("ID", *recordSet.Id)

internal/cmd/project/describe/describe.go

@@ -6,23 +6,26 @@ import (
 	"fmt"
 
 	"github.com/stackitcloud/stackit-cli/internal/pkg/args"
-	"github.com/stackitcloud/stackit-cli/internal/pkg/errors"
 	"github.com/stackitcloud/stackit-cli/internal/pkg/examples"
 	"github.com/stackitcloud/stackit-cli/internal/pkg/flags"
 	"github.com/stackitcloud/stackit-cli/internal/pkg/globalflags"
 	"github.com/stackitcloud/stackit-cli/internal/pkg/services/resourcemanager/client"
 	"github.com/stackitcloud/stackit-cli/internal/pkg/tables"
+	"github.com/stackitcloud/stackit-cli/internal/pkg/utils"
 
 	"github.com/spf13/cobra"
 	"github.com/stackitcloud/stackit-sdk-go/services/resourcemanager"
 )
 
 const (
 	includeParentsFlag = "include-parents"
+
+	projectIdArg = "PROJECT_ID"
 )
 
 type inputModel struct {
 	*globalflags.GlobalFlagModel
+	ArgProjectId   string
 	IncludeParents bool
 }
 
@@ -31,7 +34,7 @@ func NewCmd() *cobra.Command {
 		Use:   "describe",
 		Short: "Shows details of a STACKIT project",
 		Long:  "Shows details of a STACKIT project.",
-		Args:  args.NoArgs,
+		Args:  args.SingleOptionalArg(projectIdArg, utils.ValidateUUID),
 		Example: examples.Build(
 			examples.NewExample(
 				`Get the details of the configured STACKIT project`,
@@ -45,7 +48,7 @@ func NewCmd() *cobra.Command {
 		),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			ctx := context.Background()
-			model, err := parseInput(cmd)
+			model, err := parseInput(cmd, args)
 			if err != nil {
 				return err
 			}
@@ -74,20 +77,30 @@ func configureFlags(cmd *cobra.Command) {
 	cmd.Flags().Bool(includeParentsFlag, false, "When true, the details of the parent resources will be included in the output")
 }
 
-func parseInput(cmd *cobra.Command) (*inputModel, error) {
+func parseInput(cmd *cobra.Command, inputArgs []string) (*inputModel, error) {
+	var projectId string
+	if len(inputArgs) > 0 {
+		projectId = inputArgs[0]
+	}
+
 	globalFlags := globalflags.Parse(cmd)
-	if globalFlags.ProjectId == "" {
-		return nil, &errors.ProjectIdError{}
+	if globalFlags.ProjectId == "" && projectId == "" {
+		return nil, fmt.Errorf("Project ID needs to be provided either as an argument or as a flag")
+	}
+
+	if projectId == "" {
+		projectId = globalFlags.ProjectId
 	}
 
 	return &inputModel{
 		GlobalFlagModel: globalFlags,
+		ArgProjectId:    projectId,
 		IncludeParents:  flags.FlagToBoolValue(cmd, includeParentsFlag),
 	}, nil
 }
 
 func buildRequest(ctx context.Context, model *inputModel, apiClient *resourcemanager.APIClient) resourcemanager.ApiGetProjectRequest {
-	req := apiClient.GetProject(ctx, model.ProjectId)
+	req := apiClient.GetProject(ctx, model.ArgProjectId)
 	req.IncludeParents(model.IncludeParents)
 	return req
 }

internal/cmd/project/describe/describe_test.go

@@ -19,10 +19,20 @@ type testCtxKey struct{}
 var testCtx = context.WithValue(context.Background(), testCtxKey{}, "foo")
 var testClient = &resourcemanager.APIClient{}
 var testProjectId = uuid.NewString()
+var testProjectId2 = uuid.NewString()
+
+func fixtureArgValues(mods ...func(argValues []string)) []string {
+	argValues := []string{
+		testProjectId,
+	}
+	for _, mod := range mods {
+		mod(argValues)
+	}
+	return argValues
+}
 
 func fixtureFlagValues(mods ...func(flagValues map[string]string)) map[string]string {
 	flagValues := map[string]string{
-		projectIdFlag:      testProjectId,
 		includeParentsFlag: "false",
 	}
 	for _, mod := range mods {
@@ -34,9 +44,10 @@ func fixtureFlagValues(mods ...func(flagValues map[string]string)) map[string]st
 func fixtureInputModel(mods ...func(model *inputModel)) *inputModel {
 	model := &inputModel{
 		GlobalFlagModel: &globalflags.GlobalFlagModel{
-			ProjectId: testProjectId,
+			ProjectId: "",
 		},
 		IncludeParents: false,
+		ArgProjectId:   testProjectId,
 	}
 	for _, mod := range mods {
 		mod(model)
@@ -55,24 +66,50 @@ func fixtureRequest(mods ...func(request *resourcemanager.ApiGetProjectRequest))
 func TestParseInput(t *testing.T) {
 	tests := []struct {
 		description   string
+		argValues     []string
 		flagValues    map[string]string
 		labelValues   []string
 		isValid       bool
 		expectedModel *inputModel
 	}{
 		{
 			description:   "base",
+			argValues:     fixtureArgValues(),
 			flagValues:    fixtureFlagValues(),
 			isValid:       true,
 			expectedModel: fixtureInputModel(),
 		},
 		{
 			description: "no values",
+			argValues:   []string{},
 			flagValues:  map[string]string{},
 			isValid:     false,
 		},
+		{
+			description: "project id arg takes precedence",
+			argValues:   fixtureArgValues(),
+			flagValues: fixtureFlagValues(func(flagValues map[string]string) {
+				flagValues[projectIdFlag] = testProjectId2
+			}),
+			isValid: true,
+			expectedModel: fixtureInputModel(func(model *inputModel) {
+				model.ProjectId = testProjectId2
+			}),
+		},
+		{
+			description: "project id arg missing",
+			argValues:   []string{},
+			flagValues: fixtureFlagValues(func(flagValues map[string]string) {
+				flagValues[projectIdFlag] = testProjectId
+			}),
+			isValid: true,
+			expectedModel: fixtureInputModel(func(model *inputModel) {
+				model.ProjectId = testProjectId
+			}),
+		},
 		{
 			description: "project id missing",
+			argValues:   []string{},
 			flagValues: fixtureFlagValues(func(flagValues map[string]string) {
 				delete(flagValues, projectIdFlag)
 			}),
@@ -112,6 +149,14 @@ func TestParseInput(t *testing.T) {
 				}
 			}
 
+			err = cmd.ValidateArgs(tt.argValues)
+			if err != nil {
+				if !tt.isValid {
+					return
+				}
+				t.Fatalf("error validating args: %v", err)
+			}
+
 			err = cmd.ValidateRequiredFlags()
 			if err != nil {
 				if !tt.isValid {
@@ -120,7 +165,7 @@ func TestParseInput(t *testing.T) {
 				t.Fatalf("error validating flags: %v", err)
 			}
 
-			model, err := parseInput(cmd)
+			model, err := parseInput(cmd, tt.argValues)
 			if err != nil {
 				if !tt.isValid {
 					return

internal/cmd/project/list/list.go

@@ -49,7 +49,7 @@ func NewCmd() *cobra.Command {
 		Args:  args.NoArgs,
 		Example: examples.Build(
 			examples.NewExample(
-				`List all STACKIT projects that the authenticated user is a member of`,
+				`List all STACKIT projects that the authenticated user or service account is a member of`,
 				"$ stackit project list"),
 			examples.NewExample(
 				`List all STACKIT projects that are children of a specific parent`,

internal/pkg/args/args.go

@@ -42,3 +42,28 @@ func SingleArg(argName string, validate func(value string) error) cobra.Position
 		return nil
 	}
 }
+
+// SingleOptionalArg checks if one or no arguments were provided and validates it if provided
+// using the validate function. It returns an error if more than one argument is provided, or if
+// the argument is invalid. For no validation, you can pass a nil validate function
+func SingleOptionalArg(argName string, validate func(value string) error) cobra.PositionalArgs {
+	return func(cmd *cobra.Command, args []string) error {
+		if len(args) > 1 {
+			return &errors.SingleOptionalArgExpectedError{
+				Cmd:      cmd,
+				Expected: argName,
+				Count:    len(args),
+			}
+		}
+		if len(args) == 1 && validate != nil {
+			err := validate(args[0])
+			if err != nil {
+				return &errors.ArgValidationError{
+					Arg:     argName,
+					Details: err.Error(),
+				}
+			}
+		}
+		return nil
+	}
+}

internal/pkg/args/args_test.go

@@ -107,3 +107,74 @@ func TestSingleArg(t *testing.T) {
 		})
 	}
 }
+
+func TestSingleOptionalArg(t *testing.T) {
+	tests := []struct {
+		description  string
+		args         []string
+		validateFunc func(value string) error
+		isValid      bool
+	}{
+		{
+			description: "valid",
+			args:        []string{"arg"},
+			validateFunc: func(value string) error {
+				return nil
+			},
+			isValid: true,
+		},
+		{
+			description: "no_arg",
+			args:        []string{},
+			isValid:     true,
+		},
+		{
+			description: "more_than_one_arg",
+			args:        []string{"arg", "arg2"},
+			isValid:     false,
+		},
+		{
+			description: "empty_arg",
+			args:        []string{""},
+			isValid:     true,
+		},
+		{
+			description: "invalid_arg",
+			args:        []string{"arg"},
+			validateFunc: func(value string) error {
+				return fmt.Errorf("error")
+			},
+			isValid: false,
+		},
+		{
+			description:  "nil validation function",
+			args:         []string{"arg"},
+			validateFunc: nil,
+			isValid:      true,
+		},
+		{
+			description:  "nil validation function, no args",
+			args:         []string{},
+			validateFunc: nil,
+			isValid:      true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			cmd := &cobra.Command{
+				Use:   "test",
+				Short: "Test command",
+			}
+
+			argFunction := SingleOptionalArg("test", tt.validateFunc)
+			err := argFunction(cmd, tt.args)
+
+			if tt.isValid && err != nil {
+				t.Fatalf("should not have failed: %v", err)
+			}
+			if !tt.isValid && err == nil {
+				t.Fatalf("should have failed")
+			}
+		})
+	}
+}

internal/pkg/errors/errors.go

@@ -107,6 +107,8 @@ For more details on the available storages for the configured flavor (%[3]s), ru
 
 	SINGLE_ARG_EXPECTED = `expected 1 argument %q, %d were provided`
 
+	SINGLE_OPTIONAL_ARG_EXPECTED = `expected no more than 1 argument %q, %d were provided`
+
 	SUBCOMMAND_UNKNOWN = `unknown subcommand %q`
 
 	SUBCOMMAND_MISSING = `missing subcommand`
@@ -260,6 +262,17 @@ func (e *SingleArgExpectedError) Error() string {
 	return AppendUsageTip(err, e.Cmd).Error()
 }
 
+type SingleOptionalArgExpectedError struct {
+	Cmd      *cobra.Command
+	Expected string
+	Count    int
+}
+
+func (e *SingleOptionalArgExpectedError) Error() string {
+	err := fmt.Errorf(SINGLE_OPTIONAL_ARG_EXPECTED, e.Expected, e.Count)
+	return AppendUsageTip(err, e.Cmd).Error()
+}
+
 // Used when an unexpected non-flag input (either arg or subcommand) is found
 type InputUnknownError struct {
 	ProvidedInput string