Conversation
ogzhanolguncu
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a dedicated tools module and workspace, moves tool version pins out of the main module, updates Makefile to add a setup/tools-driven workflow for install/build/fmt/generate, replaces direct tool invocations with Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Make as Makefile
participant Setup as setup target
participant ToolsMod as go/tools module
participant GoTool as `go tool`
Dev->>Make: run `make generate` / `make fmt` / `make install` / `make build`
Make->>Setup: run `setup` (new prerequisite)
Setup->>ToolsMod: ensure `go.work` + tools module present
Setup->>GoTool: prepare or reference tool binaries (via `go install` / go tool usage)
alt generate
Make->>GoTool: `go tool buf generate` / `go tool sqlc generate` -> `go generate`
else fmt
Make->>GoTool: `go tool golangci-lint run` then `go fmt`
end
GoTool-->>Make: exit status
Make-->>Dev: success / failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-09-10T04:47:17.076ZApplied to files:
📚 Learning: 2025-09-12T08:01:20.792ZApplied to files:
📚 Learning: 2025-09-01T02:33:43.791ZApplied to files:
📚 Learning: 2025-10-15T10:12:40.810ZApplied to files:
📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
📚 Learning: 2025-09-01T01:57:42.227ZApplied to files:
📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
📚 Learning: 2025-07-15T14:45:18.920ZApplied to files:
📚 Learning: 2025-08-08T15:20:40.288ZApplied to files:
📚 Learning: 2025-07-15T14:59:30.212ZApplied to files:
🪛 checkmake (0.2.2)go/Makefile[warning] 1-1: Missing required phony target "all" (minphony) ⏰ 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). (5)
🔇 Additional comments (3)
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. Comment |
|
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
go/Makefile (2)
26-32: No error propagation in tools target.If one tool installation fails (e.g., network error during
curlfor golangci-lint), the Makefile continues installing other tools. This can result in a partial setup where some tools are missing but downstream targets (fmt,generate) proceed and fail with confusing errors. Consider addingset -eto the target or refactoring to a shell script.Minor issue in practice if users run
make toolsbefore other targets, but worth addressing for robustness.Suggested fix:
tools: + set -e; \ # Check if tool is installed with correct version via grep. If not found (grep fails), install it. @buf --version 2>/dev/null | grep -q "$(BUF_VERSION)" || (echo "Installing buf@$(BUF_VERSION)..." && GOBIN=$(GOBIN) go install github.com/bufbuild/buf/cmd/buf@v$(BUF_VERSION)) && \ @echo "Installing protoc-gen-go-restate@latest..." && \ @GOBIN=$(GOBIN) go install github.com/restatedev/sdk-go/protoc-gen-go-restate@latest && \ @golangci-lint --version 2>/dev/null | grep -q "$(GOLANGCI_VERSION)" || (echo "Installing golangci-lint@$(GOLANGCI_VERSION)..." && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(GOBIN) v$(GOLANGCI_VERSION)) && \ @sqlc version 2>/dev/null | grep -q "$(SQLC_VERSION)" || (echo "Installing sqlc@$(SQLC_VERSION)..." && GOBIN=$(GOBIN) go install github.com/sqlc-dev/sqlc/cmd/sqlc@v$(SQLC_VERSION))Actually, mixing
set -ewith||chains is tricky in Makefiles. A cleaner approach is a dedicated shell script or.SHELLFLAGS := -ecat the top of the Makefile.
26-26: Optional: Address checkmake warning on target body length.Static analysis flags that the
toolstarget body (6 lines) exceeds the recommended max length of 5. This is a minor style guideline and acceptable for a "chill" review, but if you want to reduce cognitive load, consider extracting into sub-targets (e.g.,tools: install-buf install-golangci install-sqlc, with each as a separate target).This is optional and can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/Makefile(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/Makefile
🪛 checkmake (0.2.2)
go/Makefile
[warning] 26-26: Target body for "tools" exceeds allowed length of 5 (6).
(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: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
go/Makefile (3)
1-1: Good structural integration of tool versioning.The dependency chain (install → tools, fmt → tools, generate → tools) ensures tools are available before use. Renaming install to depend on tools and keeping go mod tidy is clean.
Also applies to: 22-23, 34-34, 51-51
16-20: Version variables and GOBIN setup are clear and maintainable.The constants make version updates straightforward, and deriving GOBIN from
go env GOPATHis idiomatic.
28-28: Version check patterns are robust and working correctly.The grep patterns successfully use substring matching to detect versions in tool outputs:
- buf outputs "1.58.0" and grep finds "1.59.0" pattern (will correctly trigger reinstall)
- golangci-lint outputs "golangci-lint has version 2.5.0..." and grep finds "2.5.0" pattern ✓
- sqlc follows the same substring matching approach
The implementation is not fragile—substring matching is inherently robust against minor formatting changes. Version mismatches are correctly detected and trigger reinstalls as intended.
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
Im not sure I like pinning our versions via our makefile..
I was thinking we actually use our tools.go file and use it properly e.g adding the restate as new dependency and then just running go install which should ensure that we install the correct version (as pinned by our tools.go file )
Otherwise we could use go run and pin it using go run somepinned version or we try to make it work using go tool which should be the most modern version as it got introduced in go 1.24
We already have 2 of them and they are not doing anything atm. How do we plan to pin |
|
by using the |
|
@Flo4604 aight aight lemme have another look tomorrow |
|
I'm with flo on this one |
|
Okay lemme tackle it that way |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/tools/README.md (1)
11-17: Add language identifier to fenced code block.The markdown linter flags this fenced code block as missing a language specifier.
Apply this diff:
-``` +```text repo/ ├── go.mod # Main project dependencies └── tools/ ├── go.mod # Tool dependencies (isolated) └── go.sum</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4de6753b14fd30aaad4658407ed349c5f72fd944 and 4a6a86e3b01307be092cac3cda6f99593330d003. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `go/go.sum` is excluded by `!**/*.sum` * `go/tools/go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (10)</summary> * `.gitignore` (1 hunks) * `go/Makefile` (3 hunks) * `go/apps/api/routes/v2_keys_verify_key/200_test.go` (2 hunks) * `go/go.mod` (2 hunks) * `go/pkg/db/generate.go` (1 hunks) * `go/pkg/partition/db/generate.go` (1 hunks) * `go/pkg/tools/dependencies.go` (0 hunks) * `go/tools.go` (0 hunks) * `go/tools/README.md` (1 hunks) * `go/tools/go.mod` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * go/pkg/tools/dependencies.go * go/tools.go </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * .gitignore * go/apps/api/routes/v2_keys_verify_key/200_test.go * go/tools/go.mod </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📚 Learning: 2025-09-12T08:01:20.792Z</summary>Learnt from: Flo4604
PR: #3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.**Applied to files:** - `go/pkg/db/generate.go` - `go/pkg/partition/db/generate.go` </details> <details> <summary>📚 Learning: 2025-09-01T02:33:43.791Z</summary>Learnt from: imeyer
PR: #3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from thego/directory where thebuf.yamlfile is located. This means theout: genconfiguration ingo/buf.gen.yamlgenerates files togo/gen/relative to the repository root, which aligns with the expected directory structure.**Applied to files:** - `go/Makefile` </details> </details><details> <summary>🪛 checkmake (0.2.2)</summary> <details> <summary>go/Makefile</summary> [warning] 1-1: Missing required phony target "all" (minphony) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>go/tools/README.md</summary> 11-11: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: Test Go API Local / Test * GitHub Check: Test API / API Test Local * GitHub Check: Build / Build * GitHub Check: Test Packages / Test </details> <details> <summary>🔇 Additional comments (9)</summary><blockquote> <details> <summary>go/pkg/db/generate.go (1)</summary><blockquote> `4-4`: **LGTM! Properly uses `go tool` prefix.** The change correctly updates the directive to use `go tool sqlc generate`, ensuring the workspace-managed version from `tools/go.mod` is used instead of any globally installed version. </blockquote></details> <details> <summary>go/go.mod (1)</summary><blockquote> `124-124`: **Dependency updates look routine.** These indirect dependency version bumps are standard maintenance updates that align with the module's evolving dependency graph. Also applies to: 140-140, 151-152, 215-215 </blockquote></details> <details> <summary>go/tools/README.md (1)</summary><blockquote> `1-147`: **Comprehensive documentation of the tools management approach.** This README effectively documents the architecture, rationale, and usage patterns for the workspace-based tooling approach. The emphasis on using the `go tool` prefix (lines 92-118) and the maintenance guide (lines 83-147) are particularly valuable for preventing common mistakes. </blockquote></details> <details> <summary>go/pkg/partition/db/generate.go (1)</summary><blockquote> `3-3`: **LGTM! Consistent use of `go tool` prefix.** This matches the pattern in `go/pkg/db/generate.go` and correctly delegates to the workspace-managed sqlc version. </blockquote></details> <details> <summary>go/Makefile (5)</summary><blockquote> `18-21`: **Workspace setup correctly bootstraps the tooling approach.** The `setup` target properly initializes the Go workspace linking the main module and tools module, aligning with the architecture documented in `go/tools/README.md`. --- `25-30`: **Tools target correctly delegates to workspace management.** Instead of installing tools directly (old approach), this now just prints usage instructions for `go tool` commands. The actual tool versions are managed via the `go/tools/go.mod` module and workspace. --- `32-34`: **Formatting correctly uses workspace-managed golangci-lint.** The `go tool golangci-lint` invocation ensures the version from `tools/go.mod` is used. --- `49-53`: **Code generation properly uses workspace-managed tools.** Both `go tool buf` invocations (lines 50-51) and the `go generate` command (line 52) will use workspace-managed tool versions, ensuring consistency across environments. --- `1-53`: **All dependencies verified—tools module is properly configured.** Verification confirms that `go/tools/go.mod` exists with all required tools declared via the modern Go `tool` directive: - buf, golangci-lint, sqlc, and protoc-gen-go-restate are all present - The workspace initialization via `go work init . tools` will function as intended </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
in our go.mod @imeyer |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
go/tools/README.md (2)
11-17: Add language specifiers to fenced code blocks for clarity and to satisfy markdown linting.Lines 11–17 (directory structure) and 30–37 (Go code) should have language identifiers specified.
Apply this diff to add language specifiers:
- ``` + ``` repo/ ├── go.mod # Main project dependencies └── tools/ ├── go.mod # Tool dependencies (isolated) └── go.sum - ``` + ``` **Why a workspace?** - **Unified tool access** - Run `go tool <name>` from anywhere in the repo without specifying which module - **No `-modfile`** - Without workspace, you'd need `go tool -modfile=tools/go.mod <name>` every time - **Multi-module development** - If you're working on both the main code and tool configurations, the workspace makes it seamless - ```go + ```go go 1.25.1 use ( . // Main module ./tools // Tools module ) - ``` + ```Also applies to: 30-37
85-90: Document potential tool-to-tool dependency conflict trade-offs.The README documents adding tools to a single
tools/go.modmodule. A past review noted that consolidating multiple tools in one module could cause issues if tool A's transitive dependencies conflict with tool B's transitive dependencies. The current design uses a single shared tools module rather than separate modules per tool.Consider documenting this trade-off or mitigation strategy (e.g., how conflicts are monitored or resolved), so future maintainers understand the design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/tools/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/tools/README.md
🪛 markdownlint-cli2 (0.18.1)
go/tools/README.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Test Packages / Test
🔇 Additional comments (3)
go/tools/README.md (3)
45-58: Clarify setup requirements and auto-creation behavior.Lines 50–51 indicate
make installcreates the workspace, while lines 53–58 suggest the workspace is auto-created on subsequent tool commands. The relationship between these flows is unclear and may confuse developers about what's required during setup versus what's optional. Based on the PR description, asetuptarget was introduced that makes other targets depend on it.Clarify whether:
- Setup is a one-time prerequisite that must run before any tool command?
- Or is setup optional and workspace auto-creation handles it?
- What is the actual role of
make installvsmake toolsvsmake setup(if it exists)?Please verify the actual Makefile behavior for the
setup,install,fmt, andgeneratetargets to ensure the documentation accurately reflects the workflow.
92-142: Excellent critical mistakes and verification guidance.Lines 92–142 provide clear, actionable examples of common mistakes and verification steps. The wrong vs. correct examples (lines 92–118) effectively communicate the importance of the
go toolprefix, and the version verification guidance (lines 120–126) gives developers a practical way to confirm they're using pinned versions. Thegrepsearch pattern for audit (line 133) is also helpful for enforcement.
75-79: No issues found. Thego get -toolsyntax is correct for Go 1.24+.The syntax
go get -tool github.com/bufbuild/buf/cmd/buf@v1.60.0matches the documented standard for updating tool dependencies in Go 1.24+. The command and documentation are accurate—users following this guide will work with the correct syntax.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/Makefile (1)
1-1: Static analysis: Missingalltarget.The checkmake linter suggests adding a
.PHONY alltarget and correspondingall:target as a common convention. While not required in modern Makefiles, adding one can improve usability:-.PHONY: install setup tools fmt test-unit test-integration test-integration-long test-stress test build generate pull up clean k8s-check k8s-up k8s-down k8s-reset k8s-status start-mysql start-ctrl start-all dev +.PHONY: all install setup tools fmt test-unit test-integration test-integration-long test-stress test build generate pull up clean k8s-check k8s-up k8s-down k8s-reset k8s-status start-mysql start-ctrl start-all dev + +all: build testThis is optional but follows Makefile best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/Makefile(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
📚 Learning: 2025-09-10T04:47:17.076Z
Learnt from: imeyer
PR: unkeyed/unkey#3941
File: go/deploy/spire/Makefile:110-110
Timestamp: 2025-09-10T04:47:17.076Z
Learning: In the SPIRE Makefile (go/deploy/spire/Makefile), the create-users target has been removed, so any references to create-users in target dependencies should also be removed. Both install-agent and install-server targets had create-users dependencies that needed to be cleaned up.
Applied to files:
go/Makefile
📚 Learning: 2025-09-12T08:01:20.792Z
Learnt from: Flo4604
PR: unkeyed/unkey#3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.
Applied to files:
go/Makefile
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/Makefile
📚 Learning: 2025-09-01T01:57:42.227Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/metald.proto:5-9
Timestamp: 2025-09-01T01:57:42.227Z
Learning: In the unkeyed/unkey repository, buf is configured to properly resolve metald proto imports like "metald/v1/vm.proto" without needing the full "go/proto/" prefix. The buf lint command `buf lint --path proto/metald` passes successfully with these relative import paths.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/Makefile
📚 Learning: 2025-10-15T10:12:40.810Z
Learnt from: Flo4604
PR: unkeyed/unkey#4098
File: go/proto/ctrl/v1/deployment.proto:33-36
Timestamp: 2025-10-15T10:12:40.810Z
Learning: In the Unkey codebase proto files (ctrl/v1/build.proto, ctrl/v1/deployment.proto, hydra/v1/deployment.proto), use `dockerfile_path` (not `docker_file_path`) for consistency in generated Go field names.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/Makefile
🪛 checkmake (0.2.2)
go/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
go/Makefile (4)
16-18: Verify tools directory exists before workspace init.The
setuptarget conditionally initializes the workspace, but doesn't validate that thetoolsdirectory exists. Iftools/go.modis missing or the directory structure is incomplete,go work init . toolswill fail cryptically.Consider adding a validation check:
# One-time setup for new developers setup: + @if [ ! -d "tools" ]; then echo "ERROR: tools directory not found. Ensure go/tools module exists."; exit 1; fi @[ -f go.work ] || go work init . toolsAlternatively, if
toolsis optional or created elsewhere, document this assumption clearly.
23-28: Thetoolstarget is now informational only—consider clarifying intent or consolidating.This target no longer installs or manages tools; it only prints usage documentation. The actual tooling is managed by the workspace setup in the
setuptarget.This is appropriate as documentation, but verify:
- Is this target ever invoked directly by developers, or is it primarily documentation?
- Should
make toolsrunsetupas a dependency to ensure workspace is ready before printing?If developers rely on
make toolsto ensure tools are installed, consider addingsetupas a dependency:-tools: setup +tools: setup @echo "Tools are ready. Usage:"(Already present in your code, so this is LGTM.)
30-32: Minor: Consider grouping formatting operations infmttarget.Lines 30–32 apply
go fmtfollowed bygo tool golangci-lint run. Both are formatting operations; grouping them (especially ensuring go fmt runs before linting) is correct. However, ifgolangci-lintfixes are expected to be applied, ensure the order reflects that intent.Current order (go fmt → golangci-lint) is reasonable. No action required unless linter fixes should be applied after go fmt.
47-51: Verify buf templates exist before generate invokes them.Lines 48–49 invoke
go tool buf generatewith specific templates (buf.gen.connect.yaml,buf.gen.restate.yaml). Ensure these template files are present in the same directory as the Makefile.If either template is missing,
buf generatewill fail. Consider adding a validation step or ensuring template discovery is documented.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/04/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/Makefile (1)
1-1: Static analysis: missingalltarget.Checkmake flagged a missing
allphony target. While not strictly required, it's a common convention for Makefiles. Consider adding:-.PHONY: install setup tools fmt test-unit ... +.PHONY: all install setup tools fmt test-unit ... + +all: buildOr another sensible default. This is optional and can be deferred if not a project convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go/Makefile(3 hunks)go/go.mod(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
📚 Learning: 2025-09-01T01:57:42.227Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3899
File: go/proto/metald/v1/metald.proto:5-9
Timestamp: 2025-09-01T01:57:42.227Z
Learning: In the unkeyed/unkey repository, buf is configured to properly resolve metald proto imports like "metald/v1/vm.proto" without needing the full "go/proto/" prefix. The buf lint command `buf lint --path proto/metald` passes successfully with these relative import paths.
Applied to files:
go/go.modgo/Makefile
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/go.mod
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/go.modgo/Makefile
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/go.modgo/Makefile
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/go.modgo/Makefile
📚 Learning: 2025-09-10T04:47:17.076Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3941
File: go/deploy/spire/Makefile:110-110
Timestamp: 2025-09-10T04:47:17.076Z
Learning: In the SPIRE Makefile (go/deploy/spire/Makefile), the create-users target has been removed, so any references to create-users in target dependencies should also be removed. Both install-agent and install-server targets had create-users dependencies that needed to be cleaned up.
Applied to files:
go/Makefile
📚 Learning: 2025-09-12T08:01:20.792Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.
Applied to files:
go/Makefile
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/Makefile
📚 Learning: 2025-10-15T10:12:40.810Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4098
File: go/proto/ctrl/v1/deployment.proto:33-36
Timestamp: 2025-10-15T10:12:40.810Z
Learning: In the Unkey codebase proto files (ctrl/v1/build.proto, ctrl/v1/deployment.proto, hydra/v1/deployment.proto), use `dockerfile_path` (not `docker_file_path`) for consistency in generated Go field names.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/Makefile
🪛 checkmake (0.2.2)
go/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
🔇 Additional comments (7)
go/Makefile (5)
16-18: Verifygo work inithandles existing go.work gracefully.Line 18 uses
go work init . toolswith a condition[ -f go.work ] ||. However,go work initwill fail ifgo.workalready exists. Consider usinggo work useinstead or adding error handling:setup: - @[ -f go.work ] || go work init . tools + @go work init . tools 2>/dev/null || go work use . toolsThis ensures the workspace is properly configured whether the file exists or not. Verify this pattern works correctly in your local environment.
46-50: No validation check needed—buf is properly declared as a tool dependency.The repository already has robust tool management in place:
- buf is declared in
go/tools/go.modin the tool block- The setup target initializes the workspace with
go work init . tools, which includes the tools module- The generate target already depends on setup, ensuring initialization completes before buf commands run
With Go's workspace-based tool management,
go tool bufwill automatically use the version fromgo/tools/go.mod. If the setup fails, that error will surface; if it succeeds, buf is available. The suggested validation check is redundant—if buf becomes unavailable, the subsequentgo tool buf generatecommand will fail with a clear error anyway.
23-28: The review comment is incorrect regarding the dependency chain and impact.The review claims that downstream targets (
fmt,generate) may fail silently if tools aren't installed because thetoolstarget doesn't verify tool availability. This diagnosis is fundamentally wrong:
Dependency chain is incorrect: The
fmttarget (line 30) andgeneratetarget (line 46) depend directly onsetup, not on thetoolstarget. Therefore, thetoolstarget's verification status has zero impact on whether these targets succeed or fail.Failures are not silent: If
buforgolangci-lintare missing, commands likego tool buf generateandgo tool golangci-lint runwill fail loudly with "command not found" errors, not silently.Suggested fixes are misdirected:
- Option A (rename to
tools-usage) doesn't affectfmt/generatesince they don't depend on it- Option B (add verification commands to
toolstarget) won't prevent failures infmt/generatesince they never invoke thetoolstargetThe
setuptarget correctly initializes the workspace viago work init . tools, and bothfmtandgenerateproperly depend onsetup. The Makefile works as designed—no issue exists with the dependency chain.Likely an incorrect or invalid review comment.
30-32: Remove the suggested error handling check—the current setup already validates tool availability.The
setuptarget initializes go.work by runninggo work init . tools, which fails immediately with a clear error if the tools directory is missing or if go/tools/go.mod is invalid. Sincefmtdepends onsetup, the tool availability is guaranteed beforego tool golangci-lint runexecutes. If golangci-lint is missing from go/tools/go.mod, thego toolinvocation will fail with an explicit error message. No additional validation is needed.Likely an incorrect or invalid review comment.
16-50: Setup is correctly configured; add go.work to .gitignore.The tools setup is well-implemented and documented. Verified:
go/tools/go.modexists with all pinned tool versions (buf, golangci-lint, sqlc, oapi-codegen, protoc-gen-go-restate)go/tools/README.mdprovides clear documentation on architecture, setup, and critical best practices (e.g., always usego toolprefix)make setupcorrectly initializes the workspace viago work init . toolsmake fmtandmake generateare properly configured to usego toolcommandsHowever,
go.workshould be added togo/.gitignore. Since it's generated locally by each developer's setup, excluding it prevents merge conflicts and unnecessary diffs across branches. Addgo.workto the existing.gitignorefile.go/go.mod (2)
135-135: Indirect dependency updates appear as expected side-effects.The changes to
google/pprof,moby/sys/atomicwriter,onsi/ginkgo/v2,onsi/gomega, andgotest.tools/v3appear to be automatic version updates from the dependency resolver after removing the tool requires. These are indirect dependencies (testing and profiling utilities from the Docker/Moby ecosystem).Assuming the tool pinning PR objectives are primarily about the removed direct requires (buf, sqlc, oapi-codegen, etc.), these indirect updates are expected collateral.
To confirm these versions are secure and intentional, consider:
- Running
go mod verifyto confirm all checksums are correct- Reviewing the go.sum file for any unexpected additions
Also applies to: 151-151, 162-163, 227-227
7-58: Tool migration implementation is complete and correct—no further action needed.Verification confirms:
- All 5 tools (buf, golangci-lint/v2, oapi-codegen/v2, sqlc, protoc-gen-go-restate) properly defined in
go/tools/go.modwith Go 1.22+tool()directive syntax ✓- Main
go/go.modcorrectly removes direct tool requires ✓- Makefile
toolstarget creates workspace viago work init . tools✓- All
go:generatedirectives updated togo tool sqlcandgo tool oapi-codegeninvocations ✓- No broken imports or lingering tool references ✓
- Version comments in generated files (sqlc v1.29.0, oapi-codegen/v2 v2.5.0) are tool output metadata, not code dependencies ✓
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
go/Makefile (1)
1-1: Addallphony target to align with Make conventions.The .PHONY declaration on line 1 includes many targets but lacks the conventional
alltarget, which typically builds the project. This is a minor best-practice gap flagged by checkmake.Consider adding an
alltarget (e.g.,all: build test) if applicable to your workflow, or addallto the .PHONY list as a no-op if the project uses a different primary target.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/Makefile(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
📚 Learning: 2025-09-12T08:01:20.792Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.
Applied to files:
go/Makefile
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/Makefile
📚 Learning: 2025-10-15T10:12:40.810Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4098
File: go/proto/ctrl/v1/deployment.proto:33-36
Timestamp: 2025-10-15T10:12:40.810Z
Learning: In the Unkey codebase proto files (ctrl/v1/build.proto, ctrl/v1/deployment.proto, hydra/v1/deployment.proto), use `dockerfile_path` (not `docker_file_path`) for consistency in generated Go field names.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/Makefile
📚 Learning: 2025-07-15T14:45:18.920Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3564
File: go/cmd/cli/commands/deploy/flags.go:17-20
Timestamp: 2025-07-15T14:45:18.920Z
Learning: In the go/cmd/cli/commands/deploy/ directory, ogzhanolguncu prefers to keep potentially temporary features (like UNKEY_DOCKER_REGISTRY environment variable) undocumented in help text if they might be deleted in the future, to avoid documentation churn.
Applied to files:
go/Makefile
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/Makefile
🪛 checkmake (0.2.2)
go/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (3)
go/Makefile (3)
30-32:go tool golangci-lintrequires prior tool installation.Line 32 invokes
go tool golangci-lint run, which assumes golangci-lint has been installed into the Go cache. This will fail if the setup target (line 16) does not actually install tools or if tools are not already present.The dependency chain is:
fmt→setup→go work init . tools(creates workspace)- Then:
go tool golangci-lint run(searches for installed binary)Confirm that setup (or another target) installs golangci-lint before fmt is run, otherwise developers will see "go tool golangci-lint: not found" errors.
Add an installation step to setup (lines 16-18) or document the prerequisite for
make fmtto work.
46-48:go tool bufinvocations depend on prior tool installation.Lines 47-48 invoke
go tool buf generatewith specific templates, but this assumes buf has been installed into the Go cache by the setup target (line 16). Without actual installation in setup, these commands will fail with "go tool buf: not found".Verify that the setup target installs all required tools (buf, golangci-lint, sqlc, protoc-gen-go-restate) or document how they are pre-installed before developers run
make generate.
23-28: The review comment is incorrect.The repository uses Go 1.24+'s native
tooldirective to manage development tools, and tool versions are already pinned ingo/tools/go.mod. Specifically:
- buf v1.59.0
- golangci-lint/v2 v2.5.0
- sqlc v1.29.0
- protoc-gen-go-restate (from restatedev/sdk-go)
The installation mechanism is present and functional: The
setuptarget (line 16–18) runsgo work init . tools, which creates a Go workspace linking the main module and the separate tools module. Tools are accessed viago tool <name>commands, automatically using the pinned versions fromtools/go.mod. The Makefile'sgenerateandfmttargets already invoke these commands (go tool buf generate,go tool golangci-lint run), so the PR objective to pin and use consistent tool versions is already satisfied.The
toolstarget at lines 23–28 is appropriately informational—it documents how to invoke the tools, not a mechanism for installation itself.Likely an incorrect or invalid review comment.
4 participants
What does this PR do?
This PR install pinned versions of some packages. Such as,
sqlc,buf,golangci-lint. This will ensure that whoever runs themake generateormake fmtwill run same version ofsqlcorgolangci-lintFixes #4065
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?
make toolsandmake generatemake sure it works in/godir.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated