Merged
Conversation
- pins as many container versions as possible - combanitoric tests are less combanitoric - organize api/integration/multi_node_ratelimiting/generated/* with build tags based on the number of nodes in the workload - update Makefile targets - update prometheus config to be forward compatible with newer versions of prometheus
imeyer
requested review from
Flo4604,
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Contributor
📝 WalkthroughWalkthroughIn CI, timeouts and steps are adjusted: API test timeout increased; Go API job adds Docker Buildx, refactors test steps with gated stress tests. Prometheus config adds validation scheme, scrape protocols, HTTP/2, and redirect following. Go Makefile reorganizes test targets and container orchestration. Test generator switches to curated cases and adds per-case build tags. Changes
Sequence Diagram(s)sequenceDiagram
participant GA as GitHub Actions
participant Buildx as Docker Buildx
participant Make as make (go/)
participant Tests as go test
GA->>Buildx: setup-buildx
GA->>Make: make test-integration (env: DOCKER_BUILDKIT=1, COMPOSE_DOCKER_CLI_BUILD=1)
Make->>Make: up (pull, docker compose up -d, CH migration retry)
Make->>Tests: go test -tags=integration ...
Note over GA: On main/tag only
GA->>Make: make test-stress (same env)
Make->>Tests: go test -tags=stress,integration,integration_long ...
sequenceDiagram
participant Gen as generate_tests/main.go
participant FS as Filesystem
Gen->>Gen: Build extremeEdgeCases + realisticCombinations
loop for each TestCase
Gen->>Gen: tag = tc.BuildTag()
Gen->>FS: write test file with //go:build <tag> (if any)
end
Gen->>FS: print summary counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (5)
.github/workflows/job_test_api_local.yaml (1)
29-31: Pin Goose to v3.24.3 for reproducible CI and buildsWe found two places installing Goose with
@latest, which risks unexpected breakages. Update both to the known-good tag v3.24.3 (latest as of May 7):• .github/workflows/job_test_api_local.yaml (around lines 29–31)
• internal/clickhouse/Dockerfile (line 4)Example replacements:
- - name: Install Goose - run: go install github.com/pressly/goose/v3/cmd/goose@latest + - name: Install Goose + run: go install github.com/pressly/goose/v3/cmd/goose@v3.24.3- RUN go install github.com/pressly/goose/v3/cmd/goose@latest + RUN go install github.com/pressly/goose/v3/cmd/goose@v3.24.3go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go (3)
69-87: Add doc comments for exported methods to meet Go doc guidelines.Both methods are exported and currently undocumented.
-func (tc TestCase) PackageName() string { +// PackageName returns a unique, go-safe package name for the generated test case. +func (tc TestCase) PackageName() string { @@ -func (tc TestCase) TestName() string { +// TestName returns a stable test function name for the generated test case. +func (tc TestCase) TestName() string {
133-137: Harden template parsing to fail on missing keys.Option("missingkey=error") prevents silent template omissions.
- tmpl, err := template.New("test").Parse(testTemplate) + tmpl, err := template.New("test").Option("missingkey=error").Parse(testTemplate)
153-167: Close the file via defer to avoid leaks on early panics.Current code closes after Execute; aborts won’t close the handle.
- file, err := os.Create(filePath) + file, err := os.Create(filePath) if err != nil { panic(err) } - err = tmpl.Execute(file, tc) + defer func() { + _ = file.Close() + }() + + err = tmpl.Execute(file, tc) if err != nil { panic(err) } - - err = file.Close() - if err != nil { - panic(err) - }go/Makefile (1)
26-38: Optional: factor the ClickHouse migration loop into a dedicated target to shorten “up”.This addresses the long body warning and improves reuse.
-up: pull - @docker compose -f ../deployment/docker-compose.yaml up -d planetscale mysql redis clickhouse s3 otel - @echo "Starting ClickHouse migrations (will retry if ClickHouse isn't ready)..." - @for i in {1..10}; do \ - echo "Migration attempt $$i..."; \ - if docker compose -f ../deployment/docker-compose.yaml run --rm clickhouse_migrator; then \ - echo "Migrations completed successfully!"; \ - break; \ - else \ - echo "Migration failed, retrying in 5 seconds..."; \ - sleep 5; \ - fi; \ - done +up: pull migrate-clickhouse + @docker compose -f ../deployment/docker-compose.yaml up -d planetscale mysql redis clickhouse s3 otel + +migrate-clickhouse: + @echo "Starting ClickHouse migrations (will retry if ClickHouse isn't ready)..." + @for i in {1..10}; do \ + echo "Migration attempt $$i..."; \ + if docker compose -f ../deployment/docker-compose.yaml run --rm clickhouse_migrator; then \ + echo "Migrations completed successfully!"; \ + break; \ + else \ + echo "Migration failed, retrying in 5 seconds..."; \ + sleep 5; \ + fi; \ + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (63)
go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0010_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000010000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000010000_load01_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000010000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000300000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0010_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0010_duration000060000_load01_50_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000010000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000010000_load01_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000010000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000300000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000300000_load01_50_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0010_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0010_duration000060000_load01_50_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0100_duration000010000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0100_duration000010000_load01_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0100_duration000010000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0100_duration000300000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes05_limit0100_duration000300000_load01_50_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000030000_load02_00_windows005/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000300000_load01_00_windows003/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**
📒 Files selected for processing (5)
.github/workflows/job_test_api_local.yaml(1 hunks).github/workflows/job_test_go_api_local.yaml(1 hunks)deployment/config/prometheus.yml(1 hunks)go/Makefile(3 hunks)go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go
🧠 Learnings (4)
📚 Learning: 2025-08-08T19:52:42.073Z
Learnt from: imeyer
PR: unkeyed/unkey#3765
File: .github/workflows/job_detect_changes.yaml:43-43
Timestamp: 2025-08-08T19:52:42.073Z
Learning: In the unkeyed/unkey repository, the workflows `.github/workflows/job_test_api_local.yaml` and `.github/workflows/job_test_go_api_local.yaml` should keep their Blacksmith self-hosted runners (blacksmith-4vcpu-ubuntu-2404 and blacksmith-8vcpu-ubuntu-2404 respectively) as these are performance-critical test jobs that require dedicated resources.
Applied to files:
.github/workflows/job_test_api_local.yaml
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go integration tests with real dependencies
Applied to files:
go/Makefile
📚 Learning: 2025-07-21T18:05:58.236Z
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/deploy/{assetmanagerd,billaged,builderd,metald}/**/Makefile : Use `make build` to test that the binary builds.
Applied to files:
go/Makefile
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Use table-driven tests in Go
Applied to files:
go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go
🪛 YAMLlint (1.37.1)
deployment/config/prometheus.yml
[error] 9-9: trailing spaces
(trailing-spaces)
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
🪛 checkmake (0.2.2)
go/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 26-26: Target body for "up" exceeds allowed length of 5 (12).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
.github/workflows/job_test_go_api_local.yaml (1)
28-31: Ensure stress tests run on direct pushes to main
In reusable workflows called viaworkflow_call,github.event_namereflects the original trigger (e.g.,push,pull_request). Currently the stress‐test step only runs onmerge_groupor tag events. If you invoke this workflow on a direct push tomain, those runs will be skipped. Please verify whether any callers trigger this onpushtomain. If so, broaden the condition as follows:• File: .github/workflows/job_test_go_api_local.yaml
• Step: “Run stress tests” (around lines 28–31)- if: github.event_name == 'merge_group' || startsWith(github.ref, 'refs/tags/') + if: github.event_name == 'merge_group' \ + || startsWith(github.ref, 'refs/tags/') \ + || (github.event_name == 'push' && github.ref == 'refs/heads/main')If no caller ever invokes this on a push to
main, you can leave the condition as is.deployment/config/prometheus.yml (1)
1-5: Prometheus image tag already covers new config fields
Your Docker Compose usesprom/prometheus:latest, which always pulls the current stable release and includes support for:
metric_name_validation_scheme(added in v2.30.0)scrape_protocols(added in v2.43.0)No changes are required to enable these fields. If you need reproducible deployments, consider pinning the image to a specific version (≥ v2.43.0).
• deployment/docker-compose.yaml:242 (image: prom/prometheus:latest)
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/11/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
18 tasks
Merged
18 tasks
18 tasks
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
2 participants
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.
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Chores
Tests