Conversation
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
Eliminate the hand-rolled Request/Response abstraction layer in favor of a standard net/http + chi router. All entry points (server, lambda, verify, sample) now construct stdlib *http.Request objects and route them through a shared chi handler returned by App.Handler(). This removes the custom HandleRequest dispatcher, consolidates routing and middleware (admin auth, panic recovery), and makes tests more realistic by exercising the actual router via httptest.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internal/domain/, move sync logic intointernal/sync/, and consolidate the App factory intointernal/app/factory.go*http.Requestobjects and route through a sharedhttp.Handlerreturned byApp.Handler(), with proper middleware for admin auth and panic recoveryerrors.Is()instead oferrors.HasType()),GetOrCreateTeamerror classification, sync rule failure detection, PR review deduplication, Okta pagination, and token refresh race conditioninternal/app24.5% → 55.8%,internal/config14.8% → 74.1%,internal/notifiers2.7% → 97.3%; add integration test hardening (dynamic ports, env restore, unexpected-call validation)Commits
4fa3862— restructure packages, fix bugs, and improve test coverage2dc32b5— improve dx (Dockerfile, compose.yaml, Makefile, dependabot)7a620fa— replace custom request/response types with chi router