Skip to content

Commit c9e51a3

Browse files
christiangdaclaude
andcommitted
refactor: modernize idpscimcli CLI with Go 1.26 best practices
- Fix logger initialization order: configure log handler options before creating the handler, so log level/format from config takes effect. - Replace os.Exit(1) in getGWSDirectoryService with error returns, making the function testable and letting cobra handle the exit. - Fix show() to return errors instead of silently discarding marshal failures. - Extract duplicated HTTP client builders into newSCIMHTTPClient() and newGWSHTTPClient() helpers, reducing 3 identical builder blocks to 1. - Extract newAWSSCIMService() helper to deduplicate SCIM service creation across 3 AWS command handlers. - Fix typos: "usrs" → "users", "Servive" → "Service" in help text. - Use strings.TrimSuffix for config filename parsing instead of fragile index slicing. - Add unit tests for show() covering JSON, YAML, default format, marshal errors, and empty structs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 0b3ea3b commit c9e51a3

5 files changed

Lines changed: 164 additions & 132 deletions

File tree

cmd/idpscimcli/cmd/aws.go

Lines changed: 22 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@ package cmd
33
import (
44
"context"
55
"log/slog"
6-
"time"
76

8-
"github.com/slashdevops/httpx"
97
"github.com/slashdevops/idp-scim-sync/internal/version"
108
"github.com/slashdevops/idp-scim-sync/pkg/aws"
119
"github.com/spf13/cobra"
@@ -66,7 +64,7 @@ var (
6664
Use: "list",
6765
Aliases: []string{"l"},
6866
Short: "return available users",
69-
Long: `list available usrs in AWS SSO SCIM`,
67+
Long: `list available users in AWS SSO SCIM`,
7068
RunE: runAWSUsersList,
7169
}
7270
)
@@ -87,95 +85,65 @@ func init() {
8785
awsUsersCmd.Flags().StringVarP(&filter, "filter", "q", "", "AWS SSO SCIM API Filter, example: --filter 'displayName eq \"User Bar\" and id eq \"12324\"'")
8886
}
8987

88+
// newAWSSCIMService creates an AWS SCIM service with the configured HTTP client.
89+
func newAWSSCIMService() (*aws.SCIMService, error) {
90+
svc, err := aws.NewSCIMService(newSCIMHTTPClient(), cfg.AWSSCIMEndpoint, cfg.AWSSCIMAccessToken)
91+
if err != nil {
92+
return nil, err
93+
}
94+
svc.UserAgent = "idp-scim-sync/" + version.Version
95+
return svc, nil
96+
}
97+
9098
func runAWSServiceConfig(_ *cobra.Command, _ []string) error {
9199
ctx, cancel := context.WithTimeout(context.Background(), reqTimeout)
92100
defer cancel()
93101

94-
httpRetryClient := httpx.NewClientBuilder().
95-
WithMaxRetries(10).
96-
WithRetryStrategy(httpx.JitterBackoffStrategy).
97-
WithRetryBaseDelay(500 * time.Millisecond).
98-
WithRetryMaxDelay(10 * time.Second).
99-
Build()
100-
101-
awsSCIMService, err := aws.NewSCIMService(httpRetryClient, cfg.AWSSCIMEndpoint, cfg.AWSSCIMAccessToken)
102+
svc, err := newAWSSCIMService()
102103
if err != nil {
103-
slog.Error("error creating SCIM service", "error", err.Error())
104104
return err
105105
}
106-
awsSCIMService.UserAgent = "idp-scim-sync/" + version.Version
107106

108-
awsServiceConfig, err := awsSCIMService.ServiceProviderConfig(ctx)
107+
result, err := svc.ServiceProviderConfig(ctx)
109108
if err != nil {
110-
slog.Error("error getting service provider config", "error", err.Error())
111109
return err
112110
}
113111

114-
show(outFormat, awsServiceConfig)
115-
116-
return nil
112+
return show(outFormat, result)
117113
}
118114

119115
func runAWSGroupsList(_ *cobra.Command, _ []string) error {
120116
ctx, cancel := context.WithTimeout(context.Background(), reqTimeout)
121117
defer cancel()
122118

123-
httpClient := httpx.NewClientBuilder().
124-
WithMaxRetries(10).
125-
WithRetryStrategy(httpx.JitterBackoffStrategy).
126-
WithRetryBaseDelay(500 * time.Millisecond).
127-
WithRetryMaxDelay(10 * time.Second).
128-
WithMaxIdleConns(100).
129-
WithMaxIdleConnsPerHost(100).
130-
Build()
131-
132-
awsSCIMService, err := aws.NewSCIMService(httpClient, cfg.AWSSCIMEndpoint, cfg.AWSSCIMAccessToken)
119+
svc, err := newAWSSCIMService()
133120
if err != nil {
134-
slog.Error("error creating SCIM service", "error", err.Error())
135121
return err
136122
}
137-
awsSCIMService.UserAgent = "idp-scim-sync/" + version.Version
138123

139-
awsGroupsResponse, err := awsSCIMService.ListGroups(ctx, filter)
124+
result, err := svc.ListGroups(ctx, filter)
140125
if err != nil {
141-
slog.Error("error listing groups", "error", err.Error())
142126
return err
143127
}
144-
slog.Info("groups found", "groups", awsGroupsResponse.TotalResults)
145-
146-
show(outFormat, awsGroupsResponse)
128+
slog.Info("groups found", "groups", result.TotalResults)
147129

148-
return nil
130+
return show(outFormat, result)
149131
}
150132

151133
func runAWSUsersList(_ *cobra.Command, _ []string) error {
152134
ctx, cancel := context.WithTimeout(context.Background(), reqTimeout)
153135
defer cancel()
154136

155-
httpClient := httpx.NewClientBuilder().
156-
WithMaxRetries(10).
157-
WithRetryStrategy(httpx.JitterBackoffStrategy).
158-
WithRetryBaseDelay(500 * time.Millisecond).
159-
WithRetryMaxDelay(10 * time.Second).
160-
WithMaxIdleConns(100).
161-
WithMaxIdleConnsPerHost(100).
162-
Build()
163-
164-
awsSCIMService, err := aws.NewSCIMService(httpClient, cfg.AWSSCIMEndpoint, cfg.AWSSCIMAccessToken)
137+
svc, err := newAWSSCIMService()
165138
if err != nil {
166-
slog.Error("error creating SCIM service", "error", err.Error())
167139
return err
168140
}
169-
awsSCIMService.UserAgent = "idp-scim-sync/" + version.Version
170141

171-
awsUsersResponse, err := awsSCIMService.ListUsers(ctx, filter)
142+
result, err := svc.ListUsers(ctx, filter)
172143
if err != nil {
173-
slog.Error("error listing users", "error", err.Error())
174144
return err
175145
}
176-
slog.Info("users found", "users", awsUsersResponse.TotalResults)
177-
178-
show(outFormat, awsUsersResponse)
146+
slog.Info("users found", "users", result.TotalResults)
179147

180-
return nil
148+
return show(outFormat, result)
181149
}

cmd/idpscimcli/cmd/common.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,21 @@ import (
77
"gopkg.in/yaml.v3"
88
)
99

10-
// show resource structure as outFormat
11-
func show(outFormat string, resource any) {
12-
switch outFormat {
13-
case "json":
14-
j, _ := json.MarshalIndent(resource, "", " ")
15-
fmt.Print(string(j))
10+
// show prints the resource in the specified output format to stdout.
11+
func show(format string, resource any) error {
12+
switch format {
1613
case "yaml":
17-
y, _ := yaml.Marshal(resource)
18-
fmt.Print(string(y))
14+
b, err := yaml.Marshal(resource)
15+
if err != nil {
16+
return fmt.Errorf("error marshaling to YAML: %w", err)
17+
}
18+
fmt.Print(string(b))
1919
default:
20-
j, _ := json.MarshalIndent(resource, "", " ")
21-
fmt.Print(string(j))
20+
b, err := json.MarshalIndent(resource, "", " ")
21+
if err != nil {
22+
return fmt.Errorf("error marshaling to JSON: %w", err)
23+
}
24+
fmt.Println(string(b))
2225
}
26+
return nil
2327
}

cmd/idpscimcli/cmd/common_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
//go:build unit
2+
3+
package cmd
4+
5+
import (
6+
"testing"
7+
)
8+
9+
func TestShow_JSON(t *testing.T) {
10+
data := map[string]string{"key": "value"}
11+
12+
if err := show("json", data); err != nil {
13+
t.Fatalf("show(json) returned error: %v", err)
14+
}
15+
}
16+
17+
func TestShow_YAML(t *testing.T) {
18+
data := map[string]string{"key": "value"}
19+
20+
if err := show("yaml", data); err != nil {
21+
t.Fatalf("show(yaml) returned error: %v", err)
22+
}
23+
}
24+
25+
func TestShow_DefaultIsJSON(t *testing.T) {
26+
data := map[string]string{"key": "value"}
27+
28+
if err := show("unknown", data); err != nil {
29+
t.Fatalf("show(unknown) returned error: %v", err)
30+
}
31+
}
32+
33+
func TestShow_JSONMarshalError(t *testing.T) {
34+
// channels cannot be marshaled to JSON
35+
data := make(chan int)
36+
37+
if err := show("json", data); err == nil {
38+
t.Fatal("show(json) with unmarshalable data should return error")
39+
}
40+
}
41+
42+
func TestShow_EmptyStruct(t *testing.T) {
43+
type empty struct{}
44+
45+
if err := show("json", empty{}); err != nil {
46+
t.Fatalf("show(json) with empty struct returned error: %v", err)
47+
}
48+
49+
if err := show("yaml", empty{}); err != nil {
50+
t.Fatalf("show(yaml) with empty struct returned error: %v", err)
51+
}
52+
}

0 commit comments

Comments
 (0)