Skip to content

Commit 3388b6c

Browse files
committed
admin: address CodeRabbit nitpicks (readonly review)
Four nitpick-tier findings collapsed into one commit: - main.go: clarify the dynamoServer field doc — drop "exposed", which read like the field were exported. State explicitly that it is package-private state passed to startAdminFromFlags. - internal/admin/dynamo_handler_test.go: drop the unused listOrder field on stubTablesSource (no test ever set it; carried over from an aborted "ordering bug" simulation). Replace the hand-rolled insertion sort with sort.Strings — stdlib idiom, less surface to maintain. - adapter/dynamodb_admin_test.go: document why us-west-2 is a fine hardcoded region in the SigV4 client (the adapter does not enforce region match). Pointer for the future: if that ever changes, the region should be sourced from the same constant on both sides rather than re-hardcoded everywhere.
1 parent 2971a9a commit 3388b6c

3 files changed

Lines changed: 19 additions & 27 deletions

File tree

adapter/dynamodb_admin_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,12 @@ func TestDynamoDB_AdminDescribeTable_GSI_SortedDeterministic(t *testing.T) {
172172

173173
func newDynamoClient(t *testing.T, address string) *dynamodb.Client {
174174
t.Helper()
175+
// Region is intentionally arbitrary here. The test DynamoDB
176+
// server does not enforce a region match in its SigV4 path —
177+
// every existing adapter test uses "us-west-2" as a placeholder
178+
// for the same reason. If the server later starts requiring a
179+
// specific region, source it from the same constant the server
180+
// reads instead of hardcoding it on each side independently.
175181
cfg, err := config.LoadDefaultConfig(context.Background(),
176182
config.WithRegion("us-west-2"),
177183
config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider("dummy", "dummy", "")),

internal/admin/dynamo_handler_test.go

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"errors"
77
"net/http"
88
"net/http/httptest"
9+
"sort"
910
"strings"
1011
"testing"
1112

@@ -14,30 +15,23 @@ import (
1415
)
1516

1617
// stubTablesSource is the in-memory test double the dynamo handler
17-
// tests use. Returning sorted names from a map iteration mirrors the
18-
// adapter contract (lex-sorted by AdminListTables).
18+
// tests use. AdminListTables returns names in lex order, matching
19+
// the adapter's contract.
1920
type stubTablesSource struct {
20-
tables map[string]*DynamoTableSummary
21-
listErr error
22-
descErr error
23-
listOrder []string // overrides sorted order if non-nil; lets us simulate adapter ordering bugs
21+
tables map[string]*DynamoTableSummary
22+
listErr error
23+
descErr error
2424
}
2525

2626
func (s *stubTablesSource) AdminListTables(_ context.Context) ([]string, error) {
2727
if s.listErr != nil {
2828
return nil, s.listErr
2929
}
30-
if s.listOrder != nil {
31-
out := make([]string, len(s.listOrder))
32-
copy(out, s.listOrder)
33-
return out, nil
34-
}
3530
out := make([]string, 0, len(s.tables))
3631
for k := range s.tables {
3732
out = append(out, k)
3833
}
39-
// AdminListTables's contract is sorted output.
40-
stableSort(out)
34+
sort.Strings(out)
4135
return out, nil
4236
}
4337

@@ -52,16 +46,6 @@ func (s *stubTablesSource) AdminDescribeTable(_ context.Context, name string) (*
5246
return t, true, nil
5347
}
5448

55-
// stableSort is a tiny helper to avoid importing sort in test files
56-
// that already have their own sort dependency style.
57-
func stableSort(s []string) {
58-
for i := 1; i < len(s); i++ {
59-
for j := i; j > 0 && s[j-1] > s[j]; j-- {
60-
s[j-1], s[j] = s[j], s[j-1]
61-
}
62-
}
63-
}
64-
6549
func newDynamoHandlerForTest(src TablesSource) *DynamoHandler {
6650
return NewDynamoHandler(src)
6751
}

main.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -983,10 +983,12 @@ type runtimeServerRunner struct {
983983
pprofToken string
984984
metricsRegistry *monitoring.Registry
985985

986-
// dynamoServer is populated by start() and exposed so the admin
987-
// listener can call its SigV4-bypass admin entrypoints (see
988-
// adapter/dynamodb_admin.go) without going through HTTP. It is
989-
// nil until start() reaches the dynamo step.
986+
// dynamoServer is populated by start() and made available to
987+
// startAdminFromFlags in this package so the admin listener can
988+
// call SigV4-bypass admin entrypoints (see
989+
// adapter/dynamodb_admin.go) without going through HTTP. The
990+
// field is unexported on purpose — it is package-private state,
991+
// not a public API. Nil until start() reaches the dynamo step.
990992
dynamoServer *adapter.DynamoDBServer
991993
}
992994

0 commit comments

Comments
 (0)