feat: slimmer CLI flags with sensible defaults#4056
Conversation
mcstepp
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
go/cmd/deploy/control_plane.go (1)
56-68: Avoid sending an “empty but present” ApiId (use nil when absent).Setting ApiId to &c.opts.ApiID sends presence=true even when empty, which can be treated as invalid by the server. Only set the pointer when non-empty.
- createReq := connect.NewRequest(&ctrlv1.CreateDeploymentRequest{ - ProjectId: c.opts.ProjectID, - ApiId: &c.opts.ApiID, + var apiID *string + if c.opts.ApiID != "" { + apiID = &c.opts.ApiID + } + createReq := connect.NewRequest(&ctrlv1.CreateDeploymentRequest{ + ProjectId: c.opts.ProjectID, + ApiId: apiID, Branch: c.opts.Branch, SourceType: ctrlv1.SourceType_SOURCE_TYPE_CLI_UPLOAD, EnvironmentSlug: c.opts.Environment, DockerImage: dockerImage, GitCommitSha: commitInfo.CommitSHA, GitCommitMessage: commitInfo.Message, GitCommitAuthorHandle: commitInfo.AuthorHandle, GitCommitAuthorAvatarUrl: commitInfo.AuthorAvatarURL, GitCommitTimestamp: commitInfo.CommitTimestamp, })go/apps/ctrl/services/deployment/deploy_workflow.go (1)
666-690: Remove unused createGatewayConfig function
This helper isn’t referenced anywhere; delete it to eliminate dead code drift.go/pkg/codes/constants_gen.go (1)
31-35: Refine URN mapping for connect.CodeUnauthenticated
- In
control_plane.go:251, theconnect.CodeUnauthenticatedbranch always usesUnkeyAuthErrorsAuthenticationMalformed, even for valid-but-invalid credentials.- Restrict
AuthenticationMalformedto actual format errors.- Map missing credentials to
UnkeyAuthErrorsAuthenticationMissingor introduce a genericAuthenticationFailedURN for other auth failures.go/apps/ctrl/services/deployment/create_deployment.go (1)
90-117: Use sanitized values to set sql.NullString.Valid; trim commit message whitespace.Currently Valid derives from the raw request, which can produce Valid=true with an empty sanitized string. Also trim commit message spaces to avoid persisting whitespace-only values.
Apply this diff:
- gitCommitSha := req.Msg.GetGitCommitSha() - gitCommitMessage := trimLength(req.Msg.GetGitCommitMessage(), 10240) - gitCommitAuthorHandle := trimLength(strings.TrimSpace(req.Msg.GetGitCommitAuthorHandle()), 256) - gitCommitAuthorAvatarUrl := trimLength(strings.TrimSpace(req.Msg.GetGitCommitAuthorAvatarUrl()), 512) + gitCommitSha := req.Msg.GetGitCommitSha() + gitCommitMessage := trimLength(strings.TrimSpace(req.Msg.GetGitCommitMessage()), 10240) + gitCommitAuthorHandle := trimLength(strings.TrimSpace(req.Msg.GetGitCommitAuthorHandle()), 256) + gitCommitAuthorAvatarUrl := trimLength(strings.TrimSpace(req.Msg.GetGitCommitAuthorAvatarUrl()), 512) @@ - GitCommitMessage: sql.NullString{String: gitCommitMessage, Valid: req.Msg.GetGitCommitMessage() != ""}, - GitCommitAuthorHandle: sql.NullString{String: gitCommitAuthorHandle, Valid: req.Msg.GetGitCommitAuthorHandle() != ""}, - GitCommitAuthorAvatarUrl: sql.NullString{String: gitCommitAuthorAvatarUrl, Valid: req.Msg.GetGitCommitAuthorAvatarUrl() != ""}, + GitCommitMessage: sql.NullString{String: gitCommitMessage, Valid: gitCommitMessage != ""}, + GitCommitAuthorHandle: sql.NullString{String: gitCommitAuthorHandle, Valid: gitCommitAuthorHandle != ""}, + GitCommitAuthorAvatarUrl: sql.NullString{String: gitCommitAuthorAvatarUrl, Valid: gitCommitAuthorAvatarUrl != ""},
🧹 Nitpick comments (4)
go/pkg/db/bulk_deployment_insert.sql_generated.go (1)
50-51: Keep gofmt-compliant indentation.These lines now use spaces instead of the gofmt-standard tabs. Please re-run gofmt (or restore tabs) so the file stays consistent with Go formatting.
- _, err := db.ExecContext(ctx, bulkQuery, allArgs...) - return err + _, err := db.ExecContext(ctx, bulkQuery, allArgs...) + return errgo/apps/ctrl/services/deployment/create_deployment_simple_test.go (1)
410-416: Prefer a shared test constant for the hardcoded workspace ID.Improves clarity and avoids typos across tests.
+const testWorkspaceID = "ws_test123" @@ - WorkspaceID: "ws_test123", // In production, this is inferred from ProjectID via DB lookup, just hardcoded for the test + WorkspaceID: testWorkspaceID, // In production, inferred from ProjectID; hardcoded for the testgo/cmd/deploy/main.go (1)
108-109: Clarify api-id flag description.Slightly tweak wording to avoid confusion with API keys.
Apply this diff:
- cli.String("api-id", "API ID for API key authentication", cli.EnvVar(EnvApiID)), + cli.String("api-id", "Target API ID (optional)", cli.EnvVar(EnvApiID)),go/cmd/deploy/config.go (1)
81-90: Consider including api_id in generated config for discoverability.Even if optional, adding api_id (empty) helps users know it exists.
Apply this diff:
- config := &Config{ - ProjectID: projectID, - Context: context, - } + config := &Config{ + ApiID: "", + ProjectID: projectID, + Context: context, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go/gen/proto/ctrl/v1/ctrlv1connect/deployment.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**internal/proto/generated/ctrl/v1/deployment_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (35)
go/apps/ctrl/services/deployment/create_deployment.go(3 hunks)go/apps/ctrl/services/deployment/create_deployment_simple_test.go(1 hunks)go/apps/ctrl/services/deployment/deploy_workflow.go(2 hunks)go/cmd/deploy/config.go(3 hunks)go/cmd/deploy/control_plane.go(1 hunks)go/cmd/deploy/init.go(2 hunks)go/cmd/deploy/main.go(5 hunks)go/pkg/codes/constants_gen.go(5 hunks)go/pkg/db/bulk_acme_challenge_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_acme_user_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_api_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_audit_log_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_audit_log_target_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_deployment_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_deployment_step_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_domain_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_identity_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_identity_insert_ratelimit.sql_generated.go(1 hunks)go/pkg/db/bulk_key_encryption_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_key_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_key_insert_ratelimit.sql_generated.go(1 hunks)go/pkg/db/bulk_key_permission_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_key_role_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_keyring_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_permission_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_project_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_ratelimit_namespace_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_ratelimit_override_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_role_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_role_permission_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_workspace_insert.sql_generated.go(1 hunks)go/pkg/partition/db/bulk_certificate_insert.sql_generated.go(1 hunks)go/pkg/partition/db/bulk_gateway_upsert.sql_generated.go(1 hunks)go/pkg/partition/db/bulk_vm_upsert.sql_generated.go(1 hunks)go/proto/ctrl/v1/deployment.proto(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
PR: unkeyed/unkey#3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/bulk_key_insert_ratelimit.sql_generated.gogo/pkg/db/bulk_keyring_insert.sql_generated.gogo/pkg/db/bulk_api_insert.sql_generated.gogo/pkg/db/bulk_key_insert.sql_generated.gogo/pkg/db/bulk_key_permission_insert.sql_generated.go
🧬 Code graph analysis (4)
go/apps/ctrl/services/deployment/create_deployment.go (3)
go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/db/environment_find_by_project_id_and_slug.sql_generated.go (1)
FindEnvironmentByProjectIdAndSlugParams(21-25)go/apps/ctrl/services/deployment/deploy_workflow.go (1)
DeployRequest(62-69)
go/cmd/deploy/main.go (1)
go/pkg/cli/flag.go (4)
String(419-451)EnvVar(320-339)Default(364-416)Bool(493-529)
go/apps/ctrl/services/deployment/deploy_workflow.go (2)
internal/proto/generated/partition/v1/gateway_pb.ts (1)
AuthConfig(130-135)go/gen/proto/partition/v1/gateway.pb.go (3)
AuthConfig(253-258)AuthConfig(271-271)AuthConfig(286-288)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-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: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
🔇 Additional comments (18)
go/pkg/db/bulk_acme_user_insert.sql_generated.go (1)
38-39: No issues found.Indentation change only; behavior is unchanged.
go/pkg/db/bulk_permission_insert.sql_generated.go (1)
41-42: No issues spotted.Pure whitespace tweak; behavior unchanged.
go/pkg/db/bulk_api_insert.sql_generated.go (1)
42-43: Formatting tweak looks good.Indentation change preserves behavior; no further action needed.
go/pkg/db/bulk_workspace_insert.sql_generated.go (1)
40-41: Indentation tweak looks good.Whitespace-only adjustment, no behavioral change. Thanks for keeping the generated file consistent.
go/pkg/db/bulk_key_insert.sql_generated.go (1)
50-51: Whitespace-only tweak is fineIndentation change keeps the generated file consistent; no functional impact detected.
go/pkg/db/bulk_domain_insert.sql_generated.go (1)
44-45: Indentation tweak looks harmless.No behavioral impact here.
go/pkg/db/bulk_audit_log_target_insert.sql_generated.go (1)
45-46: No functional change observed.Indentation tweak only; bulk insert logic remains intact.
go/pkg/db/bulk_identity_insert_ratelimit.sql_generated.go (1)
48-49: Formatting-only change looks good.Indentation update is a no-op; query and args remain identical.
go/apps/ctrl/services/deployment/deploy_workflow.go (2)
62-69: ApiID added to DeployRequest: LGTM.Field name/tag align with downstream usage.
423-427: AuthConfig is conditionally set based on ApiID: LGTM.Mapping ApiID to AuthConfig.KeyAuthId is correct and preserves previous behavior when empty.
go/cmd/deploy/init.go (1)
59-62: Signature update looks correct.createConfigWithValues now matches the new (configDir, projectID, context) signature.
go/apps/ctrl/services/deployment/create_deployment.go (3)
29-40: Workspace inference from project looks good.Project lookup with proper NotFound mapping and deriving workspaceID from the project is correct.
41-51: Environment lookup scoped by inferred workspace is appropriate.Using both workspaceID and projectID for the environment query and returning NotFound is sound.
130-138: ApiID propagation to workflow request is correct.DeployRequest now includes ApiID and inferred workspace; matches the new flow.
go/cmd/deploy/main.go (1)
100-126: Flags and defaults look consistent with the new model.
- api-id flag with UNKEY_API_ID env is good.
- context default "." and env default "preview" align with goals.
- linux default true is reasonable for cloud deploys.
go/cmd/deploy/config.go (1)
114-136: mergeWithFlags logic is sound.Flag precedence over file values and defaulting context to "." are correct.
go/proto/ctrl/v1/deployment.proto (2)
141-150: New GetProject messages look fine.Schema is straightforward; aligns with workspace inference needs.
165-167: GetProject RPC addition looks good.Complements CreateDeployment’s workspace inference path.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
looks good overall, just small stuff
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/07/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
4 participants
What does this PR do?
This PR simplifies the
unkey deploycommand by automatically inferring the workspace ID from the project ID on the server side, eliminating the need for users to specify it.Changes:
Protocol Buffers:
Server-side (ctrl service):
CreateDeploymentto look up project byproject_idand extractworkspace_idfrom itCLI:
--workspace-idflag andUNKEY_WORKSPACE_IDenvironment variableWorkspaceIDfromConfigstruct (no longer inunkey.json)WorkspaceIDfromDeployOptionsstructcontrol_plane.goto not sendworkspace_idin deployment requests--contextdefaults to.(current directory) and--linuxdefaults totrueTests:
create_deployment_simple_test.goto removeWorkspaceIdfrom test requestsUser Impact:
Users can now deploy with just:
Instead of:
The workspace is automatically determined from the project on the server side.
How should this be tested?
Prerequisites:
Set up test project:
Create a workspace and project
Note the project_id for testing
Build and Install:
Test Scenarios
Expected unkey.json:
Expected: Deployment succeeds using project_id from config and API_KEY from env, workspace inferred automatically
Expected: Deployment succeeds without requiring --workspace-id
Expected: Error message "project not found: invalid_project"
Check ctrl service logs to confirm workspace ID is correctly inferred from project
Verify deployment is created under the correct workspace in the database
Run Tests:
Breaking Changes
Migration path:
Remove --workspace-id flag from any scripts/CI pipelines
Remove workspace_id from unkey.json config files (optional, but recommended)
Update any code calling the CreateDeployment RPC endpoint
Type of change
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated