Skip to content

Commit 4fa3862

Browse files
committed
refactor: restructure packages, fix bugs, and improve test coverage
Reorganize internal packages into domain/sync layers, fix multiple correctness bugs, harden the integration test infrastructure, and add comprehensive unit tests across all major packages. Architecture: - Extract shared App factory into internal/app/factory.go, removing duplicated newApp() from all four cmd/ entry points - Move domain types/interfaces/errors into internal/domain/ - Move sync logic into internal/sync/ with full unit tests - Add context.Context to OktaClient interface, remove stored-context anti-pattern - Add compile-time interface assertions for GitHubClient and OktaClient - Remove unused GitHubClientFactory and cross-installation code path Bug fixes: - Fix mapErrorResponse: errors.HasType() never matched errors.Mark() markers; switch to errors.Is() so domain errors return correct HTTP status codes (401/400/503/502) instead of always 500 - Fix GetOrCreateTeam: non-404 errors (403, 500) were misclassified as ErrTeamNotFound - Fix sync all-rules-failed detection: compared against len(reports) instead of tracking enabled rule count separately - Fix PR review deduplication: count only latest review per user - Add GitHub Check Runs support alongside legacy commit statuses - Fix token refresh TOCTOU race with double-check pattern - Add Okta API pagination to ListGroups, GetGroupByName, GetGroupMembers - Normalize username case in team membership comparisons Test infrastructure: - Replace hardcoded ports (9001-9003) with dynamic TLS listeners - Add IP SANs to self-signed test certificates - Add env var save/restore between integration test scenarios - Add unexpected-destructive-call validation - Fix test log handler Enabled() to respect verbose flag - Add request body size limit (10MB) to HTTP server Test coverage (before -> after): - internal/app: 24.5% -> 55.8% - internal/config: 14.8% -> 74.1% - internal/notifiers: 2.7% -> 97.3% - internal/sync: 82.8% -> 83.3% Cleanup: - Remove all fmt.Errorf usage, consistently use cockroachdb/errors - Remove dead ErrOAuthTokenExpired sentinel - Extract extractGroupName() helper in okta package - Pre-compile regex in sync.go computeTeamName - Trim leading/trailing dashes from generated team names
1 parent 73c627d commit 4fa3862

34 files changed

+2606
-515
lines changed

.github/.dependabot.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,8 @@ updates:
44
directory: "/"
55
schedule:
66
interval: "daily"
7+
8+
- package-ecosystem: "gomod"
9+
directory: "/"
10+
schedule:
11+
interval: "weekly"

.github/workflows/ci.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,10 @@ jobs:
4646
with:
4747
go-version: '1.24'
4848

49-
- name: Run tests
49+
- name: Run unit tests
5050
run: make test
5151

52-
- name: Run tests
52+
- name: Run integration tests
5353
run: make test-verify-verbose
5454

5555
build:

cmd/lambda/main.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@ package main
33
import (
44
"context"
55
"encoding/json"
6-
"fmt"
76
"log/slog"
87
"strings"
98
"sync"
109

1110
awsevents "github.com/aws/aws-lambda-go/events"
1211
"github.com/aws/aws-lambda-go/lambda"
12+
"github.com/cockroachdb/errors"
1313
"github.com/cruxstack/github-ops-app/internal/app"
1414
"github.com/cruxstack/github-ops-app/internal/config"
1515
)
@@ -27,10 +27,10 @@ func initApp() {
2727

2828
cfg, err := config.NewConfig()
2929
if err != nil {
30-
initErr = fmt.Errorf("config init failed: %w", err)
30+
initErr = errors.Wrap(err, "config init failed")
3131
return
3232
}
33-
appInst, initErr = app.New(context.Background(), cfg)
33+
appInst, initErr = app.NewApp(context.Background(), cfg, logger)
3434
})
3535
}
3636

@@ -99,7 +99,7 @@ func EventBridgeHandler(ctx context.Context, evt awsevents.CloudWatchEvent) erro
9999
resp := appInst.HandleRequest(ctx, req)
100100

101101
if resp.StatusCode >= 400 {
102-
return fmt.Errorf("scheduled event failed: %s", string(resp.Body))
102+
return errors.Newf("scheduled event failed: %s", string(resp.Body))
103103
}
104104

105105
return nil
@@ -122,7 +122,7 @@ func UniversalHandler(ctx context.Context, event json.RawMessage) (any, error) {
122122
return nil, EventBridgeHandler(ctx, eventBridgeEvent)
123123
}
124124

125-
return nil, fmt.Errorf("unknown lambda event type")
125+
return nil, errors.New("unknown lambda event type")
126126
}
127127

128128
func main() {

cmd/sample/main.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func main() {
3232
os.Exit(1)
3333
}
3434

35-
a, err := app.New(ctx, cfg)
35+
a, err := app.NewApp(ctx, cfg, logger)
3636
if err != nil {
3737
logger.Error("failed to initialize app", slog.String("error", err.Error()))
3838
os.Exit(1)
@@ -52,7 +52,11 @@ func main() {
5252
}
5353

5454
for i, sample := range samples {
55-
eventType := sample["event_type"].(string)
55+
eventType, ok := sample["event_type"].(string)
56+
if !ok {
57+
logger.Error("missing or invalid event_type", slog.Int("sample", i))
58+
os.Exit(1)
59+
}
5660

5761
switch eventType {
5862
case "okta_sync":

cmd/server/main.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func main() {
3030
os.Exit(1)
3131
}
3232

33-
appInst, err = app.New(ctx, cfg)
33+
appInst, err = app.NewApp(ctx, cfg, logger)
3434
if err != nil {
3535
logger.Error("app init failed", slog.String("error", err.Error()))
3636
os.Exit(1)
@@ -83,12 +83,12 @@ func main() {
8383

8484
// httpHandler converts http.Request to app.Request and handles the response.
8585
func httpHandler(w http.ResponseWriter, r *http.Request) {
86-
body, err := io.ReadAll(r.Body)
86+
defer r.Body.Close()
87+
body, err := io.ReadAll(io.LimitReader(r.Body, 10<<20)) // 10MB limit
8788
if err != nil {
8889
http.Error(w, "failed to read request body", http.StatusBadRequest)
8990
return
9091
}
91-
defer r.Body.Close()
9292

9393
headers := make(map[string]string)
9494
for key, values := range r.Header {

cmd/verify/.env.test

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
# config used during offline verification testing
22
#
3-
# - all api calls are mocked all config is fake
4-
# - private keys is dynamically generated during test setup
5-
# - endpoints are https with tls configured during setup
3+
# - all api calls are mocked and all config is fake
4+
# - private keys are dynamically generated during test setup
5+
# - endpoints use dynamic ports assigned at runtime
6+
# - base urls are set per-scenario by the test runner
67

78
APP_DEBUG_ENABLED=false
89

@@ -11,15 +12,13 @@ APP_GITHUB_APP_ID=123456
1112
APP_GITHUB_INSTALLATION_ID=987654
1213
APP_GITHUB_ORG=acme-ghorg
1314
APP_GITHUB_WEBHOOK_SECRET=test_webhook_secret
14-
APP_GITHUB_BASE_URL=https://localhost:9001/
1515

1616
# github pr compliance configuration
1717
APP_PR_COMPLIANCE_ENABLED=true
1818
APP_PR_MONITORED_BRANCHES=main,master
1919

2020
# okta configuration (oauth2 with private key)
2121
APP_OKTA_DOMAIN=dev-12345.okta.com
22-
APP_OKTA_CLIENT_ID=test-client-id
2322

2423
# okta sync rules
2524
APP_OKTA_GITHUB_USER_FIELD=githubUsername
@@ -28,4 +27,3 @@ APP_OKTA_SYNC_RULES=[{"enabled":true,"okta_group_name":"Engineering","github_tea
2827
# slack configuration
2928
APP_SLACK_TOKEN=xoxb-test-token
3029
APP_SLACK_CHANNEL=C01234TEST
31-
APP_SLACK_API_URL=https://localhost:9003/

cmd/verify/logger.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ type testHandler struct {
1818

1919
// Enabled returns true for all log levels when verbose mode is enabled.
2020
func (h *testHandler) Enabled(_ context.Context, _ slog.Level) bool {
21-
return true
21+
return h.verbose
2222
}
2323

2424
// Handle formats and writes log records to output with test-appropriate

0 commit comments

Comments
 (0)