feat: add getDeployment endpoint#4604
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds an internal POST endpoint /v2/deploy.getDeployment with OpenAPI schemas and generated Go models, a new route handler wired into registration and build targets, tests for success and error cases, and test-harness enhancements to provide controller clients and deployment setup helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant API as v2 deploy.getDeployment Handler
participant Keys as Key Service
participant Ctrl as Controller DeploymentService
participant DB as Database
Client->>API: POST /v2/deploy.getDeployment { deploymentId } (Authorization)
API->>Keys: Resolve root key from session
Keys-->>API: rootKey / error
API->>Ctrl: GetDeployment(deploymentId, rootKey)
Ctrl-->>API: Deployment data (id, status, hostnames, steps) / error
API->>DB: (optional) record/request metadata
DB-->>API: ack
API-->>Client: 200 { data: { id, status, hostnames, steps }, meta } / error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
svc/api/openapi/openapi-generated.yaml (1)
4271-4429: Doc/contract mismatch: endpoints claim permission enforcement that (per PR) doesn’t exist yetLine 4271-4281, 4325-4332, 4379-4385: the descriptions say “Requires a valid root key with appropriate permissions”, but the PR objectives explicitly say there are currently no permission checks for deployment-related operations (to be added later). This is likely to confuse internal consumers and is a spec-contract mismatch.
Suggested adjustment: state that authentication is required (root key), and explicitly note that authorization/permission checks are not enforced yet (or remove the “appropriate permissions” phrasing until 403 is real).
Proposed diff (apply similarly across deploy internal endpoints)
description: | **INTERNAL** - This endpoint is internal and may change without notice. Not recommended for production use. Retrieves deployment information including status, error messages, and steps. - **Authentication**: Requires a valid root key with appropriate permissions. + **Authentication**: Requires a valid root key. + **Authorization**: Deployment permission checks are not enforced yet (will be added later).
🤖 Fix all issues with AI agents
In
`@svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yaml`:
- Around line 8-9: The example deployment ID in
V2DeployGetDeploymentRequestBody.yaml is inconsistent with real IDs; update the
example value (the example field currently set to "dep_abc123xyz") to use the
actual deployment ID format that starts with "d_" (for example
"d_5TV5hCUQ3atBZNz3") so the example matches real-world IDs and testing
instructions.
In `@svc/api/routes/v2_deploy_get_deployment/200_test.go`:
- Around line 90-113: The helper createTestDeployment currently overrides the
Authorization header with rootKey which is inconsistent with the other test that
relies on the interceptor's ctrlToken; decide which token should be used and
make tests consistent: either (A) remove the explicit Header().Set call in
createTestDeployment so it uses the interceptor-provided ctrlToken (and update
callers if they need a different token), or (B) make the helper accept an auth
token parameter (e.g., token string) and set Header().Set("Authorization",
fmt.Sprintf("Bearer %s", token)) so callers can explicitly pass rootKey or
ctrlToken; update the v2_deploy_create_deployment test to match the chosen
pattern and add a short comment in createTestDeployment explaining why that
token is required.
In `@svc/api/routes/v2_deploy_get_deployment/404_test.go`:
- Around line 53-68: Test filename and expectations mismatch: rename the test
file svc/api/routes/v2_deploy_get_deployment/404_test.go to 500_test.go (and
update the t.Run name or comment to "deployment not found - expects 500") so the
filename reflects the actual assertion (require.Equal expecting
http.StatusInternalServerError) for handler.Request / handler.Response in this
route test; also open a tracker issue to update the CTRL service to return 404
for missing deployments and, when that is fixed, change the assertion back to
http.StatusNotFound and rename the test accordingly.
🧹 Nitpick comments (6)
svc/api/routes/v2_deploy_get_deployment/401_test.go (1)
3-15: Consider organizing imports into separate groups.Per coding guidelines, Go imports should be organized with blank lines separating: (1) Standard library, (2) External/third-party packages, (3) Internal packages.
🔧 Suggested import organization
import ( "fmt" "net/http" "testing" "connectrpc.com/connect" "github.com/stretchr/testify/require" + "github.com/unkeyed/unkey/gen/proto/ctrl/v1/ctrlv1connect" "github.com/unkeyed/unkey/pkg/rpc/interceptor" "github.com/unkeyed/unkey/pkg/testutil" "github.com/unkeyed/unkey/pkg/testutil/containers" handler "github.com/unkeyed/unkey/svc/api/routes/v2_deploy_get_deployment" )svc/api/routes/v2_deploy_get_deployment/404_test.go (1)
3-17: Consider organizing imports into separate groups.Per coding guidelines, add a blank line between external/third-party packages and internal packages.
🔧 Suggested import organization
import ( "fmt" "net/http" "testing" "connectrpc.com/connect" "github.com/stretchr/testify/require" + "github.com/unkeyed/unkey/gen/proto/ctrl/v1/ctrlv1connect" "github.com/unkeyed/unkey/pkg/rpc/interceptor" "github.com/unkeyed/unkey/pkg/testutil" "github.com/unkeyed/unkey/pkg/testutil/containers" "github.com/unkeyed/unkey/pkg/testutil/seed" "github.com/unkeyed/unkey/pkg/uid" handler "github.com/unkeyed/unkey/svc/api/routes/v2_deploy_get_deployment" )svc/api/routes/v2_deploy_get_deployment/400_test.go (1)
45-52: Unused project creation in test setup.The project is created but appears unused after setup. The
workspaceandrootKeyare used butprojectIDand the created project are not referenced in any of the test cases. Consider removing if not needed, or verify if this is intentional setup for the test harness.🧹 Suggested cleanup
workspace := h.CreateWorkspace() rootKey := h.CreateRootKey(workspace.ID) - - projectID := uid.New(uid.ProjectPrefix) - - h.CreateProject(seed.CreateProjectRequest{ - WorkspaceID: workspace.ID, - Name: "test-project", - ID: projectID, - Slug: "production", - })svc/api/routes/v2_deploy_get_deployment/handler.go (1)
66-96: Consider simplifying step transformation.The explicit nil initialization at lines 71-74 is redundant since Go zero-values struct fields to nil for pointer types. The conditional assignments that follow are correct.
🧹 Simplified step initialization
for i, protoStep := range deployment.GetSteps() { - step := openapi.V2DeployDeploymentStep{ - ErrorMessage: nil, - CreatedAt: nil, - Message: nil, - Status: nil, - } + step := openapi.V2DeployDeploymentStep{} if protoStep.GetStatus() != "" {svc/api/openapi/openapi-generated.yaml (2)
452-472: Tighten request/response envelopes (additionalProperties: false) and align ID examplesLine 452-462:
V2DeployGetDeploymentRequestBodyshould likely setadditionalProperties: false(most request bodies do), to avoid silently accepting/ignoring typos in internal clients. Also, the example usesdep_...but the PR test example usesd_...; please align examples to the canonical ID format used by the system.Line 463-471: consider
additionalProperties: falseonV2DeployGetDeploymentResponseBodyas well for consistency.Proposed diff
V2DeployGetDeploymentRequestBody: type: object required: - deploymentId properties: deploymentId: type: string minLength: 1 description: Unique deployment identifier to retrieve - example: "dep_abc123xyz" + example: "d_5TV5hCUQ3atBZNz3" + additionalProperties: false V2DeployGetDeploymentResponseBody: type: object required: - meta - data properties: meta: $ref: "#/components/schemas/Meta" data: $ref: "#/components/schemas/V2DeployGetDeploymentResponseData" + additionalProperties: false
2668-2724: ClarifyidvsdeploymentId, and lock downV2DeployDeploymentStepshapeLine 2668-2705:
V2DeployGetDeploymentResponseDatareturnsid, while other deploy responses usedeploymentId(e.g., create). Either is fine, but mixing them increases client foot-guns; at minimum, make the description explicit thatidis the deployment id (it is currently, but I’d consider renaming for consistency if feasible).Line 2706-2724:
V2DeployDeploymentStep.statusis a free-form string with a lower-case example (“completed”), while the top-levelstatusis a strict enum with uppercase values. If step statuses are also finite, prefer an enum (or document allowed values). Also recommendadditionalProperties: false(and possiblyrequired) so internal clients can rely on stable shapes.Proposed diff
V2DeployGetDeploymentResponseData: type: object required: - id - status properties: id: type: string description: Unique deployment identifier - example: "dep_abc123xyz" + example: "d_5TV5hCUQ3atBZNz3" status: type: string description: Current deployment status enum: - UNSPECIFIED - PENDING - BUILDING - DEPLOYING - NETWORK - READY - FAILED example: "READY" @@ steps: type: array items: $ref: "#/components/schemas/V2DeployDeploymentStep" description: Deployment steps with status and messages + additionalProperties: false V2DeployDeploymentStep: type: object + additionalProperties: false properties: status: type: string description: Step status example: "completed" message: type: string description: Step message example: "Image pulled successfully" errorMessage: type: string description: Error message if step failed example: "Connection timeout" createdAt: type: integer format: int64 description: Unix timestamp in milliseconds example: 1704067200000
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
svc/api/openapi/gen.gosvc/api/openapi/openapi-generated.yamlsvc/api/openapi/openapi-split.yamlsvc/api/openapi/spec/paths/v2/deploy/createDeployment/index.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/index.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployDeploymentStep.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseBody.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseData.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/index.yamlsvc/api/routes/BUILD.bazelsvc/api/routes/register.gosvc/api/routes/v2_deploy_get_deployment/200_test.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/BUILD.bazelsvc/api/routes/v2_deploy_get_deployment/handler.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Organize Go imports in groups separated by blank lines: (1) Standard library, (2) External/third-party packages, (3) Internal packages (github.com/unkeyed/unkey/internal/...), (4) Package-level (github.com/unkeyed/unkey/pkg/...), (5) Service-level (github.com/unkeyed/unkey/svc/...), (6) Generated code (github.com/unkeyed/unkey/gen/...)
Use thefaultpackage for structured error handling withfault.Wrap(),fault.Code(),fault.Internal(), andfault.Public()methods
Usesnake_case.gofor Go filenames and*_test.gofor test files
UsePascalCasefor exported Go functions and types,camelCasefor unexported functions and types
Use short receiver names like(s *Service)or(h *Handler)in Go methods
UseSCREAMING_SNAKE_CASEfor Go constants
Runmake bazelafter adding new Go files to sync BUILD.bazel files
Runmake fmtbefore committing Go changes to format and lint code
Files:
svc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/200_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/register.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_get_deployment/handler.gosvc/api/openapi/gen.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Usetestify/requirefor assertions in Go tests instead of manual assertions
Use nested test scenarios witht.Run()in Go tests for better organization and clarity
Files:
svc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/200_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/400_test.go
svc/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the
pkg/zencustom HTTP framework for Go HTTP handlers with the pattern of binding request bodies, handling business logic, and returning JSON responses
Files:
svc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/200_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/register.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_get_deployment/handler.gosvc/api/openapi/gen.go
🧠 Learnings (7)
📓 Common learnings
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.
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: 2026-01-13T17:46:51.335Z
Learnt from: CR
Repo: unkeyed/unkey PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T17:46:51.335Z
Learning: Applies to **/*.go : Run `make bazel` after adding new Go files to sync BUILD.bazel files
Applied to files:
svc/api/routes/BUILD.bazelsvc/api/routes/v2_deploy_get_deployment/BUILD.bazel
📚 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_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/200_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/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_get_deployment/401_test.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/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yamlsvc/api/routes/register.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/routes/v2_deploy_get_deployment/handler.go
📚 Learning: 2026-01-13T17:46:51.335Z
Learnt from: CR
Repo: unkeyed/unkey PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T17:46:51.335Z
Learning: Applies to svc/**/*.go : Use the `pkg/zen` custom HTTP framework for Go HTTP handlers with the pattern of binding request bodies, handling business logic, and returning JSON responses
Applied to files:
svc/api/routes/v2_deploy_get_deployment/handler.go
🧬 Code graph analysis (4)
svc/api/routes/v2_deploy_get_deployment/401_test.go (3)
pkg/testutil/http.go (1)
CallRoute(413-447)pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/routes/v2_deploy_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)
svc/api/routes/v2_deploy_get_deployment/200_test.go (5)
pkg/testutil/http.go (2)
NewHarness(60-209)CallRoute(413-447)pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/routes/v2_deploy_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)svc/api/openapi/gen.go (2)
Meta(293-296)Set(45-45)
svc/api/routes/v2_deploy_get_deployment/404_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_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)
svc/api/routes/v2_deploy_get_deployment/400_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_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)svc/api/openapi/gen.go (2)
BadRequestErrorResponse(79-85)Meta(293-296)
⏰ 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 (25)
svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/index.yaml (1)
6-14: LGTM!The internal designation is appropriate. The
x-speakeasy-group: internalextension and the description notice clearly communicate that this endpoint is not intended for external consumption and may change without notice.svc/api/openapi/spec/paths/v2/deploy/createDeployment/index.yaml (1)
6-14: LGTM!Consistent internal designation pattern with the other deploy endpoints. The changes appropriately mark this endpoint as internal.
svc/api/routes/BUILD.bazel (1)
39-39: LGTM!The new route dependency is correctly added and maintains alphabetical ordering with the existing dependencies.
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseBody.yaml (1)
1-9: LGTM!The response body schema follows the standard pattern with
metaanddatafields. The$refpaths are correctly structured.svc/api/routes/register.go (1)
31-31: LGTM!The import and route registration follow the established patterns for v2/deploy endpoints. The handler correctly uses
CtrlDeploymentClientconsistent withv2DeployCreateDeployment.Also applies to: 341-350
svc/api/routes/v2_deploy_get_deployment/401_test.go (1)
39-53: LGTM!The test correctly validates that requests with invalid authorization tokens receive a 401 Unauthorized response. Good use of
t.Run()for subtest organization andrequirefor assertions as per coding guidelines.svc/api/openapi/openapi-split.yaml (1)
140-141: LGTM!The new endpoint path is correctly added following the established pattern and alphabetical ordering within the Deploy Endpoints section.
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployDeploymentStep.yaml (1)
1-19: Consider specifying required fields for the deployment step schema.The schema defines all properties as optional. For a deployment step,
statusandcreatedAtwould typically be essential fields that should always be present. Consider adding arequiredarray if this is the intended contract.type: object required: - status - createdAt properties: # ... existing propertiessvc/api/routes/v2_deploy_get_deployment/BUILD.bazel (1)
1-42: LGTM!The Bazel build configuration is well-structured with appropriate dependencies for both the library and test targets. The test target correctly includes all four test files (200, 400, 401, 404) and necessary test dependencies including
testify/require.svc/api/routes/v2_deploy_get_deployment/400_test.go (3)
1-18: LGTM!Import organization follows the coding guidelines correctly: standard library first, then external packages, then internal packages.
59-82: LGTM!Well-structured subtests for deployment ID validation. Good use of
t.Run()for organization andtestify/requirefor assertions. The tests properly verify both the HTTP status code and the error response structure.
84-112: LGTM!Authorization header validation tests are correctly structured. Testing both missing and malformed authorization headers with appropriate 400 status expectations.
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseData.yaml (1)
1-36: LGTM!Well-structured OpenAPI schema with appropriate required fields (
id,status), a comprehensive status enum matching the handler implementation, and properly typed optional fields for error messages, hostnames, and deployment steps. Examples are helpful for API consumers.svc/api/routes/v2_deploy_get_deployment/handler.go (4)
1-16: LGTM!Import organization follows the coding guidelines correctly: standard library, external packages, generated code, internal services, pkg utilities, and service-level packages.
98-125: LGTM!Response construction is well-structured with proper nil checks for optional fields. The JSON response follows the zen framework pattern correctly.
128-147: LGTM!The status enum mapping is comprehensive and includes a sensible default fallback to "UNSPECIFIED" for unknown status values. The enum values match the OpenAPI spec.
38-48: No action needed. The pattern of deferringemit()before error checking is intentional and safe—GetRootKeyalways returns a safe no-op emit function even when the main operation fails, allowing it to be deferred immediately. This pattern is consistently used across all handlers in the codebase.svc/api/routes/v2_deploy_get_deployment/200_test.go (3)
1-19: LGTM!Import organization follows the coding guidelines correctly: standard library, external packages, generated proto code, and internal packages properly grouped.
21-41: LGTM!Test setup correctly initializes the harness, CTRL client with authorization interceptor, and registers the handler route.
43-87: LGTM!Well-structured integration test that exercises the full flow: creating workspace, project, environment, deployment, and then verifying the getDeployment endpoint returns the expected data. Good use of
t.Run()for organization and comprehensive assertions.svc/api/openapi/spec/paths/v2/deploy/getDeployment/index.yaml (1)
1-53: LGTM - OpenAPI spec is well-structured.The endpoint specification follows the existing patterns for deploy endpoints. The internal designation and warning are appropriate given that permission checks are noted for future implementation.
Consider adding a
403 Forbiddenresponse when permission checks are implemented, similar to other secured endpoints.svc/api/openapi/gen.go (4)
757-770: LGTM - Generated deployment step type.The
V2DeployDeploymentStepstruct correctly models deployment steps with optional fields for timestamps, messages, and status.
795-828: LGTM - Generated request/response types are consistent.The generated types for the getDeployment endpoint follow the established patterns in this file. The response data structure appropriately captures deployment status, error information, hostnames, and execution steps.
2428-2429: LGTM - Request body alias follows established pattern.
30-39: No changes needed. The status constants are properly typed withV2DeployGetDeploymentResponseDataStatusand have no name collisions at package scope. This is correctly generated code from oapi-codegen/v2 v2.5.1.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
svc/api/openapi/openapi-generated.yaml (2)
4271-4321: Docs: “appropriate permissions” claim is misleading until permission checks land.The description says “Requires a valid root key with appropriate permissions” (Line 4276), but the PR objective explicitly notes deployment permission checks will be added later. Consider clarifying that auth is required and authorization is TBD (to avoid consumers inferring enforcement that doesn’t exist yet).
4325-4376: Docs: align wording with current enforcement (auth yes, authorization later).Same issue as createDeployment: description states “appropriate permissions” (Line 4330-4331) while authorization is explicitly deferred in this PR.
♻️ Duplicate comments (1)
svc/api/openapi/openapi-generated.yaml (1)
2668-2724: Fix inconsistent deployment ID examples (dep_vsd_).
V2DeployGetDeploymentResponseData.idexample is"dep_abc123xyz"(Line 2677) while the request example is"d_abc123xyz"(Line 461). This mismatch has already caused confusion in prior review threads.Proposed diff
V2DeployGetDeploymentResponseData: type: object required: - id - status properties: id: type: string description: Unique deployment identifier - example: "dep_abc123xyz" + example: "d_abc123xyz"
🧹 Nitpick comments (1)
svc/api/openapi/openapi-generated.yaml (1)
452-471: ConsideradditionalProperties: falsefor the new request/response envelope schemas.Many schemas in this spec are closed objects; these two aren’t, which weakens request validation / client typings.
Proposed diff
V2DeployGetDeploymentRequestBody: type: object + additionalProperties: false required: - deploymentId properties: deploymentId: type: string minLength: 1 description: Unique deployment identifier to retrieve example: "d_abc123xyz" V2DeployGetDeploymentResponseBody: type: object + additionalProperties: false required: - meta - data properties: meta: $ref: "#/components/schemas/Meta" data: $ref: "#/components/schemas/V2DeployGetDeploymentResponseData"
📜 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/getDeployment/V2DeployGetDeploymentRequestBody.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
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-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/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts:110-147
Timestamp: 2025-07-25T19:11:00.208Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts, the user mcstepp prefers to keep mock data fallbacks in POC/demonstration code for simplicity, even if it wouldn't be production-ready. This aligns with the PR being work-in-progress for demonstration purposes.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
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). (4)
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Bazel / Build
- GitHub Check: Bazel / Test
- GitHub Check: autofix
🔇 Additional comments (1)
svc/api/openapi/openapi-generated.yaml (1)
4376-4429: Clarify authorization requirements in the endpoint documentation.The status enum is correct and includes all expected values (BUILDING, DEPLOYING, NETWORK, READY, plus UNSPECIFIED, PENDING, FAILED). However, the description states "Requires a valid root key with appropriate permissions" while the implementation only validates root key presence via
GetRootKey()without checking specific permissions. Either add permission validation logic or update the description to reflect that only a valid root key is required.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yaml
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseData.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
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)
79-79: Typo in assertion comment.The comment is incomplete - missing the expected status code.
📝 Suggested fix
- 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 4380-4433: The OpenAPI description for operationId
deploy.getDeployment (path /v2/deploy.getDeployment) incorrectly claims
per-deployment permission enforcement; update the description field to read:
"Requires a valid root key. No per-deployment permission checks are currently
enforced." Alternatively, if you prefer to enforce permissions, modify the
deploy.getDeployment handler (the code path that calls GetRootKey()) to perform
an explicit authorization check against the deployment's workspace/project
before returning the deployment (validate root key scope/ownership) and then
update the OpenAPI to reflect the enforced behavior.
♻️ Duplicate comments (1)
svc/api/routes/v2_deploy_get_deployment/404_test.go (1)
38-53: File name vs. expected status code mismatch.As noted in the previous review, the file is named
404_test.gobut expects HTTP 500. Consider renaming to500_test.goor tracking the CTRL service fix to return proper 404 status.
🧹 Nitpick comments (5)
pkg/testutil/http.go (2)
192-206: Consider using an HTTP client with timeout for CTRL clients.Using
http.DefaultClientmeans no request timeout is configured. While this may be acceptable for tests, a hung CTRL service could cause tests to block indefinitely. Consider creating a client with a reasonable timeout.♻️ Suggested improvement
+ httpClient := &http.Client{ + Timeout: 30 * time.Second, + } + ctrlDeploymentClient := ctrlv1connect.NewDeploymentServiceClient( - http.DefaultClient, + httpClient, ctrlURL, connect.WithInterceptors(interceptor.NewHeaderInjector(map[string]string{ "Authorization": fmt.Sprintf("Bearer %s", ctrlToken), })), ) ctrlBuildClient := ctrlv1connect.NewBuildServiceClient( - http.DefaultClient, + httpClient, ctrlURL, connect.WithInterceptors(interceptor.NewHeaderInjector(map[string]string{ "Authorization": fmt.Sprintf("Bearer %s", ctrlToken), })), )
314-319: Consider a more descriptive default forProjectSlug.The default
ProjectSlug: "production"could be confusing since "production" typically refers to an environment, not a project. A default like"test-project"(matchingProjectName) would be clearer.♻️ Suggested improvement
config := CreateTestDeploymentSetupOptions{ ProjectName: "test-project", - ProjectSlug: "production", + ProjectSlug: "test-project", EnvironmentSlug: "production", SkipEnvironment: false, }svc/api/openapi/openapi-generated.yaml (2)
454-474: ConsideradditionalProperties: falseon new getDeployment request/response envelopes for consistency.Most request/response bodies in this spec are strict; these new ones currently allow extra keys by default.
Proposed change (apply in the source OpenAPI, then regenerate)
V2DeployGetDeploymentRequestBody: type: object + additionalProperties: false required: - deploymentId properties: deploymentId: type: string minLength: 1 pattern: "^d_[a-zA-Z0-9]+$" description: Unique deployment identifier to retrieve example: "d_abc123xyz" V2DeployGetDeploymentResponseBody: type: object + additionalProperties: false required: - meta - data properties: meta: $ref: "#/components/schemas/Meta" data: $ref: "#/components/schemas/V2DeployGetDeploymentResponseData"
2672-2728: Document/verifysteps[].statussemantics vs top-levelstatusenum.Top-level
statusis an uppercase enum, whileV2DeployDeploymentStep.statusis a free-form string with a lowercase example (“completed”). If this is intentional, it’d help to explicitly document expected values/casing to avoid clients building the wrong assumptions.Optional tightening (only if it matches real responses)
V2DeployDeploymentStep: type: object + additionalProperties: false + required: + - status + - createdAt properties: status: type: string description: Step status example: "completed" message: type: string description: Step message example: "Image pulled successfully" errorMessage: type: string description: Error message if step failed example: "Connection timeout" createdAt: type: integer format: int64 description: Unix timestamp in milliseconds example: 1704067200000svc/api/routes/v2_deploy_get_deployment/400_test.go (1)
30-37: Consider removing unused project setup.The
projectIDandh.CreateProject()are not used in any of the test requests sincegetDeploymentonly requiresdeploymentId. This setup can be removed to simplify the test.♻️ Suggested cleanup
workspace := h.CreateWorkspace() rootKey := h.CreateRootKey(workspace.ID) - - projectID := uid.New(uid.ProjectPrefix) - - h.CreateProject(seed.CreateProjectRequest{ - WorkspaceID: workspace.ID, - Name: "test-project", - ID: projectID, - Slug: "production", - })You can also remove the unused imports:
import ( "fmt" "net/http" "testing" "github.com/stretchr/testify/require" "github.com/unkeyed/unkey/pkg/testutil" - "github.com/unkeyed/unkey/pkg/testutil/seed" - "github.com/unkeyed/unkey/pkg/uid" "github.com/unkeyed/unkey/svc/api/openapi" handler "github.com/unkeyed/unkey/svc/api/routes/v2_deploy_get_deployment" )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
pkg/testutil/BUILD.bazelpkg/testutil/http.gosvc/api/openapi/openapi-generated.yamlsvc/api/openapi/spec/paths/v2/deploy/createDeployment/V2DeployCreateDeploymentRequestBody.yamlsvc/api/openapi/spec/paths/v2/deploy/createDeployment/V2DeployCreateDeploymentResponseData.yamlsvc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlRequestBody.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseData.yamlsvc/api/routes/v2_deploy_create_deployment/200_test.gosvc/api/routes/v2_deploy_create_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/BUILD.bazelsvc/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_get_deployment/200_test.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/BUILD.bazel
💤 Files with no reviewable changes (2)
- svc/api/routes/v2_deploy_generate_upload_url/BUILD.bazel
- svc/api/routes/v2_deploy_create_deployment/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (3)
- svc/api/routes/v2_deploy_get_deployment/BUILD.bazel
- svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentResponseData.yaml
- svc/api/routes/v2_deploy_get_deployment/200_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Organize Go imports in groups separated by blank lines: (1) Standard library, (2) External/third-party packages, (3) Internal packages (github.com/unkeyed/unkey/internal/...), (4) Package-level (github.com/unkeyed/unkey/pkg/...), (5) Service-level (github.com/unkeyed/unkey/svc/...), (6) Generated code (github.com/unkeyed/unkey/gen/...)
Use thefaultpackage for structured error handling withfault.Wrap(),fault.Code(),fault.Internal(), andfault.Public()methods
Usesnake_case.gofor Go filenames and*_test.gofor test files
UsePascalCasefor exported Go functions and types,camelCasefor unexported functions and types
Use short receiver names like(s *Service)or(h *Handler)in Go methods
UseSCREAMING_SNAKE_CASEfor Go constants
Runmake bazelafter adding new Go files to sync BUILD.bazel files
Runmake fmtbefore committing Go changes to format and lint code
Files:
svc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.gopkg/testutil/http.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.gosvc/api/routes/v2_deploy_generate_upload_url/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/404_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Usetestify/requirefor assertions in Go tests instead of manual assertions
Use nested test scenarios witht.Run()in Go tests for better organization and clarity
Files:
svc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.gosvc/api/routes/v2_deploy_generate_upload_url/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/404_test.go
svc/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the
pkg/zencustom HTTP framework for Go HTTP handlers with the pattern of binding request bodies, handling business logic, and returning JSON responses
Files:
svc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_get_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.gosvc/api/routes/v2_deploy_generate_upload_url/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/404_test.go
🧠 Learnings (16)
📓 Common learnings
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.
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_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_generate_upload_url/404_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/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.go
📚 Learning: 2026-01-13T17:46:51.335Z
Learnt from: CR
Repo: unkeyed/unkey PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T17:46:51.335Z
Learning: Applies to svc/**/*.go : Use the `pkg/zen` custom HTTP framework for Go HTTP handlers with the pattern of binding request bodies, handling business logic, and returning JSON responses
Applied to files:
svc/api/routes/v2_deploy_generate_upload_url/401_test.gosvc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_generate_upload_url/404_test.go
📚 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_get_deployment/400_test.gosvc/api/routes/v2_deploy_get_deployment/401_test.gosvc/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/200_test.gosvc/api/routes/v2_deploy_generate_upload_url/200_test.go
📚 Learning: 2026-01-13T17:46:51.335Z
Learnt from: CR
Repo: unkeyed/unkey PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T17:46:51.335Z
Learning: Applies to **/*_test.go : Use `testify/require` for assertions in Go tests instead of manual assertions
Applied to files:
svc/api/routes/v2_deploy_create_deployment/400_test.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.go
📚 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:
svc/api/routes/v2_deploy_create_deployment/400_test.gosvc/api/routes/v2_deploy_generate_upload_url/400_test.gopkg/testutil/BUILD.bazel
📚 Learning: 2025-09-25T15:12:21.739Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: go/cmd/deploy/control_plane.go:64-69
Timestamp: 2025-09-25T15:12:21.739Z
Learning: The API key authentication system in go/cmd/deploy/control_plane.go and related files is temporary demo code that will be replaced with a proper authentication system for private beta.
Applied to files:
svc/api/routes/v2_deploy_create_deployment/401_test.gosvc/api/routes/v2_deploy_create_deployment/200_test.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/openapi/spec/paths/v2/deploy/createDeployment/V2DeployCreateDeploymentResponseData.yamlsvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yamlsvc/api/openapi/openapi-generated.yaml
📚 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:
pkg/testutil/http.gosvc/api/routes/v2_deploy_generate_upload_url/404_test.go
📚 Learning: 2025-09-11T08:17:21.423Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/services/deployment/fallbacks/k8s.go:236-247
Timestamp: 2025-09-11T08:17:21.423Z
Learning: The K8s fallback backend in go/apps/ctrl/services/deployment/fallbacks/k8s.go is specifically designed for scenarios where the ctrl service runs inside a Kubernetes cluster. It should not be used when ctrl runs outside the cluster - in those cases, the Docker fallback should be used instead.
Applied to files:
pkg/testutil/http.gosvc/api/routes/v2_deploy_create_deployment/404_test.gosvc/api/routes/v2_deploy_create_deployment/200_test.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/api/routes/v2_deploy_get_deployment/404_test.gosvc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yamlsvc/api/openapi/openapi-generated.yaml
📚 Learning: 2025-09-12T18:11:33.481Z
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.
Applied to files:
svc/api/routes/v2_deploy_get_deployment/404_test.go
📚 Learning: 2025-01-30T20:51:44.359Z
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.
Applied to files:
svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yaml
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts:110-147
Timestamp: 2025-07-25T19:11:00.208Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts, the user mcstepp prefers to keep mock data fallbacks in POC/demonstration code for simplicity, even if it wouldn't be production-ready. This aligns with the PR being work-in-progress for demonstration purposes.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
svc/api/openapi/openapi-generated.yaml
🧬 Code graph analysis (5)
svc/api/routes/v2_deploy_get_deployment/400_test.go (4)
pkg/testutil/http.go (1)
CallRoute(512-546)svc/api/routes/v2_deploy_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)svc/api/openapi/gen.go (2)
BadRequestErrorResponse(79-85)Meta(293-296)
svc/api/routes/v2_deploy_get_deployment/401_test.go (3)
svc/api/routes/v2_deploy_generate_upload_url/401_test.go (1)
TestUnauthorizedAccess(14-51)pkg/testutil/http.go (2)
NewHarness(67-238)CallRoute(512-546)svc/api/routes/v2_deploy_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)
pkg/testutil/http.go (2)
pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)pkg/testutil/seed/seed.go (3)
CreateProjectRequest(124-132)New(39-46)CreateEnvironmentRequest(168-176)
svc/api/routes/v2_deploy_get_deployment/404_test.go (3)
pkg/testutil/http.go (1)
CallRoute(512-546)svc/api/routes/v2_deploy_get_deployment/handler.go (3)
Handler(23-28)Request(19-19)Response(20-20)pkg/testutil/seed/seed.go (1)
CreateProjectRequest(124-132)
svc/api/routes/v2_deploy_generate_upload_url/200_test.go (2)
pkg/testutil/http.go (1)
CreateTestDeploymentSetupOptions(303-308)svc/api/routes/v2_deploy_generate_upload_url/handler.go (1)
Request(19-19)
⏰ 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 (23)
svc/api/openapi/spec/paths/v2/deploy/createDeployment/V2DeployCreateDeploymentResponseData.yaml (1)
8-8: LGTM!The example value is now consistent with the actual deployment ID format (
d_prefix) as demonstrated in the PR testing instructions.svc/api/openapi/spec/paths/v2/deploy/getDeployment/V2DeployGetDeploymentRequestBody.yaml (1)
1-10: LGTM!The schema properly defines the request body with appropriate validation:
- Required
deploymentIdfield correctly specified- Regex pattern
^d_[a-zA-Z0-9]+$validates the expected deployment ID format- Example now uses the correct
d_prefix matching actual deployment IDsGood follow-up on the previous review feedback by adding the pattern validation and fixing the example format.
pkg/testutil/BUILD.bazel (1)
9-33: LGTM!The new Bazel dependencies correctly support the CTRL client integration added to the test harness. The additions (
ctrlv1connect,interceptor,uid,connect) align with the imports inhttp.go.svc/api/openapi/spec/paths/v2/deploy/createDeployment/V2DeployCreateDeploymentRequestBody.yaml (1)
13-13: LGTM!The pattern validation is consistent with the
generateUploadUrlendpoint, ensuring uniform input validation across deployment-related endpoints.svc/api/routes/v2_deploy_generate_upload_url/404_test.go (1)
18-23: LGTM!Good refactor to use the harness-provided
CtrlBuildClientinstead of manually creating the CTRL client in each test. This reduces boilerplate and centralizes client configuration.pkg/testutil/http.go (2)
62-63: LGTM!Good addition of CTRL service clients to the harness, enabling deployment-related tests to use pre-configured clients.
310-362: Well-structured test helper.The
CreateTestDeploymentSetuphelper effectively reduces boilerplate in deployment tests by encapsulating workspace, project, and environment creation with sensible defaults and customization options.svc/api/openapi/spec/paths/v2/deploy/generateUploadUrl/V2DeployGenerateUploadUrlRequestBody.yaml (1)
10-10: Pattern is correct and matches UID generation format.The pattern
^proj_[a-zA-Z0-9]+$correctly validates project IDs generated byuid.New(uid.ProjectPrefix). The uid package encodes random bytes using the Bitcoin Base58 alphabet (which contains only alphanumeric characters, excluding 0, I, O, i, l, o), and the final format isproj_followed by the base58-encoded value. The pattern appropriately enforces alphanumeric characters only, with no underscores, hyphens, or special characters in the encoded portion.svc/api/openapi/openapi-generated.yaml (3)
2649-2657: ExampledeploymentIdupdated tod_…prefix — good.
4273-4380: Internal tagging looks consistent; double-check how these endpoints are surfaced to consumers.
x-speakeasy-group: internal+ the “INTERNAL” disclaimer is clear, but these paths are still present in the generated OpenAPI. Verify your doc/SDK generation actually hides or segregates them as intended.
389-444: No action required onprojectIdregex pattern.The regex
^proj_[a-zA-Z0-9]+$already exists in the source spec files and is consistent with actual ID generation logic (proj_${generateRandomString(10)}). Examples likeproj_your_project_idare intentionally treated as invalid placeholders in validation checks, not legitimate ID formats. The pattern correctly constrains to alphanumeric characters only and does not reject valid production IDs.Likely an incorrect or invalid review comment.
svc/api/routes/v2_deploy_generate_upload_url/401_test.go (1)
17-22: LGTM!The migration to harness-provided
h.CtrlBuildClientis consistent with the test refactoring pattern across the PR and reduces boilerplate.svc/api/routes/v2_deploy_create_deployment/401_test.go (1)
18-23: LGTM!The migration to
h.CtrlDeploymentClientis consistent with the harness-based pattern used across the test suite.svc/api/routes/v2_deploy_get_deployment/401_test.go (1)
1-38: LGTM!The new unauthorized access test follows the established pattern and correctly tests that invalid authorization tokens return 401. The minimal setup (no workspace/project creation) is appropriate since authorization validation precedes database lookups.
svc/api/routes/v2_deploy_create_deployment/404_test.go (2)
19-24: LGTM!The migration to
h.CtrlDeploymentClientis consistent with the harness-based pattern.
81-83: Verify error type for environment not found case.The test expects
project_not_founderror type and "Project not found." detail when testing the environment not found scenario. This seems semantically incorrect - an environment-specific lookup failure should ideally return an environment-related error.If this is the current handler behavior (returning project_not_found for environment lookups), consider opening an issue to track returning a more specific error like
environment_not_found.svc/api/routes/v2_deploy_get_deployment/404_test.go (1)
1-53: LGTM!The test correctly validates the deployment not found scenario with proper harness setup and assertions.
svc/api/routes/v2_deploy_generate_upload_url/400_test.go (1)
23-23: LGTM!The change to use
h.CtrlBuildClientfrom the test harness simplifies test setup and aligns with the broader refactoring pattern across the deployment route tests.svc/api/routes/v2_deploy_create_deployment/400_test.go (1)
23-23: LGTM!The switch to
h.CtrlDeploymentClientis consistent with the harness-based client approach used across other deployment route tests.svc/api/routes/v2_deploy_generate_upload_url/200_test.go (1)
20-36: LGTM!The refactoring to use
h.CreateTestDeploymentSetupwithSkipEnvironment: trueis appropriate since upload URL generation doesn't require an environment. The test setup is now cleaner and consistent with the harness-based approach.svc/api/routes/v2_deploy_get_deployment/400_test.go (1)
1-98: Well-structured test file for the new getDeployment endpoint.The test covers essential bad request scenarios: missing/empty deploymentId and authorization issues. The implementation follows established patterns with proper use of
testify/requireandt.Run()for organization. As per coding guidelines, assertions userequireconsistently.svc/api/routes/v2_deploy_create_deployment/200_test.go (2)
22-22: LGTM!The switch to
h.CtrlDeploymentClientaligns with the harness-based approach across deployment tests.
26-133: Good use of CreateTestDeploymentSetup with varied configurations.Each subtest appropriately creates its own isolated setup with relevant options:
- Default setup for basic docker image test
- Custom project/environment slugs for build context test
- Custom project name for git commit info test
This provides good test isolation and demonstrates the flexibility of the helper.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/testutil/http.go`:
- Around line 314-319: The default ProjectSlug in the
CreateTestDeploymentSetupOptions literal is incorrectly set to "production";
update the ProjectSlug field to "test-project" so it matches ProjectName and
avoids confusion with EnvironmentSlug—locate the
CreateTestDeploymentSetupOptions initialization (the config variable) and change
ProjectSlug: "production" to ProjectSlug: "test-project".
♻️ Duplicate comments (1)
svc/api/openapi/openapi-generated.yaml (1)
4273-4326: Docs still claim “appropriate permissions” despite deployment authz being intentionally missingGiven the PR objective (“permission checks will be added later”), the OpenAPI descriptions should not imply authorization enforcement.
Proposed doc wording fix (apply to createDeployment, generateUploadUrl, getDeployment)
- **Authentication**: Requires a valid root key with appropriate permissions. + **Authentication**: Requires a valid root key. + **Authorization**: No per-project/deployment permission checks are currently enforced.Also applies to: 4327-4379, 4380-4433
🧹 Nitpick comments (2)
svc/api/openapi/openapi-generated.yaml (1)
2672-2729: Consider makingV2DeployDeploymentStep.createdAt(and possiblystatus) required if always presentRight now
V2DeployDeploymentStephas norequiredfields, so clients can’t rely on timestamps or status being set even whenstepsis returned.pkg/testutil/http.go (1)
188-206: Consider extracting the shared interceptor to reduce duplication.Both clients use the same authorization header interceptor. Extracting it to a variable improves readability and ensures consistency.
♻️ Suggested refactor
// Get CTRL service URL and token ctrlURL, ctrlToken := containers.ControlPlane(t) +// Create shared auth interceptor for CTRL clients +ctrlAuthInterceptor := interceptor.NewHeaderInjector(map[string]string{ + "Authorization": fmt.Sprintf("Bearer %s", ctrlToken), +}) + // Create CTRL clients ctrlDeploymentClient := ctrlv1connect.NewDeploymentServiceClient( http.DefaultClient, ctrlURL, - connect.WithInterceptors(interceptor.NewHeaderInjector(map[string]string{ - "Authorization": fmt.Sprintf("Bearer %s", ctrlToken), - })), + connect.WithInterceptors(ctrlAuthInterceptor), ) ctrlBuildClient := ctrlv1connect.NewBuildServiceClient( http.DefaultClient, ctrlURL, - connect.WithInterceptors(interceptor.NewHeaderInjector(map[string]string{ - "Authorization": fmt.Sprintf("Bearer %s", ctrlToken), - })), + connect.WithInterceptors(ctrlAuthInterceptor), )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/testutil/http.gosvc/api/openapi/gen.gosvc/api/openapi/openapi-generated.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Organize Go imports in groups separated by blank lines: (1) Standard library, (2) External/third-party packages, (3) Internal packages (github.com/unkeyed/unkey/internal/...), (4) Package-level (github.com/unkeyed/unkey/pkg/...), (5) Service-level (github.com/unkeyed/unkey/svc/...), (6) Generated code (github.com/unkeyed/unkey/gen/...)
Use thefaultpackage for structured error handling withfault.Wrap(),fault.Code(),fault.Internal(), andfault.Public()methods
Usesnake_case.gofor Go filenames and*_test.gofor test files
UsePascalCasefor exported Go functions and types,camelCasefor unexported functions and types
Use short receiver names like(s *Service)or(h *Handler)in Go methods
UseSCREAMING_SNAKE_CASEfor Go constants
Runmake bazelafter adding new Go files to sync BUILD.bazel files
Runmake fmtbefore committing Go changes to format and lint code
Files:
pkg/testutil/http.gosvc/api/openapi/gen.go
svc/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the
pkg/zencustom HTTP framework for Go HTTP handlers with the pattern of binding request bodies, handling business logic, and returning JSON responses
Files:
svc/api/openapi/gen.go
🧠 Learnings (10)
📓 Common learnings
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.
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-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:
pkg/testutil/http.go
📚 Learning: 2025-09-11T08:17:21.423Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/services/deployment/fallbacks/k8s.go:236-247
Timestamp: 2025-09-11T08:17:21.423Z
Learning: The K8s fallback backend in go/apps/ctrl/services/deployment/fallbacks/k8s.go is specifically designed for scenarios where the ctrl service runs inside a Kubernetes cluster. It should not be used when ctrl runs outside the cluster - in those cases, the Docker fallback should be used instead.
Applied to files:
pkg/testutil/http.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/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts:110-147
Timestamp: 2025-07-25T19:11:00.208Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts, the user mcstepp prefers to keep mock data fallbacks in POC/demonstration code for simplicity, even if it wouldn't be production-ready. This aligns with the PR being work-in-progress for demonstration purposes.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 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-25T15:12:21.739Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: go/cmd/deploy/control_plane.go:64-69
Timestamp: 2025-09-25T15:12:21.739Z
Learning: The API key authentication system in go/cmd/deploy/control_plane.go and related files is temporary demo code that will be replaced with a proper authentication system for private beta.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-16T17:51:57.297Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3617
File: go/apps/api/openapi/openapi.yaml:3309-3312
Timestamp: 2025-07-16T17:51:57.297Z
Learning: In the Unkey API OpenAPI schema, the permissions query regex for the verifyKey endpoint intentionally allows all whitespace characters (including tabs and newlines) via `\s`. Do not flag this as an error in future reviews.
Applied to files:
svc/api/openapi/openapi-generated.yaml
📚 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/openapi/openapi-generated.yaml
🧬 Code graph analysis (1)
pkg/testutil/http.go (3)
pkg/rpc/interceptor/header.go (1)
NewHeaderInjector(22-24)svc/api/integration/harness.go (2)
Harness(36-47)New(56-118)pkg/testutil/seed/seed.go (3)
CreateProjectRequest(124-132)New(39-46)CreateEnvironmentRequest(168-176)
⏰ 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)
- GitHub Check: Bazel / Build
- GitHub Check: Bazel / Test
🔇 Additional comments (12)
svc/api/openapi/gen.go (4)
30-39: LGTM - Status constants follow established patterns.The new deployment status constants use
SCREAMING_SNAKE_CASEas required by the coding guidelines for Go constants and are consistent with other status constant patterns in the file (e.g.,V2KeysVerifyKeyResponseDataCodeat lines 49-58).
757-770: LGTM - Deployment step type is well-structured.The
V2DeployDeploymentSteptype correctly uses pointer types for all optional fields with appropriateomitemptyJSON tags, following the established pattern in this generated file.
795-828: LGTM - GetDeployment request/response types are consistent.The new types follow the established patterns in this file:
- Request body contains only the required
DeploymentIdidentifier- Response body uses the standard
Data+Metastructure- Response data includes required fields (
Id,Status) and appropriate optional fields- Status type provides type safety by referencing the constants defined at lines 30-39
2428-2429: LGTM - JSON request body type alias follows established pattern.The
DeployGetDeploymentJSONRequestBodyalias is consistent with other request body aliases in this generated file (e.g.,DeployCreateDeploymentJSONRequestBodyat line 2423).svc/api/openapi/openapi-generated.yaml (4)
433-444: KeepprojectIdvalidation consistent across deploy endpointsGood to see the same
projectIdpattern applied here—just ensure it matches real IDs (same concern as createDeployment).
454-475: GetDeployment request/response envelopes look consistent with the rest of v2Request body is minimal and the
deploymentIdpattern aligns with thed_prefix used elsewhere.
2649-2657: Good: deployment ID example prefix corrected tod_This avoids confusing clients with
dep_examples when the system usesd_….
389-423: No issues found with theprojectIdregex pattern. All project IDs in the codebase use alphanumeric characters only (e.g.,proj_test456,proj_123abc,proj_flo). The pattern^proj_[a-zA-Z0-9]+$correctly matches the actual ID format in use and will not cause avoidable rejections.pkg/testutil/http.go (4)
3-40: LGTM!The new imports are correctly placed within the existing import structure and all appear to be used in the new code.
62-63: LGTM!The new CTRL client fields are appropriately named and exported for test access.
222-223: LGTM!The CTRL clients are correctly wired into the Harness struct.
311-366: LGTM!The
CreateTestDeploymentSetupmethod follows the established pattern in this file (similar toSetupAnalytics), correctly usest.Helper(), and properly handles the optional environment creation based onSkipEnvironment. The resource creation order (workspace → root key → project → environment) is correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
What does this PR do?
This PR creates an endpoint for
getDeploymentprocess to decouple CLI and CTRL. That way CLI can talk to API instead of CTRL directly.Note:
/v2/deploy.getDeploymentdoesn't do any permission check for deployment related stuff. Will be added soon.Type of change
How should this be tested?
http://localhost:3000/[workspace]/settings/root-keysrootKeyhttp://localhost:3000/[workspace]/projectsproject_idAuthorization: Bearer xxxExpected: 200 OK with {"data":{"id":"d_5TV5hCUQ3atBZNz3","status":"DEPLOYING"},"meta":{"requestId":"req_xxxx"}}
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated