Skip to content

Commit 9f4f4af

Browse files
fix(review): address code review findings across branch
- Inline Drive query escape wrappers to use shared EscapeDriveQueryValue directly - Add comprehensive unit tests for EscapeDriveQueryValue - Fix CloudIdentity groups get to use UI layer instead of direct os.Stdout - Remove duplicate splitCSVFields, use splitCSV from split_helpers.go - Make adminCustomerID configurable via GOG_CUSTOMER_ID env var - Add t.Parallel() safety comment to testutil_test.go - Move cloudchannel stub from testutil_test.go to channel_test.go - Fix batch.go channel close pattern with goroutine - Document readValueOrFile file detection heuristic Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 4f5c0fa commit 9f4f4af

32 files changed

Lines changed: 141 additions & 109 deletions
Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
11
package cmd
22

3-
const adminCustomerID = "my_customer"
3+
import "os"
4+
5+
// adminCustomerID returns the customer ID for Admin SDK calls.
6+
// Defaults to "my_customer" (the authenticated admin's domain) but
7+
// can be overridden via the GOG_CUSTOMER_ID environment variable
8+
// for multi-tenant administration.
9+
func adminCustomerID() string {
10+
if id := os.Getenv("GOG_CUSTOMER_ID"); id != "" {
11+
return id
12+
}
13+
return "my_customer"
14+
}

internal/cmd/admins.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func (c *AdminsListCmd) Run(ctx context.Context, flags *RootFlags) error {
3636
return err
3737
}
3838

39-
call := svc.RoleAssignments.List(adminCustomerID).MaxResults(c.Max)
39+
call := svc.RoleAssignments.List(adminCustomerID()).MaxResults(c.Max)
4040
if c.Page != "" {
4141
call = call.PageToken(c.Page)
4242
}
@@ -127,7 +127,7 @@ func (c *AdminsCreateCmd) Run(ctx context.Context, flags *RootFlags) error {
127127
assignment.OrgUnitId = orgID
128128
}
129129

130-
created, err := svc.RoleAssignments.Insert(adminCustomerID, assignment).Context(ctx).Do()
130+
created, err := svc.RoleAssignments.Insert(adminCustomerID(), assignment).Context(ctx).Do()
131131
if err != nil {
132132
return fmt.Errorf("assign role %s to %s: %w", c.Role, c.User, err)
133133
}
@@ -160,7 +160,7 @@ func (c *AdminsDeleteCmd) Run(ctx context.Context, flags *RootFlags) error {
160160
return err
161161
}
162162

163-
if err := svc.RoleAssignments.Delete(adminCustomerID, c.AssignmentID).Context(ctx).Do(); err != nil {
163+
if err := svc.RoleAssignments.Delete(adminCustomerID(), c.AssignmentID).Context(ctx).Do(); err != nil {
164164
return fmt.Errorf("delete admin assignment %s: %w", c.AssignmentID, err)
165165
}
166166

@@ -199,7 +199,7 @@ func resolveUserID(ctx context.Context, svc *admin.Service, user string) (string
199199
}
200200

201201
func resolveOrgUnitID(ctx context.Context, svc *admin.Service, path string) (string, error) {
202-
ou, err := svc.Orgunits.Get(adminCustomerID, path).Context(ctx).Do()
202+
ou, err := svc.Orgunits.Get(adminCustomerID(), path).Context(ctx).Do()
203203
if err != nil {
204204
return "", fmt.Errorf("resolve org unit %s: %w", path, err)
205205
}

internal/cmd/batch.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,7 @@ func (c *BatchCmd) Run(ctx context.Context, flags *RootFlags) error {
7373
tasks <- task
7474
}
7575
close(tasks)
76-
wg.Wait()
77-
close(results)
76+
go func() { wg.Wait(); close(results) }()
7877

7978
failed := 0
8079
for err := range results {

internal/cmd/channel_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,32 @@ import (
44
"context"
55
"encoding/json"
66
"net/http"
7+
"net/http/httptest"
78
"strings"
89
"testing"
910

1011
"google.golang.org/api/cloudchannel/v1"
12+
"google.golang.org/api/option"
1113

1214
"github.com/steipete/gogcli/internal/outfmt"
1315
)
1416

17+
func newCloudChannelServiceStub(t *testing.T, handler http.HandlerFunc) (*cloudchannel.Service, func()) {
18+
t.Helper()
19+
20+
srv := httptest.NewServer(handler)
21+
svc, err := cloudchannel.NewService(context.Background(),
22+
option.WithoutAuthentication(),
23+
option.WithHTTPClient(srv.Client()),
24+
option.WithEndpoint(srv.URL+"/"),
25+
)
26+
if err != nil {
27+
srv.Close()
28+
t.Fatalf("NewService: %v", err)
29+
}
30+
return svc, srv.Close
31+
}
32+
1533
func stubCloudChannelService(t *testing.T, svc *cloudchannel.Service) {
1634
t.Helper()
1735
orig := newCloudChannelService

internal/cmd/cloudidentity.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,23 +133,24 @@ func (c *CloudIdentityGroupsGetCmd) Run(ctx context.Context, flags *RootFlags) e
133133
return outfmt.WriteJSON(os.Stdout, group)
134134
}
135135

136-
fmt.Fprintf(os.Stdout, "Name: %s\n", group.Name)
136+
u := ui.FromContext(ctx)
137+
u.Out().Printf("Name: %s\n", group.Name)
137138
if group.GroupKey != nil {
138-
fmt.Fprintf(os.Stdout, "Email: %s\n", group.GroupKey.Id)
139+
u.Out().Printf("Email: %s\n", group.GroupKey.Id)
139140
}
140141
if group.DisplayName != "" {
141-
fmt.Fprintf(os.Stdout, "Display Name: %s\n", group.DisplayName)
142+
u.Out().Printf("Display Name: %s\n", group.DisplayName)
142143
}
143144
if group.Parent != "" {
144-
fmt.Fprintf(os.Stdout, "Parent: %s\n", group.Parent)
145+
u.Out().Printf("Parent: %s\n", group.Parent)
145146
}
146147
if len(group.Labels) > 0 {
147148
labels := make([]string, 0, len(group.Labels))
148149
for key := range group.Labels {
149150
labels = append(labels, key)
150151
}
151152
sort.Strings(labels)
152-
fmt.Fprintf(os.Stdout, "Labels: %s\n", strings.Join(labels, ", "))
153+
u.Out().Printf("Labels: %s\n", strings.Join(labels, ", "))
153154
}
154155
return nil
155156
}

internal/cmd/csv.go

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func (c *CSVCmd) Run(ctx context.Context, flags *RootFlags) error {
3838
return err
3939
}
4040

41-
fields := splitCSVFields(c.Fields)
41+
fields := splitCSV(c.Fields)
4242
processed := 0
4343
failed := 0
4444

@@ -80,18 +80,3 @@ func (c *CSVCmd) Run(ctx context.Context, flags *RootFlags) error {
8080
}
8181
return nil
8282
}
83-
84-
func splitCSVFields(input string) []string {
85-
trimmed := strings.TrimSpace(input)
86-
if trimmed == "" {
87-
return nil
88-
}
89-
parts := strings.Split(trimmed, ",")
90-
out := make([]string, 0, len(parts))
91-
for _, part := range parts {
92-
if value := strings.TrimSpace(part); value != "" {
93-
out = append(out, value)
94-
}
95-
}
96-
return out
97-
}

internal/cmd/domains_aliases.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func (c *DomainsAliasesListCmd) Run(ctx context.Context, flags *RootFlags) error
2525
return err
2626
}
2727

28-
resp, err := svc.DomainAliases.List(adminCustomerID).Context(ctx).Do()
28+
resp, err := svc.DomainAliases.List(adminCustomerID()).Context(ctx).Do()
2929
if err != nil {
3030
return fmt.Errorf("list domain aliases: %w", err)
3131
}
@@ -77,7 +77,7 @@ func (c *DomainsAliasesCreateCmd) Run(ctx context.Context, flags *RootFlags) err
7777
DomainAliasName: c.Alias,
7878
ParentDomainName: c.Parent,
7979
}
80-
created, err := svc.DomainAliases.Insert(adminCustomerID, req).Context(ctx).Do()
80+
created, err := svc.DomainAliases.Insert(adminCustomerID(), req).Context(ctx).Do()
8181
if err != nil {
8282
return fmt.Errorf("create domain alias %s: %w", c.Alias, err)
8383
}
@@ -110,7 +110,7 @@ func (c *DomainsAliasesDeleteCmd) Run(ctx context.Context, flags *RootFlags) err
110110
return err
111111
}
112112

113-
if err := svc.DomainAliases.Delete(adminCustomerID, c.Alias).Context(ctx).Do(); err != nil {
113+
if err := svc.DomainAliases.Delete(adminCustomerID(), c.Alias).Context(ctx).Do(); err != nil {
114114
return fmt.Errorf("delete domain alias %s: %w", c.Alias, err)
115115
}
116116

internal/cmd/domains_create.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func (c *DomainsCreateCmd) Run(ctx context.Context, flags *RootFlags) error {
2828
}
2929

3030
req := &admin.Domains{DomainName: c.Domain}
31-
created, err := svc.Domains.Insert(adminCustomerID, req).Context(ctx).Do()
31+
created, err := svc.Domains.Insert(adminCustomerID(), req).Context(ctx).Do()
3232
if err != nil {
3333
return fmt.Errorf("create domain %s: %w", c.Domain, err)
3434
}

internal/cmd/domains_delete.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func (c *DomainsDeleteCmd) Run(ctx context.Context, flags *RootFlags) error {
2727
return err
2828
}
2929

30-
if err := svc.Domains.Delete(adminCustomerID, c.Domain).Context(ctx).Do(); err != nil {
30+
if err := svc.Domains.Delete(adminCustomerID(), c.Domain).Context(ctx).Do(); err != nil {
3131
return fmt.Errorf("delete domain %s: %w", c.Domain, err)
3232
}
3333

internal/cmd/domains_get.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func (c *DomainsGetCmd) Run(ctx context.Context, flags *RootFlags) error {
2525
return err
2626
}
2727

28-
domain, err := svc.Domains.Get(adminCustomerID, c.Domain).Context(ctx).Do()
28+
domain, err := svc.Domains.Get(adminCustomerID(), c.Domain).Context(ctx).Do()
2929
if err != nil {
3030
return fmt.Errorf("get domain %s: %w", c.Domain, err)
3131
}

0 commit comments

Comments
 (0)