Conversation
|
📝 WalkthroughWalkthroughReplaces legacy vmprovisioner with metald v1 protos/client, refactors deployment workflow to use metald deployments and OpenAPI scraping/storage, updates buf config, adds Workspace.slug to DB models/queries, and disables the metald-aio docker-compose service. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant CTRL as ctrl (DeployWorkflow)
participant MDS as metald VmService
participant PDB as PartitionDB
participant GW as GatewayConfig
participant APP as DeployedApp
User->>CTRL: Request deployment
CTRL->>MDS: CreateDeployment(deployment)
MDS-->>CTRL: CreateDeploymentResponse(vm_ids)
loop Poll deployment readiness
CTRL->>MDS: GetDeployment(deployment_id)
MDS-->>CTRL: GetDeploymentResponse(vms{state,host,port})
end
CTRL->>PDB: Upsert VM addresses/status (for each vm)
par Scrape OpenAPI
CTRL->>APP: GET /openapi.yaml (via host:mappedPort)
APP-->>CTRL: OpenAPI spec (or fail)
and Update Gateway Config
CTRL->>GW: Fetch gateway config by hostname
GW-->>CTRL: Gateway config
CTRL->>GW: Upsert gateway config (inject OpenAPI)
GW-->>CTRL: Ack
end
CTRL-->>User: Deployment ready / succeeded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{env,js,ts,go}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-07-15T14:59:30.212ZApplied to files:
🧬 Code graph analysis (1)go/apps/ctrl/services/deployment/deploy_workflow.go (6)
🪛 golangci-lint (2.2.2)go/apps/ctrl/services/deployment/deploy_workflow.go180-180: apps/ctrl/services/deployment/deploy_workflow.go:180: Line contains TODO/BUG/FIXME: "TODO derive from spec" (godox) 181-181: apps/ctrl/services/deployment/deploy_workflow.go:181: Line contains TODO/BUG/FIXME: "TODO derive from spec" (godox) 182-182: apps/ctrl/services/deployment/deploy_workflow.go:182: Line contains TODO/BUG/FIXME: "TODO" (godox) ⏰ 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)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
imeyer
left a comment
imeyer
left a comment
There was a problem hiding this comment.
LGTM
(can't wait to see rabbit suggest widening integers for the fifth time)
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
deployment/docker-compose.yaml (1)
289-333: Good to keep metald-aio stubbed; add a profile so it can be toggled on.Profiles let devs
docker compose --profile metald upto enable both ctrl+metald paths without changing the file.- # metald-aio: + # metald-aio: + # profiles: ["metald"] # build: # context: ../go # dockerfile: deploy/Dockerfile.dev # platform: linux/amd64 # container_name: metald-aio # hostname: metald-aio # privileged: true # Required for systemdgo/apps/ctrl/run.go (4)
174-186: Externalize auth headers; avoid hard-coded dev credentials.Hard-coded Authorization/X-Tenant-ID will leak into non-dev runs. Gate by config and omit when empty. Use UNKEY_CTRL_METALD_TOKEN and UNKEY_CTRL_TENANT_ID per env naming guidelines.
Apply:
metaldClient := metaldv1connect.NewVmServiceClient( httpClient, cfg.MetaldAddress, connect.WithInterceptors(connect.UnaryInterceptorFunc(func(next connect.UnaryFunc) connect.UnaryFunc { return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { - logger.Info("Adding auth headers to metald request", "procedure", req.Spec().Procedure) - req.Header().Set("Authorization", "Bearer dev_user_ctrl") - req.Header().Set("X-Tenant-ID", "ctrl-tenant") + // Use config-driven headers; log at debug level to reduce noise + logger.Debug("metald request", "procedure", req.Spec().Procedure) + if cfg.MetaldAuthToken != "" { + req.Header().Set("Authorization", "Bearer "+cfg.MetaldAuthToken) + } + if cfg.TenantID != "" { + req.Header().Set("X-Tenant-ID", cfg.TenantID) + } return next(ctx, req) } })), )
167-173: Set transport timeouts for plain HTTP; current client may hang on dial.For the non-SPIFFE path you create a default client with only Timeout set. Configure Transport-level timeouts and connection pooling.
Apply:
- httpClient = &http.Client{} + httpClient = &http.Client{ + Transport: &http.Transport{ + MaxIdleConns: 100, + MaxIdleConnsPerHost: 10, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + ResponseHeaderTimeout: 15 * time.Second, + }, + }
213-229: Reduce h2c server struct noise; zero-values are redundant and obscure intent.The explicit zero assignments add no value and hinder readability.
Apply:
- h2cHandler := h2c.NewHandler(mux, &http2.Server{ - MaxHandlers: 0, - MaxConcurrentStreams: 0, - MaxDecoderHeaderTableSize: 0, - MaxEncoderHeaderTableSize: 0, - MaxReadFrameSize: 0, - PermitProhibitedCipherSuites: false, - IdleTimeout: 0, - ReadIdleTimeout: 0, - PingTimeout: 0, - WriteByteTimeout: 0, - MaxUploadBufferPerConnection: 0, - MaxUploadBufferPerStream: 0, - NewWriteScheduler: nil, - CountError: nil, - }) + h2cHandler := h2c.NewHandler(mux, &http2.Server{})
174-186: Enforce HTTP/HTTPS scheme for MetaldAddress
Add a validation in go/apps/ctrl/config.go’s parsing logic to error if cfg.MetaldAddress doesn’t start withhttp://orhttps://, preventing Connect-go from failing at runtime.go/apps/ctrl/services/deployment/deploy_workflow.go (8)
23-29: Exported type DeployWorkflow lacks doc comment.Add a GoDoc per guidelines.
Apply:
+// DeployWorkflow orchestrates deployment via metald and updates Unkey DBs. type DeployWorkflow struct { db db.Database partitionDB db.Database logger logging.Logger metaldClient metaldv1connect.VmServiceClient }
31-41: Exported constructor needs documentation.Add GoDoc and clarify dependencies.
Apply:
-// NewDeployWorkflow creates a new deploy workflow instance +// NewDeployWorkflow constructs a DeployWorkflow. +// Dependencies must be live for the workflow lifetime. func NewDeployWorkflow(database db.Database, partitionDB db.Database, logger logging.Logger, metaldClient metaldv1connect.VmServiceClient, ) *DeployWorkflow {
43-46: Document exported method Name.Apply:
-// Name returns the workflow name for registration +// Name returns the Hydra workflow name used for registration. func (w *DeployWorkflow) Name() string {
48-56: Exported DTO DeployRequest needs documentation.Apply:
+// DeployRequest is the input payload for the deployment workflow. type DeployRequest struct {
58-63: Exported DTO DeploymentResult needs documentation.Apply:
+// DeploymentResult captures the final deployment outcome. type DeploymentResult struct {
65-66: Document exported method Run and add AIDEV note.Apply:
-// Run executes the complete build and deployment workflow +// Run executes the complete deployment workflow. +// AIDEV-NOTE: This method is long-running and responds to context cancellation. func (w *DeployWorkflow) Run(ctx hydra.WorkflowContext, req *DeployRequest) error {
337-356: Status/log inconsistency: using “ready” vs “active”.You set DB status to Ready but log/return “active”. Keep consistent.
Apply:
- w.logger.Info("deployment complete", "deployment_id", req.DeploymentID, "status", "active") + w.logger.Info("deployment complete", "deployment_id", req.DeploymentID, "status", "ready") @@ - Status: "active", + Status: "ready",
200-257: AIDEV comment for gateway-config step to document invariants.This step is critical. Add an AIDEV-* comment with invariants (createdVMs non-empty, partitionDB live).
Apply:
err = hydra.StepVoid(ctx, "create-gateway-config", func(stepCtx context.Context) error { + // AIDEV-INVARIANTS: + // - createdVMs contains at least one running VM + // - partitionDB is initialized (checked below)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (30)
go/gen/proto/ctrl/v1/acme.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/build.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/acme.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/build.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/deployment.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/openapi.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/routing.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/service.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/openapi.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/routing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/assetmanagerd/v1/asset.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/assetmanagerd/v1/assetmanagerdv1connect/asset.connect.gois excluded by!**/gen/**go/gen/proto/deploy/billaged/v1/billagedv1connect/billing.connect.gois excluded by!**/gen/**go/gen/proto/deploy/billaged/v1/billing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/builderd/v1/builder.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/builderd/v1/builderdv1connect/builder.connect.gois excluded by!**/gen/**go/gen/proto/metal/vmprovisioner/v1/vmprovisioner.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metal/vmprovisioner/v1/wip.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/metald.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/metaldv1connect/metald.connect.gois excluded by!**/gen/**go/gen/proto/metald/v1/network.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/storage.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/vm.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/partition/v1/gateway.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/object.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/vaultv1connect/service.connect.gois excluded by!**/gen/**
📒 Files selected for processing (21)
deployment/docker-compose.yaml(2 hunks)go/apps/api/openapi/openapi-generated.yaml(1 hunks)go/apps/ctrl/run.go(2 hunks)go/apps/ctrl/services/deployment/deploy_workflow.go(11 hunks)go/buf.gen.yaml(1 hunks)go/buf.yaml(1 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(3 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(3 hunks)go/pkg/db/models_generated.go(1 hunks)go/pkg/db/querier_generated.go(4 hunks)go/pkg/db/workspace_find_by_id.sql_generated.go(2 hunks)go/pkg/db/workspace_list.sql_generated.go(3 hunks)go/pkg/otel/logging/slog.go(0 hunks)go/pkg/partition/db/queries/vm_upsert.sql(1 hunks)go/proto/metal/vmprovisioner/v1/vmprovisioner.proto(0 hunks)go/proto/metal/vmprovisioner/v1/wip.proto(0 hunks)go/proto/metald/v1/deployment.proto(1 hunks)go/proto/metald/v1/metald.proto(1 hunks)go/proto/metald/v1/network.proto(1 hunks)go/proto/metald/v1/storage.proto(1 hunks)go/proto/metald/v1/vm.proto(1 hunks)
💤 Files with no reviewable changes (3)
- go/pkg/otel/logging/slog.go
- go/proto/metal/vmprovisioner/v1/wip.proto
- go/proto/metal/vmprovisioner/v1/vmprovisioner.proto
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/pkg/db/models_generated.gogo/apps/ctrl/run.gogo/pkg/db/querier_generated.gogo/pkg/db/workspace_find_by_id.sql_generated.gogo/pkg/db/workspace_list.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/apps/ctrl/services/deployment/deploy_workflow.gogo/pkg/db/key_find_live_by_hash.sql_generated.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/pkg/db/models_generated.gogo/apps/ctrl/run.gogo/pkg/db/querier_generated.gogo/pkg/db/workspace_find_by_id.sql_generated.gogo/pkg/db/workspace_list.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/apps/ctrl/services/deployment/deploy_workflow.gogo/pkg/db/key_find_live_by_hash.sql_generated.go
🧠 Learnings (6)
📚 Learning: 2025-09-01T02:33:43.749Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.749Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/buf.gen.yaml
📚 Learning: 2025-09-01T01:57:42.213Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/metald.proto:5-9
Timestamp: 2025-09-01T01:57:42.213Z
Learning: In the unkeyed/unkey repository, buf is configured to properly resolve metald proto imports like "metald/v1/vm.proto" without needing the full "go/proto/" prefix. The buf lint command `buf lint --path proto/metald` passes successfully with these relative import paths.
Applied to files:
go/buf.gen.yamlgo/buf.yaml
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/apps/ctrl/run.gogo/apps/ctrl/services/deployment/deploy_workflow.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/apps/ctrl/run.go
📚 Learning: 2024-10-23T12:05:31.121Z
Learnt from: chronark
PR: unkeyed/unkey#2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The `cloudflareRatelimiter` type definition in `apps/api/src/pkg/env.ts` should not have its interface changed; it should keep the `limit` method returning `Promise<{ success: boolean }>` without additional error properties.
Applied to files:
go/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-09-01T15:10:44.916Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/deployment.proto:7-15
Timestamp: 2025-09-01T15:10:44.916Z
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:
go/proto/metald/v1/deployment.proto
🧬 Code graph analysis (5)
go/apps/ctrl/run.go (1)
go/gen/proto/metald/v1/metaldv1connect/metald.connect.go (1)
NewVmServiceClient(104-187)
go/pkg/db/workspace_list.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
Workspace(770-788)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
Workspace(770-788)
go/apps/ctrl/services/deployment/deploy_workflow.go (6)
go/gen/proto/metald/v1/metaldv1connect/metald.connect.go (1)
VmServiceClient(68-95)go/pkg/hydra/step.go (1)
StepVoid(302-310)go/gen/proto/metald/v1/deployment.pb.go (15)
CreateDeploymentResponse(144-149)CreateDeploymentResponse(162-162)CreateDeploymentResponse(177-179)CreateDeploymentRequest(100-105)CreateDeploymentRequest(118-118)CreateDeploymentRequest(133-135)DeploymentRequest(24-33)DeploymentRequest(46-46)DeploymentRequest(61-63)GetDeploymentResponse_Vm(452-460)GetDeploymentResponse_Vm(473-473)GetDeploymentResponse_Vm(488-490)GetDeploymentRequest(356-361)GetDeploymentRequest(374-374)GetDeploymentRequest(389-391)go/pkg/db/models_generated.go (1)
Deployment(527-545)go/pkg/partition/db/vm_upsert.sql_generated.go (1)
UpsertVMParams(24-31)go/pkg/partition/db/models_generated.go (1)
VmsStatusAllocated(60-60)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
Workspace(770-788)
🪛 Buf (1.55.1)
go/proto/metald/v1/network.proto
3-3: Files with package "metald.v1" must be within a directory "metald/v1" relative to root but were in directory "go/proto/metald/v1".
(PACKAGE_DIRECTORY_MATCH)
go/proto/metald/v1/vm.proto
8-8: import "metald/v1/network.proto": file does not exist
(COMPILE)
go/proto/metald/v1/deployment.proto
5-5: import "metald/v1/vm.proto": file does not exist
(COMPILE)
go/proto/metald/v1/storage.proto
3-3: Files with package "metald.v1" must be within a directory "metald/v1" relative to root but were in directory "go/proto/metald/v1".
(PACKAGE_DIRECTORY_MATCH)
go/proto/metald/v1/metald.proto
5-5: import "metald/v1/vm.proto": file does not exist
(COMPILE)
⏰ 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: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
🔇 Additional comments (14)
go/pkg/db/workspace_find_by_id.sql_generated.go (1)
13-15: LGTM: slug added and scan order remains consistent.SELECT includes ws.slug and Scan maps to Workspace.Slug right after Name, matching the struct layout.
Also applies to: 19-20, 28-29
go/pkg/db/querier_generated.go (1)
313-314: Docs synced to include slug in workspace projections.Commented SQL now reflects ws.slug/w.slug in all relevant projections; interface surface unchanged.
Also applies to: 404-405, 696-697, 1487-1488
go/pkg/db/workspace_list.sql_generated.go (1)
14-15: LGTM: slug added to ListWorkspaces projection and scan.Projection and Scan both include slug after name; aligns with Workspace struct.
Also applies to: 31-32, 51-52
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
18-19: LGTM: workspace slug included in FindLiveKeyByID.Adds ws.slug to SELECT and Scan; result shape remains stable besides the extra field.
Also applies to: 149-150, 284-285
go/pkg/db/key_find_live_by_hash.sql_generated.go (2)
18-18: ws.slug correctly added to SELECT; ordering matches scan fieldsThe projection order aligns with row.Scan; nullability fits Workspace.Slug (sql.NullString). No action needed.
148-148: Doc block synced with SELECT (includes ws.slug)Keeps generated docs consistent; should satisfy linters.
go/buf.gen.yaml (1)
3-8: Generation path and go_package alignment verified. Generated files appear under go/gen/proto/... and all protogo_packageoptions correctly reference that directory.go/pkg/partition/db/queries/vm_upsert.sql (1)
10-10: LGTM — formatting only.No functional change to the UPSERT semantics.
go/apps/api/openapi/openapi-generated.yaml (1)
5690-5690: Clearer 200/“success” semantics for ratelimit.limit — good.The note reduces SDK/client ambiguity. Ensure downstream SDKs don’t treat
success: falsewith HTTP 200 as transport success; they should surface a typed result.Please regenerate SDKs and spot-check the ratelimit client method to confirm it checks
data.successrather than status code.go/buf.yaml (1)
1-3: Use the correct Buf lint invocation to verify theprotomodule
From thego/directory, run:cd go buf lint protoThis should succeed without errors, confirming your
modules: [{ path: proto }]configuration is valid.go/proto/metald/v1/deployment.proto (1)
5-8: Import path doesn’t resolve; align Buf roots or adjust imports.Buf reports metald/v1/vm.proto not found. Same root cause as storage.proto: build roots. Fix buf.yaml (preferred) or change imports to "go/proto/metald/v1/vm.proto". Keep imports consistent across files.
Suggested buf.yaml is in the storage.proto comment.
⛔ Skipped due to learnings
Learnt from: imeyer PR: unkeyed/unkey#3899 File: go/proto/metald/v1/metald.proto:5-9 Timestamp: 2025-09-01T01:57:42.213Z 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.Learnt from: chronark PR: unkeyed/unkey#3560 File: go/deploy/metald/internal/database/repository.go:0-0 Timestamp: 2025-07-15T14:59:30.212Z Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.Learnt from: imeyer PR: unkeyed/unkey#3899 File: go/buf.gen.yaml:0-0 Timestamp: 2025-09-01T02:33:43.749Z Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.go/apps/ctrl/services/deployment/deploy_workflow.go (1)
361-509: Large commented block: either remove or guard behind a feature flag.Dead/commented code makes maintenance harder and confuses ownership.
- Remove the block, or
- Hide behind a config flag (e.g., UNKEY_CTRL_SCRAPE_OPENAPI=true) with clear TODO owner and timeline.
⛔ Skipped due to learnings
Learnt from: ogzhanolguncu PR: unkeyed/unkey#3499 File: apps/dashboard/app/new/hooks/use-workspace-step.tsx:19-26 Timestamp: 2025-07-11T13:00:05.416Z Learning: In the Unkey codebase, ogzhanolguncu prefers to keep commented code for planned future features (like slug-based workspaces) rather than removing it, as it serves as a reference for upcoming implementation.go/proto/metald/v1/network.proto (1)
3-3: Ignore buf package-directory mismatch. Buf’s PACKAGE_DIRECTORY_MATCH rule only requires the file path to suffix-match the package, and go/proto/metald/v1/network.proto correctly ends with metald/v1.Likely an incorrect or invalid review comment.
go/proto/metald/v1/vm.proto (1)
8-9: Ignore broken-imports suggestion
Themetald/v1protos build successfully under the existing buf module root (go/proto), so imports like"metald/v1/network.proto"and"metald/v1/storage.proto"resolve correctly and do not need adjustment. Remove this comment.Likely an incorrect or invalid review comment.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (09/02/25)1 gif was posted to this PR based on Andreas Thomas's automation. "Notify author when CI fails" took an action on this PR • (09/02/25)1 teammate was notified to this PR based on Andreas Thomas's automation. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
go/apps/ctrl/services/deployment/deploy_workflow.go (6)
200-206: Propagate polling step error before using createdVMs.- }) - - err = hydra.StepVoid(ctx, "create-gateway-config", func(stepCtx context.Context) error { + }) + if err != nil { + w.logger.Error("deployment prepare polling failed", "error", err, "deployment_id", req.DeploymentID) + return err + } + + err = hydra.StepVoid(ctx, "create-gateway-config", func(stepCtx context.Context) error {
91-104: Rename step labels from “version” to “deployment” for consistency.- _, err = hydra.Step(ctx, "update-version-building", func(stepCtx context.Context) (*struct{}, error) { + _, err = hydra.Step(ctx, "update-deployment-building", func(stepCtx context.Context) (*struct{}, error) { @@ - _, err = hydra.Step(ctx, "update-version-deploying", func(stepCtx context.Context) (*struct{}, error) { + _, err = hydra.Step(ctx, "update-deployment-deploying", func(stepCtx context.Context) (*struct{}, error) {Also applies to: 137-149
75-83: Prefer typed status constants over raw string “pending”.If available in db package (e.g., db.DeploymentStepsStatusPending), use it:
- Status: "pending", + Status: db.DeploymentStepsStatusPending,Confirm the enum name in your DB layer.
337-356: Unify status to “ready” (logs, error text, return).- w.logger.Info("updating deployment status to ready", "deployment_id", req.DeploymentID, "completion_time", completionTime) + w.logger.Info("updating deployment status to ready", "deployment_id", req.DeploymentID, "completion_time", completionTime) @@ - w.logger.Error("failed to update deployment status to active", "error", activeErr, "deployment_id", req.DeploymentID) - return nil, fmt.Errorf("failed to update deployment status to active: %w", activeErr) + w.logger.Error("failed to update deployment status to ready", "error", activeErr, "deployment_id", req.DeploymentID) + return nil, fmt.Errorf("failed to update deployment status to ready: %w", activeErr) @@ - w.logger.Info("deployment complete", "deployment_id", req.DeploymentID, "status", "active") + w.logger.Info("deployment complete", "deployment_id", req.DeploymentID, "status", "ready") @@ - Status: "active", + Status: "ready",
271-288: Implement RFC-1123 hostname sanitization
Replace the ad-hoc underscore replacement with a helper that lower-cases, strips invalid chars, collapses/trims dashes, enforces a 63-char label limit (and a 253-char total), and apply it to branch, identifier, and workspaceID in go/apps/ctrl/services/deployment/deploy_workflow.go:// sanitizeHostnameLabel converts an arbitrary string to an RFC-1123 label. func sanitizeHostnameLabel(s string) string { s = strings.ToLower(s) reInvalid := regexp.MustCompile(`[^a-z0-9-]`) s = reInvalid.ReplaceAllString(s, "-") s = strings.Trim(s, "-") reDashes := regexp.MustCompile(`-+`) s = reDashes.ReplaceAllString(s, "-") if len(s) > 63 { s = s[:63] } if s == "" { return "x" } return s }- cleanIdentifier := strings.ReplaceAll(identifier, "_", "-") - primaryHostname := fmt.Sprintf("%s-%s-%s.unkey.app", branch, cleanIdentifier, req.WorkspaceID) + primaryHostname := fmt.Sprintf( + "%s-%s-%s.unkey.app", + sanitizeHostnameLabel(branch), + sanitizeHostnameLabel(identifier), + sanitizeHostnameLabel(req.WorkspaceID), + ) + if len(primaryHostname) > 253 { + primaryHostname = primaryHostname[:253] + }
207-214: Guard against empty VM set when configuring gateway
Add a check after the partitionDB validation to error out ifcreatedVMsis empty:// Validate partition DB connection if w.partitionDB == nil { w.logger.Error("CRITICAL: partition database not initialized for gateway config") return fmt.Errorf("partition database not initialized for gateway config") } + if len(createdVMs) == 0 { + w.logger.Error("no VMs available for gateway config", "deployment_id", req.DeploymentID) + return fmt.Errorf("no VMs available to configure gateway") + }
♻️ Duplicate comments (3)
go/apps/ctrl/services/deployment/deploy_workflow.go (3)
112-125: Fix stale CreateVm/PrepareDeployment texts to match CreateDeployment.Comment and messages are misleading; they should reference CreateDeployment.
- // Make real metald CreateVm call + // Call metald CreateDeployment @@ - if err != nil { - w.logger.Error("metald PrepareDeployment call failed", "error", err, "docker_image", req.DockerImage) - return nil, fmt.Errorf("failed to create VM: %w", err) + if err != nil { + w.logger.Error("metald CreateDeployment call failed", "error", err, "docker_image", req.DockerImage) + return nil, fmt.Errorf("failed to create deployment: %w", err) }
159-166: Make polling loop Go 1.21-compatible and cancellation-aware; lower log level.- for i := range 300 { - time.Sleep(time.Second) - - w.logger.Info("Polling deployment", "i", i) + for i := 0; i < 300; i++ { + select { + case <-stepCtx.Done(): + return nil, stepCtx.Err() + case <-time.After(1 * time.Second): + } + + w.logger.Debug("polling deployment", "i", i)Also applies to: 162-162
176-184: Do not ignore UpsertVM errors; also use 1024 MiB.- partitiondb.Query.UpsertVM(stepCtx, w.partitionDB.RW(), partitiondb.UpsertVMParams{ + if err := partitiondb.Query.UpsertVM(stepCtx, w.partitionDB.RW(), partitiondb.UpsertVMParams{ ID: instance.Id, DeploymentID: req.DeploymentID, Address: sql.NullString{Valid: true, String: fmt.Sprintf("%s:%d", instance.Host, instance.Port)}, - CpuMillicores: 1000, // TODO - MemoryMb: 1000, // TODO + CpuMillicores: 1000, // TODO derive from spec + MemoryMb: 1024, // TODO derive from spec Status: partitiondb.VmsStatusAllocated, // TODO - }) + }); err != nil { + w.logger.Error("failed to upsert VM", "error", err, "vm_id", instance.Id) + return nil, fmt.Errorf("failed to upsert VM %s: %w", instance.Id, err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
go/apps/ctrl/services/deployment/deploy_workflow.go(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/ctrl/services/deployment/deploy_workflow.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/ctrl/services/deployment/deploy_workflow.go
🧠 Learnings (1)
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/apps/ctrl/services/deployment/deploy_workflow.go
🧬 Code graph analysis (1)
go/apps/ctrl/services/deployment/deploy_workflow.go (7)
go/gen/proto/metald/v1/metaldv1connect/metald.connect.go (1)
VmServiceClient(68-95)go/pkg/hydra/step.go (1)
StepVoid(302-310)go/gen/proto/metald/v1/deployment.pb.go (15)
CreateDeploymentResponse(144-149)CreateDeploymentResponse(162-162)CreateDeploymentResponse(177-179)CreateDeploymentRequest(100-105)CreateDeploymentRequest(118-118)CreateDeploymentRequest(133-135)DeploymentRequest(24-33)DeploymentRequest(46-46)DeploymentRequest(61-63)GetDeploymentResponse_Vm(452-460)GetDeploymentResponse_Vm(473-473)GetDeploymentResponse_Vm(488-490)GetDeploymentRequest(356-361)GetDeploymentRequest(374-374)GetDeploymentRequest(389-391)go/gen/proto/partition/v1/gateway.pb.go (6)
Deployment(104-110)Deployment(123-123)Deployment(138-140)VM(208-213)VM(226-226)VM(241-243)go/pkg/db/models_generated.go (1)
Deployment(527-545)go/pkg/partition/db/vm_upsert.sql_generated.go (1)
UpsertVMParams(24-31)go/gen/proto/metald/v1/vm.pb.go (1)
VmState_VM_STATE_RUNNING(31-31)
🪛 golangci-lint (2.2.2)
go/apps/ctrl/services/deployment/deploy_workflow.go
180-180: apps/ctrl/services/deployment/deploy_workflow.go:180: Line contains TODO/BUG/FIXME: "TODO"
(godox)
181-181: apps/ctrl/services/deployment/deploy_workflow.go:181: Line contains TODO/BUG/FIXME: "TODO"
(godox)
182-182: apps/ctrl/services/deployment/deploy_workflow.go:182: Line contains TODO/BUG/FIXME: "TODO"
(godox)
155-155: ineffectual assignment to err
(ineffassign)
197-197: ST1005: error strings should not be capitalized
(staticcheck)
⏰ 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: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
🔇 Additional comments (1)
go/apps/ctrl/services/deployment/deploy_workflow.go (1)
289-301: Domain type may be incorrect for managed subdomain.You’re creating a managed unkey.app subdomain but store Type=Custom. If you have a “managed/auto” enum, prefer that to avoid UI/policy confusion.
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 (3)
go/apps/ctrl/services/deployment/deploy_workflow.go (3)
293-298: Normalize generated hostname to lowercase.DNS is case-insensitive; store canonical lowercase to avoid duplicates.
Apply:
- primaryHostname := fmt.Sprintf("%s-%s-%s.unkey.app", branch, cleanIdentifier, req.WorkspaceID) + primaryHostname := strings.ToLower(fmt.Sprintf("%s-%s-%s.unkey.app", branch, cleanIdentifier, req.WorkspaceID))
273-276: Minor: message says “version”; this is a deployment.Apply:
- w.logger.Info("assigning domains to version", "deployment_id", req.DeploymentID) + w.logger.Info("assigning domains to deployment", "deployment_id", req.DeploymentID)
346-365: Align status wording: use “ready” consistently.DB status is DeploymentsStatusReady; logs/result say “active”.
Apply:
- w.logger.Info("deployment complete", "deployment_id", req.DeploymentID, "status", "active") + w.logger.Info("deployment ready", "deployment_id", req.DeploymentID, "status", "ready") @@ - Status: "active", + Status: "ready",
♻️ Duplicate comments (4)
go/apps/ctrl/services/deployment/deploy_workflow.go (4)
183-193: Good: UpsertVM errors are now handled. Also derive CPU/Memory/Status from metald when available.When the proto surface exposes per-VM resources/state mapping, wire them here instead of TODO/defaults to keep partition DB authoritative.
166-176: Make polling cancellable, add per-RPC timeout, and reduce log noise.Honor context cancellation, avoid 1.22-only range-over-int, and limit Info spam.
Apply:
- for i := range 300 { - time.Sleep(time.Second) - - w.logger.Info("Polling deployment", "i", i) - resp, err := w.metaldClient.GetDeployment(stepCtx, connect.NewRequest(&metaldv1.GetDeploymentRequest{ + for i := 0; i < 300; i++ { + select { + case <-stepCtx.Done(): + return nil, stepCtx.Err() + case <-time.After(1 * time.Second): + } + w.logger.Debug("polling deployment", "attempt", i+1) + callCtx, cancel := context.WithTimeout(stepCtx, 10*time.Second) + resp, err := w.metaldClient.GetDeployment(callCtx, connect.NewRequest(&metaldv1.GetDeploymentRequest{ DeploymentId: req.DeploymentID, - })) - if err != nil { - w.logger.Error("metald GetDeployment call failed", "error", err, "deployment_id", req.DeploymentID) + })) + cancel() + if err != nil { + w.logger.Error("metald GetDeployment call failed", "error", err, "code", connect.CodeOf(err).String(), "deployment_id", req.DeploymentID) return nil, fmt.Errorf("failed to get deployment: %w", err) }Also applies to: 169-175
370-518: Commented OpenAPI block: remove, gate, or document intent.Dead code drifts. Either feature-flag it, add an AIDEV-NOTE with prerequisites, or drop it.
112-125: Add per-RPC timeout and include connect code on CreateDeployment failures.Prevents hung calls and improves observability.
Apply:
- // Call metald CreateDeployment - resp, err := w.metaldClient.CreateDeployment(stepCtx, connect.NewRequest(&metaldv1.CreateDeploymentRequest{ + // Call metald CreateDeployment + callCtx, cancel := context.WithTimeout(stepCtx, 15*time.Second) + defer cancel() + resp, err := w.metaldClient.CreateDeployment(callCtx, connect.NewRequest(&metaldv1.CreateDeploymentRequest{ @@ - if err != nil { - w.logger.Error("metald CreateDeployment call failed", "error", err, "docker_image", req.DockerImage) + if err != nil { + w.logger.Error("metald CreateDeployment call failed", "error", err, "code", connect.CodeOf(err).String(), "docker_image", req.DockerImage) return nil, fmt.Errorf("failed to create deployment: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
go/apps/ctrl/services/deployment/deploy_workflow.go(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/ctrl/services/deployment/deploy_workflow.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/ctrl/services/deployment/deploy_workflow.go
🧠 Learnings (1)
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/apps/ctrl/services/deployment/deploy_workflow.go
🧬 Code graph analysis (1)
go/apps/ctrl/services/deployment/deploy_workflow.go (6)
go/gen/proto/metald/v1/metaldv1connect/metald.connect.go (1)
VmServiceClient(68-95)go/pkg/hydra/step.go (1)
StepVoid(302-310)go/gen/proto/metald/v1/deployment.pb.go (15)
CreateDeploymentResponse(144-149)CreateDeploymentResponse(162-162)CreateDeploymentResponse(177-179)CreateDeploymentRequest(100-105)CreateDeploymentRequest(118-118)CreateDeploymentRequest(133-135)DeploymentRequest(24-33)DeploymentRequest(46-46)DeploymentRequest(61-63)GetDeploymentResponse_Vm(452-460)GetDeploymentResponse_Vm(473-473)GetDeploymentResponse_Vm(488-490)GetDeploymentRequest(356-361)GetDeploymentRequest(374-374)GetDeploymentRequest(389-391)go/gen/proto/partition/v1/gateway.pb.go (6)
Deployment(104-110)Deployment(123-123)Deployment(138-140)VM(208-213)VM(226-226)VM(241-243)go/pkg/db/models_generated.go (1)
Deployment(527-545)go/pkg/partition/db/vm_upsert.sql_generated.go (1)
UpsertVMParams(24-31)
🪛 golangci-lint (2.2.2)
go/apps/ctrl/services/deployment/deploy_workflow.go
135-135: expected declaration, found ')'
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
go/apps/ctrl/services/deployment/deploy_workflow.go (3)
11-13: Switch to metald v1 client looks correct.Interfaces are preserved; constructor/field types align with generated client.
Also applies to: 28-34
137-138: Fix logging typo/casing.
[ suggest_nitpick ]
Apply:- w.logger.Error("Deployment failed", "error", err, "deployment_id", req.DeploymentID) + w.logger.Error("deployment failed", "error", err, "deployment_id", req.DeploymentID)
231-237: Gateway VM list population looks good.IDs map cleanly; protobuf marshal below handles it.
3 participants
Summary by CodeRabbit
New Features
Improvements
Removed
Documentation