From e038ed8ff558535f34f802a3fe1e3704180d4545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BCdiger=20Schmitz?= <152157960+bahkauv70@users.noreply.github.com> Date: Fri, 21 Feb 2025 09:57:55 +0100 Subject: [PATCH 1/2] fix: fix potential nil-pointer exceptions and add testcase --- internal/cmd/project/create/create.go | 9 ++- internal/cmd/project/create/create_test.go | 33 +++++++++-- internal/cmd/project/describe/describe.go | 7 ++- .../cmd/project/describe/describe_test.go | 39 +++++++++++- internal/cmd/project/list/list.go | 7 ++- internal/cmd/project/list/list_test.go | 59 +++++++++++++++++-- internal/cmd/project/member/list/list.go | 10 ++-- internal/cmd/project/member/list/list_test.go | 47 +++++++++++++-- internal/cmd/project/role/list/list_test.go | 41 +++++++++++-- 9 files changed, 222 insertions(+), 30 deletions(-) diff --git a/internal/cmd/project/create/create.go b/internal/cmd/project/create/create.go index 19d20beed..50173875d 100644 --- a/internal/cmd/project/create/create.go +++ b/internal/cmd/project/create/create.go @@ -95,7 +95,7 @@ func NewCmd(p *print.Printer) *cobra.Command { return fmt.Errorf("create project: %w", err) } - return outputResult(p, model, resp) + return outputResult(p, model.OutputFormat, *model, resp) }, } configureFlags(cmd) @@ -212,8 +212,11 @@ func buildRequest(ctx context.Context, model *inputModel, apiClient *resourceman return req, nil } -func outputResult(p *print.Printer, model *inputModel, resp *resourcemanager.Project) error { - switch model.OutputFormat { +func outputResult(p *print.Printer, outputFormat string, model inputModel, resp *resourcemanager.Project) error { + if resp == nil { + return fmt.Errorf("response is empty") + } + switch outputFormat { case print.JSONOutputFormat: details, err := json.MarshalIndent(resp, "", " ") if err != nil { diff --git a/internal/cmd/project/create/create_test.go b/internal/cmd/project/create/create_test.go index 43fa09b92..4269cd6a6 100644 --- a/internal/cmd/project/create/create_test.go +++ b/internal/cmd/project/create/create_test.go @@ -4,14 +4,13 @@ import ( "context" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/uuid" "github.com/stackitcloud/stackit-cli/internal/pkg/auth" "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" "github.com/stackitcloud/stackit-cli/internal/pkg/print" "github.com/stackitcloud/stackit-cli/internal/pkg/utils" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/google/uuid" "github.com/stackitcloud/stackit-sdk-go/services/resourcemanager" "github.com/zalando/go-keyring" ) @@ -359,3 +358,29 @@ func TestBuildRequest(t *testing.T) { }) } } + +func Test_outputResult(t *testing.T) { + type args struct { + outputFormat string + model inputModel + resp *resourcemanager.Project + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"empty", args{}, true}, + {"base", args{"", inputModel{}, &resourcemanager.Project{}}, false}, + } + + p := print.NewPrinter() + p.Cmd = NewCmd(p) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := outputResult(p, tt.args.outputFormat, tt.args.model, tt.args.resp); (err != nil) != tt.wantErr { + t.Errorf("outputResult() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/cmd/project/describe/describe.go b/internal/cmd/project/describe/describe.go index 62b997b64..26162f859 100644 --- a/internal/cmd/project/describe/describe.go +++ b/internal/cmd/project/describe/describe.go @@ -119,6 +119,9 @@ func buildRequest(ctx context.Context, model *inputModel, apiClient *resourceman } func outputResult(p *print.Printer, outputFormat string, project *resourcemanager.GetProjectResponse) error { + if project == nil { + return fmt.Errorf("response not set") + } switch outputFormat { case print.JSONOutputFormat: details, err := json.MarshalIndent(project, "", " ") @@ -146,7 +149,9 @@ func outputResult(p *print.Printer, outputFormat string, project *resourcemanage table.AddSeparator() table.AddRow("STATE", utils.PtrString(project.LifecycleState)) table.AddSeparator() - table.AddRow("PARENT ID", utils.PtrString(project.Parent.Id)) + if project.Parent != nil { + table.AddRow("PARENT ID", utils.PtrString(project.Parent.Id)) + } err := table.Display(p) if err != nil { return fmt.Errorf("render table: %w", err) diff --git a/internal/cmd/project/describe/describe_test.go b/internal/cmd/project/describe/describe_test.go index 8d8cd7490..3c2e5f658 100644 --- a/internal/cmd/project/describe/describe_test.go +++ b/internal/cmd/project/describe/describe_test.go @@ -3,13 +3,14 @@ package describe import ( "context" "testing" - - "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" - "github.com/stackitcloud/stackit-cli/internal/pkg/print" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" + "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" + "github.com/stackitcloud/stackit-cli/internal/pkg/print" + "github.com/stackitcloud/stackit-cli/internal/pkg/utils" "github.com/stackitcloud/stackit-sdk-go/services/resourcemanager" ) @@ -214,3 +215,35 @@ func TestBuildRequest(t *testing.T) { }) } } + +func Test_outputResult(t *testing.T) { + type args struct { + outputFormat string + project *resourcemanager.GetProjectResponse + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"empty", args{}, true}, + {"base", args{"", &resourcemanager.GetProjectResponse{}}, false}, + {"complete", args{"", &resourcemanager.GetProjectResponse{ + ProjectId: utils.Ptr("4711"), + Name: utils.Ptr("name"), + CreationTime: utils.Ptr(time.Now()), + LifecycleState: utils.Ptr(resourcemanager.LIFECYCLESTATE_CREATING), + Parent: &resourcemanager.Parent{Id: utils.Ptr("parent id")}, + }, + }, false}, + } + p := print.NewPrinter() + p.Cmd = NewCmd(p) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := outputResult(p, tt.args.outputFormat, tt.args.project); (err != nil) != tt.wantErr { + t.Errorf("outputResult() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/cmd/project/list/list.go b/internal/cmd/project/list/list.go index 99afb9b1f..9051dc145 100644 --- a/internal/cmd/project/list/list.go +++ b/internal/cmd/project/list/list.go @@ -242,11 +242,16 @@ func outputResult(p *print.Printer, outputFormat string, projects []resourcemana table.SetHeader("ID", "NAME", "STATE", "PARENT ID") for i := range projects { p := projects[i] + + var parentId *string + if p.Parent != nil { + parentId = p.Parent.Id + } table.AddRow( utils.PtrString(p.ProjectId), utils.PtrString(p.Name), utils.PtrString(p.LifecycleState), - utils.PtrString(p.Parent.Id), + utils.PtrString(parentId), ) } diff --git a/internal/cmd/project/list/list_test.go b/internal/cmd/project/list/list_test.go index 2f5bc9e98..7c6e73cee 100644 --- a/internal/cmd/project/list/list_test.go +++ b/internal/cmd/project/list/list_test.go @@ -9,17 +9,16 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/uuid" "github.com/stackitcloud/stackit-cli/internal/pkg/auth" "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" "github.com/stackitcloud/stackit-cli/internal/pkg/print" "github.com/stackitcloud/stackit-cli/internal/pkg/utils" - "github.com/zalando/go-keyring" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/google/uuid" sdkConfig "github.com/stackitcloud/stackit-sdk-go/core/config" "github.com/stackitcloud/stackit-sdk-go/services/resourcemanager" + "github.com/zalando/go-keyring" ) type testCtxKey struct{} @@ -495,3 +494,53 @@ func TestFetchProjects(t *testing.T) { }) } } + +func Test_outputResult(t *testing.T) { + type args struct { + outputFormat string + projects []resourcemanager.Project + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"empty", args{}, false}, + {"base", args{"", []resourcemanager.Project{{}}}, false}, + {"complete", args{"", []resourcemanager.Project{ + { + ContainerId: utils.Ptr("container-id1"), + CreationTime: utils.Ptr(time.Now()), + Labels: &map[string]string{"foo": "bar"}, + LifecycleState: utils.Ptr(resourcemanager.LIFECYCLESTATE_CREATING), + Name: utils.Ptr("some name"), + Parent: &resourcemanager.Parent{ + Id: utils.Ptr("parent-id"), + }, + ProjectId: utils.Ptr("project-id1"), + }, + { + ContainerId: utils.Ptr("container-id2"), + CreationTime: utils.Ptr(time.Now()), + Labels: &map[string]string{"foo": "bar"}, + LifecycleState: utils.Ptr(resourcemanager.LIFECYCLESTATE_CREATING), + Name: utils.Ptr("some name"), + Parent: &resourcemanager.Parent{ + Id: utils.Ptr("parent-id"), + }, + ProjectId: utils.Ptr("project-id2"), + }, + }}, false}, + } + + p := print.NewPrinter() + p.Cmd = NewCmd(p) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := outputResult(p, tt.args.outputFormat, tt.args.projects); (err != nil) != tt.wantErr { + t.Errorf("outputResult() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/cmd/project/member/list/list.go b/internal/cmd/project/member/list/list.go index 1d5056303..e874da2bf 100644 --- a/internal/cmd/project/member/list/list.go +++ b/internal/cmd/project/member/list/list.go @@ -89,7 +89,7 @@ func NewCmd(p *print.Printer) *cobra.Command { members = members[:*model.Limit] } - return outputResult(p, model, members) + return outputResult(p, model.OutputFormat, *model, members) }, } configureFlags(cmd) @@ -145,20 +145,20 @@ func buildRequest(ctx context.Context, model *inputModel, apiClient *authorizati return req } -func outputResult(p *print.Printer, model *inputModel, members []authorization.Member) error { +func outputResult(p *print.Printer, outputFormat string, model inputModel, members []authorization.Member) error { sortFn := func(i, j int) bool { switch model.SortBy { case "subject": - return *members[i].Subject < *members[j].Subject + return utils.PtrString(members[i].Subject) < utils.PtrString(members[j].Subject) case "role": - return *members[i].Role < *members[j].Role + return utils.PtrString(members[i].Role) < utils.PtrString(members[j].Role) default: return false } } sort.SliceStable(members, sortFn) - switch model.OutputFormat { + switch outputFormat { case print.JSONOutputFormat: // Show details details, err := json.MarshalIndent(members, "", " ") diff --git a/internal/cmd/project/member/list/list_test.go b/internal/cmd/project/member/list/list_test.go index 1e4050d4e..296cddd82 100644 --- a/internal/cmd/project/member/list/list_test.go +++ b/internal/cmd/project/member/list/list_test.go @@ -4,14 +4,13 @@ import ( "context" "testing" - "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" - "github.com/stackitcloud/stackit-cli/internal/pkg/print" - "github.com/stackitcloud/stackit-cli/internal/pkg/utils" - "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" "github.com/spf13/cobra" + "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" + "github.com/stackitcloud/stackit-cli/internal/pkg/print" + "github.com/stackitcloud/stackit-cli/internal/pkg/utils" "github.com/stackitcloud/stackit-sdk-go/services/authorization" ) @@ -209,3 +208,43 @@ func TestBuildRequest(t *testing.T) { }) } } + +func Test_outputResult(t *testing.T) { + type args struct { + outputFormat string + model inputModel + members []authorization.Member + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"empty", args{}, false}, + {"base", args{"", inputModel{ + Subject: utils.Ptr("subject"), + Limit: nil, + SortBy: "", + }, nil}, false}, + {"complete", args{"", inputModel{ + Subject: utils.Ptr("subject"), + Limit: nil, + SortBy: "", + }, + []authorization.Member{ + {Role: utils.Ptr("role1"), Subject: utils.Ptr("subject1")}, + {Role: utils.Ptr("role2"), Subject: utils.Ptr("subject2")}, + {Role: utils.Ptr("role3"), Subject: utils.Ptr("subject3")}, + }}, + false}, + } + p := print.NewPrinter() + p.Cmd = NewCmd(p) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := outputResult(p, tt.args.outputFormat, tt.args.model, tt.args.members); (err != nil) != tt.wantErr { + t.Errorf("outputResult() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/cmd/project/role/list/list_test.go b/internal/cmd/project/role/list/list_test.go index 59c8f9855..4e8744528 100644 --- a/internal/cmd/project/role/list/list_test.go +++ b/internal/cmd/project/role/list/list_test.go @@ -4,14 +4,13 @@ import ( "context" "testing" - "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" - "github.com/stackitcloud/stackit-cli/internal/pkg/print" - "github.com/stackitcloud/stackit-cli/internal/pkg/utils" - "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" "github.com/spf13/cobra" + "github.com/stackitcloud/stackit-cli/internal/pkg/globalflags" + "github.com/stackitcloud/stackit-cli/internal/pkg/print" + "github.com/stackitcloud/stackit-cli/internal/pkg/utils" "github.com/stackitcloud/stackit-sdk-go/services/authorization" ) @@ -172,3 +171,37 @@ func TestBuildRequest(t *testing.T) { }) } } + +func Test_outputRolesResult(t *testing.T) { + type args struct { + outputFormat string + roles []authorization.Role + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"empty", args{}, false}, + {"standard", args{"", nil}, false}, + {"complete", args{"", []authorization.Role{ + { + Description: utils.Ptr("description"), + Id: utils.Ptr("id"), + Name: utils.Ptr("name"), + Permissions: &[]authorization.Permission{ + {Description: utils.Ptr("description"), Name: utils.Ptr("name")}, + }, + }, + }}, false}, + } + p := print.NewPrinter() + p.Cmd = NewCmd(p) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := outputRolesResult(p, tt.args.outputFormat, tt.args.roles); (err != nil) != tt.wantErr { + t.Errorf("outputRolesResult() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} From af5b8fb47c28b6b49a2335491f5244535ae7a5e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BCdiger=20Schmitz?= <152157960+bahkauv70@users.noreply.github.com> Date: Thu, 6 Mar 2025 08:56:26 +0100 Subject: [PATCH 2/2] fix: add review findings --- internal/cmd/project/create/create.go | 9 ++++--- internal/cmd/project/create/create_test.go | 11 ++++---- internal/cmd/project/member/list/list.go | 9 ++++--- internal/cmd/project/member/list/list_test.go | 27 ++++++++++--------- 4 files changed, 31 insertions(+), 25 deletions(-) diff --git a/internal/cmd/project/create/create.go b/internal/cmd/project/create/create.go index 50173875d..99f62fadd 100644 --- a/internal/cmd/project/create/create.go +++ b/internal/cmd/project/create/create.go @@ -95,7 +95,7 @@ func NewCmd(p *print.Printer) *cobra.Command { return fmt.Errorf("create project: %w", err) } - return outputResult(p, model.OutputFormat, *model, resp) + return outputResult(p, *model, resp) }, } configureFlags(cmd) @@ -212,11 +212,14 @@ func buildRequest(ctx context.Context, model *inputModel, apiClient *resourceman return req, nil } -func outputResult(p *print.Printer, outputFormat string, model inputModel, resp *resourcemanager.Project) error { +func outputResult(p *print.Printer, model inputModel, resp *resourcemanager.Project) error { if resp == nil { return fmt.Errorf("response is empty") } - switch outputFormat { + if model.GlobalFlagModel == nil { + return fmt.Errorf("globalflags are empty") + } + switch model.OutputFormat { case print.JSONOutputFormat: details, err := json.MarshalIndent(resp, "", " ") if err != nil { diff --git a/internal/cmd/project/create/create_test.go b/internal/cmd/project/create/create_test.go index 4269cd6a6..8c977d3c6 100644 --- a/internal/cmd/project/create/create_test.go +++ b/internal/cmd/project/create/create_test.go @@ -361,24 +361,23 @@ func TestBuildRequest(t *testing.T) { func Test_outputResult(t *testing.T) { type args struct { - outputFormat string - model inputModel - resp *resourcemanager.Project + model inputModel + resp *resourcemanager.Project } tests := []struct { name string args args wantErr bool }{ - {"empty", args{}, true}, - {"base", args{"", inputModel{}, &resourcemanager.Project{}}, false}, + {"empty", args{model: inputModel{GlobalFlagModel: &globalflags.GlobalFlagModel{}}}, true}, + {"base", args{inputModel{GlobalFlagModel: &globalflags.GlobalFlagModel{}}, &resourcemanager.Project{}}, false}, } p := print.NewPrinter() p.Cmd = NewCmd(p) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := outputResult(p, tt.args.outputFormat, tt.args.model, tt.args.resp); (err != nil) != tt.wantErr { + if err := outputResult(p, tt.args.model, tt.args.resp); (err != nil) != tt.wantErr { t.Errorf("outputResult() error = %v, wantErr %v", err, tt.wantErr) } }) diff --git a/internal/cmd/project/member/list/list.go b/internal/cmd/project/member/list/list.go index e874da2bf..861423dc5 100644 --- a/internal/cmd/project/member/list/list.go +++ b/internal/cmd/project/member/list/list.go @@ -89,7 +89,7 @@ func NewCmd(p *print.Printer) *cobra.Command { members = members[:*model.Limit] } - return outputResult(p, model.OutputFormat, *model, members) + return outputResult(p, *model, members) }, } configureFlags(cmd) @@ -145,7 +145,10 @@ func buildRequest(ctx context.Context, model *inputModel, apiClient *authorizati return req } -func outputResult(p *print.Printer, outputFormat string, model inputModel, members []authorization.Member) error { +func outputResult(p *print.Printer, model inputModel, members []authorization.Member) error { + if model.GlobalFlagModel == nil { + return fmt.Errorf("globalflags are empty") + } sortFn := func(i, j int) bool { switch model.SortBy { case "subject": @@ -158,7 +161,7 @@ func outputResult(p *print.Printer, outputFormat string, model inputModel, membe } sort.SliceStable(members, sortFn) - switch outputFormat { + switch model.OutputFormat { case print.JSONOutputFormat: // Show details details, err := json.MarshalIndent(members, "", " ") diff --git a/internal/cmd/project/member/list/list_test.go b/internal/cmd/project/member/list/list_test.go index 296cddd82..4aecdee12 100644 --- a/internal/cmd/project/member/list/list_test.go +++ b/internal/cmd/project/member/list/list_test.go @@ -211,25 +211,26 @@ func TestBuildRequest(t *testing.T) { func Test_outputResult(t *testing.T) { type args struct { - outputFormat string - model inputModel - members []authorization.Member + model inputModel + members []authorization.Member } tests := []struct { name string args args wantErr bool }{ - {"empty", args{}, false}, - {"base", args{"", inputModel{ - Subject: utils.Ptr("subject"), - Limit: nil, - SortBy: "", + {"empty", args{model: inputModel{GlobalFlagModel: &globalflags.GlobalFlagModel{}}}, false}, + {"base", args{inputModel{ + GlobalFlagModel: &globalflags.GlobalFlagModel{}, + Subject: utils.Ptr("subject"), + Limit: nil, + SortBy: "", }, nil}, false}, - {"complete", args{"", inputModel{ - Subject: utils.Ptr("subject"), - Limit: nil, - SortBy: "", + {"complete", args{inputModel{ + GlobalFlagModel: &globalflags.GlobalFlagModel{}, + Subject: utils.Ptr("subject"), + Limit: nil, + SortBy: "", }, []authorization.Member{ {Role: utils.Ptr("role1"), Subject: utils.Ptr("subject1")}, @@ -242,7 +243,7 @@ func Test_outputResult(t *testing.T) { p.Cmd = NewCmd(p) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := outputResult(p, tt.args.outputFormat, tt.args.model, tt.args.members); (err != nil) != tt.wantErr { + if err := outputResult(p, tt.args.model, tt.args.members); (err != nil) != tt.wantErr { t.Errorf("outputResult() error = %v, wantErr %v", err, tt.wantErr) } })