Conversation
There was a problem hiding this comment.
Pull request overview
Adds documentation for running sql-tap in a CI-friendly “collect and report” mode so teams can fail builds on detected N+1 or slow queries.
Changes:
- Document the new
-ciflag in thesql-tapusage section. - Add a “CI mode” README section with a suggested shell workflow and sample report output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Start sql-tap in CI mode (background) | ||
| sql-tap -ci localhost:9091 & | ||
| CI_PID=$! | ||
|
|
||
| # Run your tests through the proxy | ||
| DATABASE_URL="postgres://user:pass@localhost:5433/db" go test ./... | ||
|
|
||
| # Stop sql-tap — prints report and exits 0 (clean) or 1 (problems found) | ||
| kill $CI_PID | ||
| wait $CI_PID |
There was a problem hiding this comment.
The CI mode example assumes sql-tapd (the proxy + gRPC server) is already running, but the snippet only starts sql-tap. Consider adding an explicit step (or a short note) showing how to start sql-tapd and clarifying that the test DB connection must point at the sql-tapd --listen address.
| # Start sql-tap in CI mode (background) | |
| sql-tap -ci localhost:9091 & | |
| CI_PID=$! | |
| # Run your tests through the proxy | |
| DATABASE_URL="postgres://user:pass@localhost:5433/db" go test ./... | |
| # Stop sql-tap — prints report and exits 0 (clean) or 1 (problems found) | |
| kill $CI_PID | |
| wait $CI_PID | |
| # Start sql-tapd (proxy + gRPC server) | |
| # --listen is the address your tests will connect to | |
| # --db is the address of the real database | |
| sql-tapd --listen localhost:5433 --db "postgres://user:pass@localhost:5432/db" & | |
| TAPD_PID=$! | |
| # Start sql-tap in CI mode (background), pointing at sql-tapd's gRPC address | |
| sql-tap -ci localhost:9091 & | |
| CI_PID=$! | |
| # Run your tests through the proxy (same address as --listen above) | |
| DATABASE_URL="postgres://user:pass@localhost:5433/db" go test ./... | |
| # Stop sql-tap — prints report and exits 0 (clean) or 1 (problems found) | |
| kill $CI_PID | |
| wait $CI_PID | |
| # Stop sql-tapd proxy | |
| kill $TAPD_PID | |
| wait $TAPD_PID |
| CI_PID=$! | ||
|
|
||
| # Run your tests through the proxy | ||
| DATABASE_URL="postgres://user:pass@localhost:5433/db" go test ./... |
There was a problem hiding this comment.
The Postgres DSN in the CI snippet omits sslmode=disable, while earlier examples in this README include it (and many local Postgres setups will fail without it). Consider aligning the CI example DSN with the earlier Postgres examples to reduce copy/paste failures.
| DATABASE_URL="postgres://user:pass@localhost:5433/db" go test ./... | |
| DATABASE_URL="postgres://user:pass@localhost:5433/db?sslmode=disable" go test ./... |
| kill $CI_PID | ||
| wait $CI_PID |
There was a problem hiding this comment.
In many CI scripts that use set -e, kill $CI_PID will fail the job if sql-tap has already exited (e.g., due to a connection error) or if the PID is missing. Consider making the cleanup more robust (e.g., using a trap and/or ignoring a failed kill) so the example works reliably in CI.
| kill $CI_PID | |
| wait $CI_PID | |
| kill "$CI_PID" 2>/dev/null || true | |
| wait "$CI_PID" 2>/dev/null || true |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.