Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds POST /v2/deploy.generateUploadUrl (OpenAPI, types, route, handler, tests); wires a CTRL BuildServiceClient through runtime and routes; centralizes Connect→fault error translation in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Router as API Routes
participant Handler
participant CtrlClient
participant Database
participant S3
Client->>API_Router: POST /v2/deploy.generateUploadUrl
API_Router->>Handler: invoke Handler
Handler->>Handler: bind & validate projectId
Handler->>CtrlClient: GenerateUploadURL(request)
CtrlClient->>Database: FindProjectById(projectId)
alt project not found
Database-->>CtrlClient: NotFound error
CtrlClient-->>Handler: NotFound error
Handler->>Handler: ctrlclient.HandleError(err,"generate upload url")
Handler-->>Client: 404 "Project not found."
else project exists
Database-->>CtrlClient: Project found
CtrlClient->>S3: create presigned upload URL & context
S3-->>CtrlClient: uploadUrl, context
CtrlClient-->>Handler: upload metadata
Handler-->>Client: 200 {uploadUrl, context}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
svc/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
📚 Learning: 2025-09-01T15:10:44.959ZApplied to files:
📚 Learning: 2025-09-11T08:17:21.423ZApplied to files:
📚 Learning: 2025-09-15T18:12:01.503ZApplied to files:
📚 Learning: 2026-01-13T17:46:51.335ZApplied to files:
📚 Learning: 2025-08-27T11:02:17.155ZApplied to files:
🧬 Code graph analysis (1)svc/api/routes/register.go (2)
⏰ 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). (2)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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 |
ogzhanolguncu
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
mcstepp and
perkinsjr
as code owners
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
svc/api/routes/v2_deploy_create_deployment/404_test.go (1)
93-98: Semantic mismatch: "environment not found" test asserts "Project not found." message.The test scenario at line 85 explicitly sets a non-existent environment (
"nonexistent-env"), yet the assertion at line 98 expects"Project not found.". This conflates two distinct error conditions and could mask environment-related bugs.Also, line 94 has a truncated format string:
"expected , received: %s"is missing the expected status.Suggested fix for the format string
- require.Equal(t, http.StatusInternalServerError, res.Status, "expected , received: %s", res.RawBody) + require.Equal(t, http.StatusInternalServerError, res.Status, "expected 500, received: %s", res.RawBody)
🤖 Fix all issues with AI agents
In @svc/api/openapi/openapi-generated.yaml:
- Around line 4247-4296: Update the OpenAPI operation for
/v2/deploy.generateUploadUrl (operationId: generateUploadUrl) to not claim
permission enforcement in the description and to add a 403 response entry;
specifically, remove or reword “Requires a valid root key with appropriate
permissions.” to indicate authentication only (or that permission checks are not
yet implemented), and add a "403" response with the existing error schema (e.g.,
Unauthorized/NotAllowed style or BadRequestErrorResponse pattern) so the
contract includes forbidden responses once checks are added.
In @svc/api/routes/v2_deploy_generate_upload_url/404_test.go:
- Around line 54-59: The test in 404_test.go is asserting a 500
InternalServerError using openapi.InternalServerErrorResponse while the file
name and OpenAPI spec expect a 404; either rename the test file to reflect a 500
case or change the assertions to expect a 404 and the NotFound response type:
replace openapi.InternalServerErrorResponse with the appropriate
openapi.NotFoundResponse (or similar) and update require.Equal checks to
http.StatusNotFound and the correct error.Type/Detail; also ensure the
handler/route under test returns 404 for missing projects if you choose the
RESTful fix.
🧹 Nitpick comments (8)
svc/ctrl/services/build/backend/depot/generate_upload_url.go (1)
40-53: Consider extracting expiration duration to a constant.The expiration is expressed as
15*time.Minuteon line 41 and as900(seconds) on line 53. If either value changes, they could fall out of sync.♻️ Suggested refactor
+const uploadURLExpiration = 15 * time.Minute + func (s *Depot) GenerateUploadURL( ctx context.Context, req *connect.Request[ctrlv1.GenerateUploadURLRequest], ) (*connect.Response[ctrlv1.GenerateUploadURLResponse], error) { // ... existing code ... // Generate presigned URL (15 minutes expiration) - uploadURL, err := s.storage.GenerateUploadURL(ctx, buildContextPath, 15*time.Minute) + uploadURL, err := s.storage.GenerateUploadURL(ctx, buildContextPath, uploadURLExpiration) // ... existing code ... return connect.NewResponse(&ctrlv1.GenerateUploadURLResponse{ UploadUrl: uploadURL, BuildContextPath: buildContextPath, - ExpiresIn: 900, // 15 minutes + ExpiresIn: int64(uploadURLExpiration.Seconds()), }), nil }svc/ctrl/services/build/backend/docker/generate_upload_url.go (1)
36-38: Consider using a more collision-resistant identifier.While
UnixNanoprovides millisecond-level uniqueness, under high concurrency on a single machine, there's a theoretical (though unlikely) collision risk. A UUID or similar would be more robust.Optional: Use UUID for build context path
+import "github.com/google/uuid" + // Generate unique S3 key for this build context -buildContextPath := fmt.Sprintf("%s/%d.tar.gz", - unkeyProjectID, - time.Now().UnixNano()) +buildContextPath := fmt.Sprintf("%s/%s.tar.gz", + unkeyProjectID, + uuid.New().String())svc/api/internal/ctrlclient/errors.go (1)
30-34: Consider making the NotFound message context-aware.The
CodeNotFoundcase hardcodes "Project not found." but this helper may be reused for other resource types in the future. UnlikeInvalidArgumentand the default case, it doesn't use thecontextparameter.If this is intentional because the current usage is limited to project-related operations, this is fine. Otherwise, consider:
♻️ Suggested improvement for flexibility
case connect.CodeNotFound: return fault.Wrap(err, fault.Code(codes.Data.Project.NotFound.URN()), - fault.Public("Project not found."), + fault.Public(fmt.Sprintf("Resource not found for %s.", context)), )svc/api/openapi/openapi-generated.yaml (2)
432-451: Tighten the new request/response schemas (additionalProperties: false) for consistency and stricter contracts.
Most v2 request/response bodies in this spec disallow unknown fields; these new ones currently allow them implicitly.Proposed change (apply in the source OpenAPI spec, then re-generate)
V2DeployGenerateUploadUrlRequestBody: type: object required: - projectId + additionalProperties: false properties: projectId: type: string minLength: 1 description: Unkey project ID for which to generate the upload URL example: "proj_123abc" V2DeployGenerateUploadUrlResponseBody: type: object required: - meta - data + additionalProperties: false properties: meta: $ref: "#/components/schemas/Meta" data: $ref: "#/components/schemas/V2DeployGenerateUploadUrlResponseData"
2634-2653: Add basic validation hints (format: uri,minimum) to reduce client-side ambiguity.
This improves generated SDK typing and validation without changing behavior.Proposed change (apply in the source OpenAPI spec, then re-generate)
V2DeployGenerateUploadUrlResponseData: type: object required: - uploadUrl - context - expiresIn + additionalProperties: false properties: uploadUrl: type: string + format: uri description: Presigned PUT URL for uploading the build context tar file example: "https://s3.amazonaws.com/bucket/path?signature=..." context: type: string description: S3 path to use in the createDeployment request when building from source example: "proj_123abc/ctx_456def.tar.gz" expiresIn: type: integer format: int64 + minimum: 1 description: Number of seconds until the upload URL expires example: 3600svc/api/routes/v2_deploy_generate_upload_url/401_test.go (1)
63-65: UseUnauthorizedErrorResponsefor proper error response validation.The test uses
handler.Response(success body type) instead of the appropriate error response type. Compare with404_test.gowhich correctly usesopenapi.InternalServerErrorResponse. This limits the test's ability to validate the actual error structure.♻️ Suggested fix
+ "github.com/unkeyed/unkey/svc/api/openapi" ... - res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) + res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) require.Equal(t, http.StatusUnauthorized, res.Status, "expected 401, received: %s", res.RawBody) require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Error.Type) + require.Equal(t, http.StatusUnauthorized, res.Body.Error.Status)svc/api/routes/v2_deploy_generate_upload_url/400_test.go (1)
85-113: Consider using consistent error response type for auth-related 400 tests.The "missing authorization header" and "malformed authorization header" tests use
handler.Responseinstead ofopenapi.BadRequestErrorResponse, which limits the ability to validate the error structure. Consider aligning with the first two test cases for consistency.♻️ Suggested improvement
t.Run("missing authorization header", func(t *testing.T) { headers := http.Header{ "Content-Type": {"application/json"}, // No Authorization header } req := handler.Request{ ProjectId: projectID, } - res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) + res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, req) require.Equal(t, http.StatusBadRequest, res.Status) require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Meta.RequestId) }) t.Run("malformed authorization header", func(t *testing.T) { headers := http.Header{ "Content-Type": {"application/json"}, "Authorization": {"malformed_header"}, } req := handler.Request{ ProjectId: projectID, } - res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) + res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, req) require.Equal(t, http.StatusBadRequest, res.Status) require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Meta.RequestId) })svc/api/routes/v2_deploy_generate_upload_url/handler.go (1)
25-30: Consider removing unused struct fields.
LoggerandDBare declared but not used in this handler. If they're not needed for future permission checks mentioned in the PR, consider removing them to reduce noise.♻️ Optional cleanup
type Handler struct { - Logger logging.Logger - DB db.Database Keys keys.KeyService CtrlClient ctrlv1connect.BuildServiceClient }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
svc/api/internal/ctrlclient/BUILD.bazelsvc/api/internal/ctrlclient/errors.gosvc/api/openapi/gen.gosvc/api/openapi/openapi-generated.yamlsvc/api/openapi/openapi-split.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlRequestBody.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseBody.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/index.yamlsvc/api/routes/BUILD.bazelsvc/api/routes/register.gosvc/api/routes/services.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/BUILD.bazelsvc/api/routes/v2_deploy_create_deployment/handler.gosvc/api/routes/v2_deploy_generate_upload_url/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.gosvc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_generate_upload_url/404_test.gosvc/api/routes/v2_deploy_generate_upload_url/BUILD.bazelsvc/api/routes/v2_deploy_generate_upload_url/handler.gosvc/api/run.gosvc/ctrl/services/build/backend/depot/generate_upload_url.gosvc/ctrl/services/build/backend/docker/generate_upload_url.go
🧰 Additional context used
🧠 Learnings (9)
📓 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-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
svc/api/routes/v2_deploy_generate_upload_url/404_test.gosvc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_generate_upload_url/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.go
📚 Learning: 2025-09-25T18:49:28.948Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: go/apps/ctrl/run.go:224-233
Timestamp: 2025-09-25T18:49:28.948Z
Learning: In go/apps/ctrl/run.go, mcstepp prefers to keep ACME service handlers wrapped with auth interceptor during the demo phase for simplicity, even though typically ACME routes would be exempt from authentication. This is part of the temporary demo authentication system.
Applied to files:
svc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/register.go
📚 Learning: 2026-01-09T12:56:40.667Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4575
File: svc/preflight/config.go:12-19
Timestamp: 2026-01-09T12:56:40.667Z
Learning: In the unkeyed/unkey repository, when cmd/preflight/main.go uses cmd.RequireString() for TLSCertFile and TLSKeyFile flags, it panics if these values are not provided, ensuring validation at the CLI parsing stage before config.Validate() is called. Therefore, additional validation in svc/preflight/config.go Validate() method for these fields is unnecessary.
Applied to files:
svc/ctrl/services/build/backend/depot/generate_upload_url.go
📚 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:
svc/ctrl/services/build/backend/depot/generate_upload_url.go
📚 Learning: 2025-09-15T18:12:01.503Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: go/apps/ctrl/run.go:212-213
Timestamp: 2025-09-15T18:12:01.503Z
Learning: In Go connect-go generated handlers, mux.Handle can accept New*ServiceHandler functions directly even though they return (string, http.Handler) tuples. The pattern mux.Handle(ctrlv1connect.New*ServiceHandler(...)) is valid and compiles successfully in unkey codebase.
Applied to files:
svc/api/internal/ctrlclient/errors.gosvc/api/routes/v2_deploy_generate_upload_url/handler.gosvc/api/routes/v2_deploy_create_deployment/handler.gosvc/api/routes/register.go
📚 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:
svc/api/routes/v2_deploy_create_deployment/handler.go
📚 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:
svc/api/routes/register.go
📚 Learning: 2025-09-01T15:10:44.959Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3899
File: go/proto/metald/v1/deployment.proto:7-15
Timestamp: 2025-09-01T15:10:44.959Z
Learning: In the unkey/unkey repository, the metald service receives deployment_id values from upstream services rather than generating them internally. The deployment_id field in DeploymentRequest is part of a service-to-service communication pattern.
Applied to files:
svc/api/routes/register.go
🧬 Code graph analysis (9)
svc/api/routes/v2_deploy_generate_upload_url/404_test.go (5)
pkg/testutil/http.go (1)
CallRoute(413-447)pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/routes/v2_deploy_generate_upload_url/handler.go (2)
Handler(25-30)Request(21-21)svc/api/routes/register.go (1)
Register(70-666)svc/api/openapi/gen.go (1)
InternalServerErrorResponse(184-190)
svc/api/routes/v2_deploy_generate_upload_url/401_test.go (5)
pkg/testutil/http.go (1)
CallRoute(413-447)pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/routes/v2_deploy_generate_upload_url/handler.go (3)
Handler(25-30)Request(21-21)Response(22-22)svc/api/routes/register.go (1)
Register(70-666)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)
svc/api/internal/ctrlclient/errors.go (2)
pkg/codes/constants_gen.go (1)
URN(5-5)pkg/codes/unkey_data.go (1)
Data(126-187)
svc/api/routes/v2_deploy_generate_upload_url/200_test.go (4)
pkg/testutil/http.go (1)
CallRoute(413-447)pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/routes/v2_deploy_generate_upload_url/handler.go (3)
Handler(25-30)Request(21-21)Response(22-22)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)
svc/api/run.go (1)
pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)
svc/api/routes/v2_deploy_generate_upload_url/handler.go (3)
svc/api/openapi/gen.go (4)
V2DeployGenerateUploadUrlRequestBody(747-750)V2DeployGenerateUploadUrlResponseBody(753-758)Meta(282-285)V2DeployGenerateUploadUrlResponseData(761-770)svc/api/routes/v2_deploy_create_deployment/handler.go (3)
Handler(25-30)Request(21-21)Response(22-22)svc/api/internal/ctrlclient/errors.go (1)
HandleError(17-52)
svc/api/routes/v2_deploy_create_deployment/handler.go (1)
svc/api/internal/ctrlclient/errors.go (1)
HandleError(17-52)
svc/api/routes/register.go (2)
svc/api/routes/v2_deploy_generate_upload_url/handler.go (1)
Handler(25-30)svc/api/routes/v2_deploy_create_deployment/handler.go (1)
Handler(25-30)
svc/api/routes/v2_deploy_generate_upload_url/400_test.go (5)
pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/routes/v2_deploy_generate_upload_url/handler.go (3)
Handler(25-30)Request(21-21)Response(22-22)svc/api/routes/register.go (1)
Register(70-666)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)svc/api/openapi/gen.go (2)
BadRequestErrorResponse(68-74)Meta(282-285)
🪛 GitHub Actions: PR
svc/api/internal/ctrlclient/BUILD.bazel
[error] 1-1: Command failed: bazel test //... --test_output=errors
svc/api/routes/v2_deploy_create_deployment/BUILD.bazel
[error] 1-1: Command failed: bazel test //... --test_output=errors
svc/api/routes/v2_deploy_generate_upload_url/BUILD.bazel
[error] 1-1: Command failed: bazel test //... --test_output=errors
svc/api/routes/BUILD.bazel
[error] 1-1: Command failed: bazel test //... --test_output=errors
⏰ 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). (3)
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Bazel / Build
- GitHub Check: Bazel / Test
🔇 Additional comments (27)
svc/ctrl/services/build/backend/depot/generate_upload_url.go (1)
24-33: Good addition: Project existence validation prevents URL generation for arbitrary project IDs.The security rationale is well-documented in the comment, and the error handling correctly distinguishes between not-found and internal errors.
svc/api/routes/v2_deploy_create_deployment/404_test.go (1)
68-68: LGTM on the error message update.The updated error detail aligns with the centralized error handling in
ctrlclient.HandleError.svc/ctrl/services/build/backend/docker/generate_upload_url.go (1)
24-33: Good security practice: project existence validation before URL generation.The pre-check prevents unauthorized generation of upload URLs for arbitrary project IDs. Error handling correctly distinguishes between not-found and internal errors.
svc/api/routes/services.go (1)
29-30: LGTM!The new
CtrlBuildClientfield follows the established naming convention and is appropriately placed alongside the existingCtrlDeploymentClient.svc/api/routes/v2_deploy_create_deployment/BUILD.bazel (2)
17-17: LGTM on the new ctrlclient dependency.The dependency addition enables the handler to use the centralized
HandleErrorutility for Connect RPC error translation.
23-43: No pipeline issues were identified in the test configuration or implementation. The BUILD.bazel dependencies are correct, test assertions match the intended error handling behavior, and the handler code properly utilizes all declared dependencies. If a pipeline failure is occurring, provide the specific error output for targeted investigation.svc/api/internal/ctrlclient/BUILD.bazel (1)
1-13: BUILD file structure is correct.The library definition with appropriate visibility scoping and dependencies is well-structured. The
errors.gosource file exists, and both consuming targets (v2_deploy_create_deploymentandv2_deploy_generate_upload_url) properly declare the//svc/api/internal/ctrlclientdependency. No missing dependencies were found in dependent targets.Likely an incorrect or invalid review comment.
svc/api/internal/ctrlclient/errors.go (1)
1-52: LGTM overall - clean centralized error handling.The
HandleErrorfunction provides a well-structured approach for converting Connect RPC errors to user-facing fault errors. The use of//nolint:exhaustivewith a sensible default case is appropriate.svc/api/routes/v2_deploy_create_deployment/handler.go (1)
16-16: LGTM - Clean refactor to use centralized error handling.The switch from inline error handling to
ctrlclient.HandleError(err, "create deployment")is a good improvement that reduces code duplication and maintains consistency with other handlers.Also applies to: 120-122
svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlRequestBody.yaml (1)
1-9: LGTM - Well-defined request schema.The schema correctly specifies
projectIdas required withminLength: 1validation, preventing empty strings. The description and example are clear.svc/api/routes/BUILD.bazel (1)
38-38: LGTM - Dependency correctly added.The new route dependency follows the established pattern and is correctly placed alongside the other deploy-related routes.
svc/api/openapi/gen.go (2)
746-770: Generated types look correct.The generated request/response types for the
generateUploadUrlendpoint are properly structured:
- Request body requires
projectId- Response data includes
context(S3 path),expiresIn(seconds as int64), anduploadUrl(presigned URL)
2367-2368: LGTM - Type alias correctly generated.svc/api/routes/v2_deploy_generate_upload_url/BUILD.bazel (1)
1-43: Build configuration looks correct, but pipeline failure needs investigation.The Bazel targets are well-structured with appropriate dependencies. However, the pipeline shows
bazel test //...failed. Since the handler.go and test files (200_test.go, 400_test.go, etc.) are not included in this review, the root cause cannot be determined from the provided context.#!/bin/bash # Check if all referenced source files exist and examine any compilation/test errors echo "=== Checking source files ===" fd -t f "handler.go|200_test.go|400_test.go|401_test.go|404_test.go" svc/api/routes/v2_deploy_generate_upload_url echo "=== Checking handler.go content ===" cat svc/api/routes/v2_deploy_generate_upload_url/handler.go 2>/dev/null || echo "handler.go not found" echo "=== Checking for any BUILD issues in deps ===" rg -l "v2_deploy_generate_upload_url" --type bazelsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/index.yaml (1)
1-49: LGTM!The OpenAPI specification is well-structured and follows the existing patterns for other v2 endpoints. Security, request/response schemas, and error responses are properly defined.
svc/api/openapi/openapi-split.yaml (1)
140-141: LGTM!The new endpoint reference is correctly placed under the Deploy Endpoints section and follows the established naming and reference patterns.
svc/api/routes/v2_deploy_generate_upload_url/200_test.go (1)
19-76: LGTM!The test is well-structured with comprehensive assertions covering the success path. It properly validates all key response fields (
UploadUrl,Context,ExpiresIn) and follows the established test patterns in the codebase.svc/api/routes/register.go (2)
30-30: LGTM!The import alias follows the established naming convention used by other route packages in this file.
341-352: LGTM!The route registration follows the established pattern for CTRL-dependent endpoints, with proper nil-check guarding and correct handler field wiring matching the
Handlerstruct definition.svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml (1)
1-19: LGTM!The OpenAPI schema correctly defines the response data structure with appropriate types, formats, and clear descriptions. The
int64format forexpiresInis suitable for representing seconds, and the examples are realistic.svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseBody.yaml (1)
1-9: LGTM!The response body schema follows the standard API response structure with
metaanddataproperties, and correctly references the shared Meta schema and the specific response data schema.svc/api/routes/v2_deploy_generate_upload_url/400_test.go (1)
20-58: LGTM!The test setup correctly initializes the harness, CTRL client with authorization, and seeds the necessary test data. The pattern is consistent with other route tests in the codebase.
svc/api/run.go (2)
302-321: LGTM!The
BuildServiceClientinitialization correctly mirrors the existingDeploymentServiceClientpattern. While there's some duplication in the interceptor setup, this keeps each client self-contained and explicit, which aligns with the codebase patterns. The log messages appropriately reflect that both deployment and build endpoints depend on the CTRL URL configuration.
335-335: LGTM!The
CtrlBuildClientis correctly wired to the routes services, enabling the conditional registration of thegenerateUploadUrlendpoint.svc/api/routes/v2_deploy_generate_upload_url/handler.go (3)
40-45: Root key validation without usage is intentional per PR description.The root key is obtained but discarded since permission checks will be added later. Consider adding a brief TODO comment to clarify the intent for future maintainers.
// TODO: Add deployment-related permission checks using the root key _, emit, err := h.Keys.GetRootKey(ctx, s)
47-68: LGTM!Request binding, input validation, and error handling follow established patterns. The use of
ctrlclient.HandleErrorproperly maps Connect RPC errors to fault errors for consistent API responses.
70-79: LGTM!Response structure correctly maps the control plane response fields (
UploadUrl,BuildContextPath→Context,ExpiresIn) to the OpenAPI-defined response format with proper metadata.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @svc/api/routes/v2_deploy_create_deployment/handler.go:
- Around line 111-113: The route change to use ctrlclient.HandleError is fine
but you must ensure ProjectId validation semantics remain correct: update the
CTRL service CreateDeployment
(svc/ctrl/services/deployment/create_deployment.go, function CreateDeployment)
to explicitly check for an empty ProjectId before performing the DB lookup and,
if empty, return a connect error with CodeInvalidArgument (with a clear message)
instead of allowing the DB lookup to produce CodeNotFound; this preserves the
original invalid-input vs missing-resource semantics so the handler's call to
ctrlclient.HandleError maps to the intended error code/message.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
svc/api/routes/v2_deploy_create_deployment/BUILD.bazelsvc/api/routes/v2_deploy_create_deployment/handler.gosvc/api/routes/v2_deploy_generate_upload_url/BUILD.bazelsvc/api/routes/v2_deploy_generate_upload_url/handler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- svc/api/routes/v2_deploy_generate_upload_url/BUILD.bazel
- svc/api/routes/v2_deploy_generate_upload_url/handler.go
🧰 Additional context used
🧠 Learnings (2)
📓 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-15T18:12:01.503Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: go/apps/ctrl/run.go:212-213
Timestamp: 2025-09-15T18:12:01.503Z
Learning: In Go connect-go generated handlers, mux.Handle can accept New*ServiceHandler functions directly even though they return (string, http.Handler) tuples. The pattern mux.Handle(ctrlv1connect.New*ServiceHandler(...)) is valid and compiles successfully in unkey codebase.
Applied to files:
svc/api/routes/v2_deploy_create_deployment/handler.go
🧬 Code graph analysis (1)
svc/api/routes/v2_deploy_create_deployment/handler.go (1)
svc/api/internal/ctrlclient/errors.go (1)
HandleError(17-52)
⏰ 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: Lint Go Code / Lint
- GitHub Check: Bazel / Build
- GitHub Check: Bazel / Test
- GitHub Check: autofix
🔇 Additional comments (1)
svc/api/routes/v2_deploy_create_deployment/BUILD.bazel (1)
15-15: LGTM!The dependency update correctly adds
ctrlclientto support the centralized error handling, replacing the previously direct dependencies oncodesandfaultpackages.
...pi/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml
Outdated
Show resolved
Hide resolved
...pi/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @svc/api/openapi/openapi-generated.yaml:
- Around line 2634-2648: The schema V2DeployGenerateUploadUrlResponseData lists
expiresIn in its required array but the expiresIn property is missing; either
remove "expiresIn" from the required list on
V2DeployGenerateUploadUrlResponseData or re-add a matching expiresIn property
under properties (e.g., type: integer/string with description and example) in
the OpenAPI source, then regenerate the openapi YAML and any client/server code
so validation/codegen is consistent.
In
@svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml:
- Around line 2-5: The OpenAPI spec lists expiresIn in the required array but
its property was removed; update the V2DeployGenerateUploadUrlResponseData
schema by removing "expiresIn" from the required list (leaving "uploadUrl" and
"context") so the required array matches the defined properties, or
alternatively restore a proper expiresIn property if it was intended to
remain—refer to the required array and the properties block (uploadUrl, context)
and make them consistent.
🧹 Nitpick comments (2)
svc/api/openapi/openapi-generated.yaml (1)
4242-4291: Docs mismatch: endpoint claims “appropriate permissions” while checks are explicitly “later”.The endpoint description says it requires “appropriate permissions” (Line 4244-4248), but the PR objectives state deployment-related permission checks are not implemented yet. Consider explicitly documenting the current behavior (e.g., “requires a valid root key; permission checks TBD”) to avoid misleading integrators.
svc/api/routes/v2_deploy_generate_upload_url/handler.go (1)
23-28: Consider removing unused struct fields or adding TODO comments.The
DBandLoggerfields are declared but not used in theHandlemethod. If these are intentionally kept for future permission checks (as noted in the PR objectives), consider adding a brief comment. Otherwise, removing them reduces confusion.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
svc/api/openapi/gen.gosvc/api/openapi/openapi-generated.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yamlsvc/api/routes/v2_deploy_generate_upload_url/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/handler.go
🚧 Files skipped from review as they are similar to previous changes (1)
- svc/api/routes/v2_deploy_generate_upload_url/200_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: apps/dashboard/lib/trpc/routers/deploy/rollback.ts:23-24
Timestamp: 2025-09-12T18:11:33.481Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/rollback.ts, the CTRL_URL environment variable should fail fast with a clear error message if missing in non-development environments, rather than defaulting to localhost which can mask production configuration issues.
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-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 Learning: 2025-09-15T18:12:01.503Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: go/apps/ctrl/run.go:212-213
Timestamp: 2025-09-15T18:12:01.503Z
Learning: In Go connect-go generated handlers, mux.Handle can accept New*ServiceHandler functions directly even though they return (string, http.Handler) tuples. The pattern mux.Handle(ctrlv1connect.New*ServiceHandler(...)) is valid and compiles successfully in unkey codebase.
Applied to files:
svc/api/routes/v2_deploy_generate_upload_url/handler.go
🧬 Code graph analysis (1)
svc/api/routes/v2_deploy_generate_upload_url/handler.go (2)
web/apps/dashboard/gen/proto/ctrl/v1/build_pb.ts (1)
GenerateUploadURLRequest(94-101)svc/api/internal/ctrlclient/errors.go (1)
HandleError(17-52)
⏰ 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: Lint Go Code / Lint
- GitHub Check: Bazel / Build
- GitHub Check: Bazel / Test
- GitHub Check: autofix
🔇 Additional comments (5)
svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml (1)
7-14: Property definitions look good.The
uploadUrlandcontextfields are properly typed with clear descriptions and appropriate examples.svc/api/openapi/gen.go (2)
746-767: Generated types look correct.These types are auto-generated from the OpenAPI spec. The
V2DeployGenerateUploadUrlResponseDatastruct correctly reflects only the defined properties (ContextandUploadUrl). The mismatch with therequiredarray in the YAML spec should be fixed there (as noted in the spec file review).
2364-2365: Type alias correctly defined.svc/api/openapi/openapi-generated.yaml (1)
4248-4254: No action needed—operationIdis unique.The
generateUploadUrloperationId appears only once in the specification with no duplicate operationIds present in the file, eliminating the risk of SDK naming collisions.Likely an incorrect or invalid review comment.
svc/api/routes/v2_deploy_generate_upload_url/handler.go (1)
38-69: Implementation looks good; root key discarding is intentional.The handler correctly:
- Validates authentication via
GetRootKey(permission checks deferred per PR objectives)- Binds and validates the request body
- Uses centralized error handling via
ctrlclient.HandleError- Maps the response fields appropriately
One minor observation: the root key value (first return) is discarded on line 39. Per PR objectives, this is intentional since permission checks will be added later. When those are implemented, you'll likely need to use the returned root key data.
...pi/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
svc/api/openapi/openapi-generated.yaml (1)
432-452: Tighten schema validation: addadditionalProperties: falseand considerformat: uriforuploadUrl.
Many existing request/response bodies in this spec are strict; these new ones being permissive can allow client typos to slip through unnoticed, andformat: uriimproves generated client types/validation.Proposed spec tweak (apply in source openapi-split.yaml)
V2DeployGenerateUploadUrlRequestBody: type: object + additionalProperties: false required: - projectId properties: projectId: type: string minLength: 1 V2DeployGenerateUploadUrlResponseBody: type: object + additionalProperties: false required: - meta - data properties: meta: $ref: "#/components/schemas/Meta" data: $ref: "#/components/schemas/V2DeployGenerateUploadUrlResponseData" V2DeployGenerateUploadUrlResponseData: type: object + additionalProperties: false required: - uploadUrl - context properties: uploadUrl: type: string + format: uri description: Presigned PUT URL for uploading the build context tar fileAlso applies to: 2634-2647
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
svc/api/openapi/openapi-generated.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlResponseData.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: apps/dashboard/lib/trpc/routers/deploy/rollback.ts:23-24
Timestamp: 2025-09-12T18:11:33.481Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/rollback.ts, the CTRL_URL environment variable should fail fast with a clear error message if missing in non-development environments, rather than defaulting to localhost which can mask production configuration issues.
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-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
svc/api/openapi/openapi-generated.yaml
⏰ 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). (3)
- GitHub Check: autofix
- GitHub Check: Bazel / Test
- GitHub Check: Bazel / Build
🔇 Additional comments (1)
svc/api/openapi/openapi-generated.yaml (1)
2626-2647: VerifycreateDeploymentresponse contract: does it also return upload-url data or not?
In this file,V2DeployCreateDeploymentResponseDatashows onlydeploymentId. If the implementation now optionally includesV2DeployGenerateUploadUrlResponseData(as described in the PR summary/AI summary), the OpenAPI contract should expose that field to avoid breaking generated clients.
svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/index.yaml
Outdated
Show resolved
Hide resolved
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
left some comments otherwise lgtm
3 participants
What does this PR do?
This PR creates an endpoint for
generateUploadUrlprocess to decouple CLI and CTRL. That way CLI can talk to API instead of CTRL directly.Note:
/v2/deploy.generateUploadUrldoesn't do any permission check for deployment related stuff. Will be added soon.HandleErrorinsvc/api/internal/ctrlclient/errors.goto convert Connect RPC errors to fault errors. This will also be used ingetDeploymentendpoint. We had this block increateDeployment, its using this helper instead to prevent duplication.projectIdcheck toctrl/services/deployment/create_deployment.go. And DB look up to verify project exists before we actually generate the upload URLctrl/services/build/backend/[docker|depot]/generate_upload_url.goType of change
How should this be tested?
http://localhost:3000/[workspace]/settings/root-keysrootKeyhttp://localhost:3000/[workspace]/projectsproject_idAuthorization: Bearer xxxExpected: 200 OK with
context,expiresInanduploadUrlto call in the CLIChecklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated