Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
📝 WalkthroughWalkthroughRefactors the dashboard diff UI to accept a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Dashboard User
participant UI as Dashboard UI
participant TRPC as TRPC Router
participant CTRL as Control Plane (OpenAPI Service)
User->>UI: Open Compare View
UI->>TRPC: getOpenApiDiff({fromId, toId})
TRPC->>CTRL: GetOpenApiDiff(old_deployment_id, new_deployment_id)\n(Auth: Bearer CTRL_API_KEY)
CTRL-->>TRPC: { changes[], summary, hasBreakingChanges }
TRPC-->>UI: { changes[], ... }
UI->>UI: Render DiffViewer(changelog=changes)
sequenceDiagram
autonumber
participant Deploy as Deploy Workflow
participant K8s as Cluster
participant App as App Pods
participant DB as Control DB
participant GW as Gateway Service
rect rgba(200,230,255,0.25)
note right of Deploy: OpenAPI scrape (new)
Deploy->>K8s: Create deployment (1 replica, reduced resources)
Deploy->>App: Probe /openapi.yaml (concurrent)
App-->>Deploy: 200 YAML? (optional)
Deploy->>Deploy: Base64-encode spec
Deploy->>DB: Upsert OpenAPI spec
end
rect rgba(220,255,220,0.25)
note right of Deploy: Gateway config (updated)
Deploy->>GW: Upsert gateway config { validation{ openapi_spec (base64) } }
GW->>GW: Init validator (decode base64 → parse)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go/apps/ctrl/services/deployment/deploy_workflow.go (2)
331-391: Error handling bug: earlier domain errors get overwritten
erris reassigned inside the loop and inspected only after the loop, so failures in earlier iterations are dropped if later iterations succeed.- for _, domain := range allDomains { - err = hydra.StepVoid(ctx, fmt.Sprintf("create-domain-entry-%s", domain.domain), func(stepCtx context.Context) error { + for _, domain := range allDomains { + if err := hydra.StepVoid(ctx, fmt.Sprintf("create-domain-entry-%s", domain.domain), func(stepCtx context.Context) error { ... - }) + }); err != nil { + return err + } } - - if err != nil { - return err - }
230-233: Align VM resource metrics with actual deployment resourcesYou deploy with 512m CPU and 512Mi memory, but persist 1000/1024 — skewing capacity/analytics.
- CpuMillicores: 1000, // TODO derive from spec - MemoryMb: 1024, // TODO derive from spec + CpuMillicores: 512, // TODO derive from deployment request/spec + MemoryMb: 512, // TODO derive from deployment request/specAlternatively, plumb these from
deploymentReq.Deploymentto avoid drift.
🧹 Nitpick comments (29)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/page.tsx (1)
33-35: Guard workspace slug before building the route
useParams()can yieldstring | string[] | undefined, so ifworkspaceSlugis ever absent (SSR fallback, malformed URL, tests), we’d push/undefined/.... Fold the slug presence check into the existing guard before routing:- const handleCompare = () => { - if (selectedFromDeployment && selectedToDeployment) { - router.push( - `/${params?.workspaceSlug}/projects/${projectId}/diff/${selectedFromDeployment}/${selectedToDeployment}`, - ); - } - }; + const handleCompare = () => { + const workspaceSlug = params?.workspaceSlug as string | undefined; + if (!workspaceSlug || !selectedFromDeployment || !selectedToDeployment) { + return; + } + router.push( + `/${workspaceSlug}/projects/${projectId}/diff/${selectedFromDeployment}/${selectedToDeployment}`, + ); + };go/apps/krane/backend/kubernetes/eviction.go (1)
5-25: Prefer structured logging over fmt.Sprintf here
k.logger.Infoalready supports key-value pairs (see the rest of this file), so we can drop the extrafmtdependency and keep logs queryable:-import ( - "context" - "fmt" - "time" +import ( + "context" + "time" @@ - k.logger.Info(fmt.Sprintf("Krane setup to auto-evict deployments after %s", ttl.String())) + k.logger.Info("Krane setup to auto-evict deployments", "ttl", ttl.String())go/k8s/manifests/dashboard.yaml (1)
55-66: Prefer a cheap health endpoint for probes.Probing “/” can be expensive for SSR apps and may flap under load. Expose a lightweight “/healthz” (or similar) and point both probes there.
go/apps/gw/services/validation/validator.go (4)
114-116: Cache key risks staleness; include spec hash.If a deployment’s spec is upserted post-deploy, keying only by deploymentID can serve a stale validator. Include a content hash.
- // Create cache key based on deployment ID (spec content could be large) - cacheKey := fmt.Sprintf("validator:%s", deploymentID) + // Include spec hash to avoid stale validators after upserts + sum := sha256.Sum256([]byte(spec)) + cacheKey := fmt.Sprintf("validator:%s:%x", deploymentID, sum)And add:
import ( "context" "encoding/base64" "fmt" + "crypto/sha256" "net/http"
131-137: Base64 decoding: be tolerant to whitespace/padding.Trim whitespace and optionally fall back to RawStdEncoding for specs without padding.
- b, err := base64.StdEncoding.DecodeString(spec) + spec = strings.TrimSpace(spec) + b, err := base64.StdEncoding.DecodeString(spec) + if err != nil { + if rb, rerr := base64.RawStdEncoding.DecodeString(spec); rerr == nil { + b, err = rb, nil + } + } if err != nil { return nil, fault.Wrap(err, fault.Internal("failed to parse OpenAPI document as base64"), ) }Also import:
- "fmt" + "fmt" + "strings"
79-93: *Avoid shadowing the imported errors package and drop redundant (v).Rename local variable and call method on the pointer directly for clarity.
- valid, errors := (*v).ValidateHttpRequest(req) + valid, valErrs := v.ValidateHttpRequest(req) ... - validationErr := s.buildValidationError(errors) + validationErr := s.buildValidationError(valErrs)
126-131: Reduce duplicate validator builds under concurrency (optional).Two simultaneous cache misses for the same key will both build validators. Consider singleflight for creation.
I can wire a singleflight.Group into Service if you want.
apps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.ts (1)
75-81: Optional: map control-plane errors to user-facing codes.Currently all failures become 500. If ctrl returns “not found/invalid argument”, consider mapping to NOT_FOUND/BAD_REQUEST to improve UX.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/components/client.tsx (6)
69-70: Normalize HTTP methods to uppercase for filters and colors.Operation case may vary; normalize to avoid mismatches.
- const operations = [...new Set(changelog.map((c) => c.operation))]; + const operations = [...new Set(changelog.map((c) => c.operation.toUpperCase()))];- if (filters.operation !== "all" && change.operation !== filters.operation) { + if (filters.operation !== "all" && change.operation.toUpperCase() !== filters.operation) { return false; }- const getOperationColor = (operation: string) => { + const getOperationColor = (operation: string) => { + const key = operation.toUpperCase(); const colors: Record<string, string> = { GET: "bg-gray-100 text-gray-800 border border-gray-200", POST: "bg-brand text-brand-foreground border border-gray-200", PUT: "bg-amber-2 text-amber-11 border border-gray-200", PATCH: "bg-gray-200 text-gray-700 border border-gray-300", DELETE: "bg-red-2 text-red-11 border border-gray-200", }; - return colors[operation] || "bg-gray-100 text-gray-800 border border-gray-200"; + return colors[key] || "bg-gray-100 text-gray-800 border border-gray-200"; };No UI change, but filters and badges will be consistent across data sources. Based on learnings.
Also applies to: 81-83, 146-155, 529-533, 864-867, 896-901
552-555: Remove duplicated change text.The timeline and change item both repeat change.text.
- {change.text && ( - <p className="text-xs text-content-subtle italic bg-background p-2 rounded"> - {change.text} - </p> - )}- {change.text && ( - <p className="text-xs text-content-subtle italic bg-background/70 p-2 rounded border-l-2 border-gray-300"> - 💡 {change.text} - </p> - )}Also applies to: 936-940
559-560: Fix “Section” label; show operationId when available.There’s no section field now; display operationId instead.
- ID: {change.id} • Section: {change.id} + ID: {change.id} + {change.operationId ? ` • Operation: ${change.operationId}` : ""}- <span>Section: {change.id}</span> + <span>Operation: {change.operationId ?? "—"}</span>Also applies to: 946-947
52-54: Default expanded paths to none (POC defaults leak UI bias).Starting expanded for “/users” endpoints can be confusing for other specs.
- const [expandedPaths, setExpandedPaths] = useState<Set<string>>( - new Set(["/users", "/users/{userId}"]), - ); + const [expandedPaths, setExpandedPaths] = useState<Set<string>>(new Set());If you want a sensible default, expand the first N paths from stats.paths. Based on learnings.
157-168: Icon heuristic should not rely on id contents.ids aren’t semantic. Derive icons from text (or a backend-provided kind) instead.
Minimal change: switch getChangeIcon() to check change.text and update its call sites.
- const getChangeIcon = (changeId: string) => { - if (changeId.includes("added") || changeId.includes("new")) { + const getChangeIcon = (t: string) => { + const s = t.toLowerCase(); + if (s.includes("added") || s.includes("new")) { return <Plus className="w-4 h-4 text-gray-600" />; } - if (changeId.includes("removed") || changeId.includes("deleted")) { + if (s.includes("removed") || s.includes("deleted")) { return <Minus className="w-4 h-4 text-alert" />; } - if (changeId.includes("changed") || changeId.includes("modified")) { + if (s.includes("changed") || s.includes("modified")) { return <Edit className="w-4 h-4 text-brand" />; } return <Info className="w-4 h-4 text-gray-600" />; };- {getChangeIcon(change.id)} + {getChangeIcon(change.text)}- {getChangeIcon(change.id)} + {getChangeIcon(change.text)}Longer-term: have the control plane include a “kind” enum for each entry. Based on learnings.
Also applies to: 549-550, 929-931
38-46: Empty-state copy could be clearer (optional).Consider “No changes found for the selected deployments” and show a CTA to pick different deployments.
Also applies to: 455-470
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/page.tsx (1)
309-309: Consider defaulting to an empty changelog array.Because the count above already guards with optional chaining, the control-plane response can leave
diffData.changesundefined. Normalizing to[]here keeps DiffViewer’s downstream memoization logic working without extra null checks.- <DiffViewer changelog={diffData.changes} /> + <DiffViewer changelog={diffData.changes ?? []} />go/apps/ctrl/services/openapi/get_diff.go (2)
16-27: Backward‑compat shim for request IDs (if you re‑add deprecated fields)If you reintroduce deprecated
old_version_id/new_version_idin the proto for JSON compatibility, prefer reading either field here to avoid breaking older clients.- // Load old version spec - oldSpec, err := s.loadOpenApiSpec(ctx, req.Msg.OldDeploymentId) + // Load old version spec + oldID := req.Msg.OldDeploymentId + if oldID == "" { oldID = req.Msg.OldVersionId } // deprecated field fallback + oldSpec, err := s.loadOpenApiSpec(ctx, oldID) ... - // Load new version spec - newSpec, err := s.loadOpenApiSpec(ctx, req.Msg.NewDeploymentId) + // Load new version spec + newID := req.Msg.NewDeploymentId + if newID == "" { newID = req.Msg.NewVersionId } // deprecated field fallback + newSpec, err := s.loadOpenApiSpec(ctx, newID)
38-45: Harden base64 decoding for OpenAPI specs
Trim whitespace, strip an optionaldata:...;base64,prefix, and explicitly error on empty payload before loading inget_diff.go. Replace direct calls tobase64.StdEncoding.DecodeStringfor botholdSpecandnewSpecwith:- b1, err := base64.StdEncoding.DecodeString(oldSpec) + b1, err := decodeBase64Spec(oldSpec) if err != nil { // existing error handling } + if len(b1) == 0 { + return nil, connect.NewError(connect.CodeInvalidArgument, fault.Wrap(fmt.Errorf("empty spec"), + fault.Internal("old version spec is empty after decoding"), + fault.Public("Empty OpenAPI specification in old version"), + )) + } s1, err := loader.LoadFromData(b1)(and the same pattern for
newSpec)Add this helper:
import "strings" func decodeBase64Spec(spec string) ([]byte, error) { s := strings.TrimSpace(spec) if strings.HasPrefix(s, "data:") { if idx := strings.LastIndex(s, ","); idx >= 0 { s = s[idx+1:] } } return base64.StdEncoding.DecodeString(s) }go/proto/ctrl/v1/openapi.proto (1)
12-19: Model severity as an enum for clarity and type‑safety
int32 levelis opaque. Define an enum (e.g., CHANGE_LEVEL_INFO/WARN/ERR) and use it here. It improves readability across languages and prevents invalid values.+enum ChangeLevel { + CHANGE_LEVEL_UNSPECIFIED = 0; + CHANGE_LEVEL_INFO = 1; + CHANGE_LEVEL_WARN = 2; + CHANGE_LEVEL_ERR = 3; +} message ChangelogEntry { string id = 1; string text = 2; - int32 level = 3; + ChangeLevel level = 3; string operation = 4; optional string operation_id = 5; string path = 6; }go/pkg/cli/flag_test.go (2)
33-40: Use t.Setenv to avoid cross‑test env leakageReplace manual
os.Setenv+defer Unsetenvwitht.Setenv, which is cleaner and automatically restores state, especially important if tests become parallelized.Example:
- os.Setenv("TEST_DURATION", "2h30m") - defer os.Unsetenv("TEST_DURATION") + t.Setenv("TEST_DURATION", "2h30m")Also applies to: 43-59, 71-87, 131-138, 155-161, 164-187, 220-227, 230-238, 241-256, 288-296, 324-338, 539-546
198-203: Prefer using ErrValidationFailed in TestDurationFlag_InvalidDuration instead of a hard-coded string
Replace the literal"validation failed"withErrValidationFailed.Error()for consistency with other validation-error tests.
Optionally, if you’d rather align parse errors with theErrInvalid*Valuepattern, introduce anErrInvalidDurationValueconstant and haveDurationFlag.Parsereturn it (updating tests accordingly).go/apps/ctrl/services/openapi/utils.go (1)
18-24: Treat empty string spec as missingA Valid=true but empty string yields base64 decode to empty and later parse error. Fail early here with a clearer error.
- if !deployment.OpenapiSpec.Valid { + if !deployment.OpenapiSpec.Valid || strings.TrimSpace(deployment.OpenapiSpec.String) == "" { return "", fault.New("deployment has no OpenAPI spec stored", fault.Public("OpenAPI specification not available for this deployment"), ) }Add import:
import ( "context" + "strings" ... )go/cmd/krane/main.go (1)
38-39: Clarify that 0 disables eviction in help textMake the flag behavior explicit for operators.
- cli.Duration("deployment-eviction-ttl", "Automatically delete deployments after some time. Use go duration formats such as 2h30m", cli.EnvVar("UNKEY_DEPLOYMENT_EVICTION_TTL")), + cli.Duration("deployment-eviction-ttl", "Automatically delete deployments after some time (0 disables). Use Go duration formats, e.g. 2h30m", cli.EnvVar("UNKEY_DEPLOYMENT_EVICTION_TTL")),go/apps/ctrl/services/deployment/deploy_workflow.go (3)
202-205: Avoid logging full response message as “status”
"status", resp.Msglikely dumps the whole proto. Prefer a concise field (e.g., instance counts) to keep logs structured.- w.logger.Info("deployment status", - "deployment_id", req.DeploymentID, - "status", resp.Msg, - ) + w.logger.Info("deployment poll", + "deployment_id", req.DeploymentID, + "instances", len(resp.Msg.GetInstances()), + )
408-434: Reuse helper to avoid duplicated gateway config constructionThis block duplicates
createGatewayConfig()below. Use the helper and set ValidationConfig whenopenapiSpecexists to keep one source of truth.- // Create VM protobuf objects for gateway config - gatewayConfig := &partitionv1.GatewayConfig{ - Deployment: &partitionv1.Deployment{ - Id: req.DeploymentID, - IsEnabled: true, - }, - Vms: make([]*partitionv1.VM, len(createdInstances)), - } - - for i, vm := range createdInstances { - gatewayConfig.Vms[i] = &partitionv1.VM{ - Id: vm.Id, - } - } - - // Only add AuthConfig if we have a KeyspaceID - if req.KeyspaceID != "" { - gatewayConfig.AuthConfig = &partitionv1.AuthConfig{ - KeyAuthId: req.KeyspaceID, - } - } + gatewayConfig, _ := createGatewayConfig(req.DeploymentID, req.KeyspaceID, createdInstances) if openapiSpec != "" { gatewayConfig.ValidationConfig = &partitionv1.ValidationConfig{ OpenapiSpec: openapiSpec, } }
26-26: Namespace TODOHardcoding
unkeywill bite multitenancy/workspace scoping later. Consider deriving namespace fromworkspace(or config) soon.go/pkg/cli/flag.go (4)
90-105: Unify DurationFlag parse/validate order
Parse()validates before parsing, but env-var handling parses first; other numeric flags (int/float/bool) also parse first. For consistency and clearer error precedence, parse first, then validate.func (f *DurationFlag) Parse(value string) error { - // Run validation if provided - if f.validate != nil { - if err := f.validate(value); err != nil { - return newValidationError(f.name, err) - } - } - var err error - f.value, err = time.ParseDuration(value) - if err != nil { - return newValidationError(f.name, err) - } + parsed, err := time.ParseDuration(value) + if err != nil { + return newValidationError(f.name, err) + } + // Run validation if provided (on the original string, like other flags) + if f.validate != nil { + if err := f.validate(value); err != nil { + return newValidationError(f.name, err) + } + } + f.value = parsed f.set = true return nil }
107-112: Comment nit: “string value” → “duration value”The receiver returns
time.Duration. Update the comment for clarity.-// Value returns the current string value +// Value returns the current duration value
453-463: Comment nit: default text
time.Duration(0)comment says “Default to false”. Suggest “Default to 0”.- value: time.Duration(0), // Default to false + value: time.Duration(0), // Default to 0
363-410: Ergonomics: consider string defaults for DurationAs a nicety, allow
Default("500ms")for*DurationFlagby parsing strings when provided.Example pattern:
case *DurationFlag: switch v := value.(type) { case time.Duration: flag.value = v case string: if d, err := time.ParseDuration(v); err == nil { flag.value = d } else { err = fmt.Errorf("default value for duration flag '%s' invalid: %v", flag.name, err) } default: err = fmt.Errorf("default value for duration flag '%s' must be time.Duration or string, got %T", flag.name, value) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go/gen/proto/ctrl/v1/openapi.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/partition/v1/gateway.pb.gois excluded by!**/*.pb.go,!**/gen/**internal/proto/generated/ctrl/v1/openapi_pb.tsis excluded by!**/generated/**internal/proto/generated/partition/v1/gateway_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (24)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/components/client.tsx(20 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/constants.ts(0 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/page.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/types.ts(0 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/page.tsx(1 hunks)apps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.ts(2 hunks)apps/dashboard/lib/trpc/routers/deploy/deployment/promote.ts(2 hunks)apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts(1 hunks)go/apps/ctrl/services/deployment/deploy_workflow.go(5 hunks)go/apps/ctrl/services/openapi/get_diff.go(4 hunks)go/apps/ctrl/services/openapi/utils.go(1 hunks)go/apps/gw/services/validation/validator.go(3 hunks)go/apps/krane/backend/kubernetes/eviction.go(2 hunks)go/apps/krane/backend/kubernetes/get_deployment.go(0 hunks)go/cmd/krane/main.go(1 hunks)go/k8s/manifests/ctrl.yaml(1 hunks)go/k8s/manifests/dashboard.yaml(1 hunks)go/k8s/manifests/krane.yaml(1 hunks)go/pkg/cli/flag.go(6 hunks)go/pkg/cli/flag_test.go(2 hunks)go/pkg/cli/help.go(2 hunks)go/proto/ctrl/v1/openapi.proto(1 hunks)go/proto/partition/v1/gateway.proto(1 hunks)internal/proto/src/index.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/types.ts
- go/apps/krane/backend/kubernetes/get_deployment.go
- apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/constants.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: mcstepp
PR: unkeyed/unkey#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.
📚 Learning: 2025-09-01T16:08:10.327Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/deployment.proto:45-52
Timestamp: 2025-09-01T16:08:10.327Z
Learning: In the unkey/unkey repository metald service, the host:port pairs in Instance messages within GetDeploymentResponse are consumed by upstream services in a tightly controlled manner, with the host:port being mapped from DNS entries rather than being used for arbitrary client connections.
Applied to files:
go/k8s/manifests/krane.yaml
📚 Learning: 2025-09-25T18:49:11.425Z
Learnt from: mcstepp
PR: unkeyed/unkey#4010
File: apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts:39-44
Timestamp: 2025-09-25T18:49:11.425Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts and similar files, mcstepp prefers to keep the demo API key authentication simple without additional validation complexity, since it's temporary code that will be replaced after the demo phase.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/deployment/promote.tsapps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.tsapps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts
📚 Learning: 2025-09-12T18:11:33.481Z
Learnt from: mcstepp
PR: unkeyed/unkey#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:
apps/dashboard/lib/trpc/routers/deploy/deployment/promote.tsapps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.tsapps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
PR: unkeyed/unkey#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:
apps/dashboard/lib/trpc/routers/deploy/deployment/promote.tsapps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.tsapps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
PR: unkeyed/unkey#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:
apps/dashboard/lib/trpc/routers/deploy/deployment/promote.tsapps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.ts
📚 Learning: 2025-09-24T18:57:34.843Z
Learnt from: mcstepp
PR: unkeyed/unkey#4010
File: QUICKSTART-DEPLOY.md:17-17
Timestamp: 2025-09-24T18:57:34.843Z
Learning: In the Unkey deployment platform, API key environment variables use component-specific naming but share the same secret value: UNKEY_API_KEY for the ctrl service (validator), API_KEY for the CLI client, and CTRL_API_KEY for the dashboard client. The ctrl service acts as the source of truth for validation.
Applied to files:
go/k8s/manifests/ctrl.yaml
📚 Learning: 2025-09-16T19:08:44.174Z
Learnt from: Flo4604
PR: unkeyed/unkey#3980
File: go/k8s/manifests/dashboard.yaml:41-42
Timestamp: 2025-09-16T19:08:44.174Z
Learning: For local development Kubernetes manifests (typically indicated by paths containing "local" or environment variables like UNKEY_REGION: "local"), hardcoded generic credentials in environment variables are acceptable for convenience. Security recommendations about using Secrets should be reserved for production or staging environments.
Applied to files:
go/k8s/manifests/ctrl.yaml
📚 Learning: 2025-07-28T20:38:53.244Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx:322-341
Timestamp: 2025-07-28T20:38:53.244Z
Learning: In apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx, mcstepp prefers to keep hardcoded endpoint logic in the getDiffType function during POC phases for demonstrating diff functionality, rather than implementing a generic diff algorithm. This follows the pattern of keeping simplified implementations for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/page.tsxapps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/components/client.tsxapps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/page.tsx
📚 Learning: 2025-07-15T14:45:18.920Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3564
File: go/cmd/cli/commands/deploy/flags.go:17-20
Timestamp: 2025-07-15T14:45:18.920Z
Learning: In the go/cmd/cli/commands/deploy/ directory, ogzhanolguncu prefers to keep potentially temporary features (like UNKEY_DOCKER_REGISTRY environment variable) undocumented in help text if they might be deleted in the future, to avoid documentation churn.
Applied to files:
go/cmd/krane/main.go
🧬 Code graph analysis (14)
go/pkg/cli/help.go (1)
go/pkg/cli/flag.go (2)
DurationFlag(84-88)EnvVar(320-339)
go/apps/ctrl/services/openapi/get_diff.go (1)
go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
apps/dashboard/lib/trpc/routers/deploy/deployment/promote.ts (1)
apps/dashboard/lib/env.ts (1)
env(3-52)
go/apps/gw/services/validation/validator.go (1)
go/pkg/fault/wrap.go (2)
Wrap(25-67)Internal(75-89)
go/pkg/cli/flag_test.go (2)
go/pkg/cli/flag.go (4)
Duration(454-490)Default(364-416)EnvVar(320-339)Flag(20-27)go/pkg/cli/command.go (1)
Command(24-40)
apps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.ts (3)
apps/dashboard/lib/env.ts (1)
env(3-52)internal/proto/generated/ctrl/v1/openapi_pb.ts (1)
OpenApiService(194-203)internal/db/src/schema/deployments.ts (1)
deployments(9-56)
apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts (2)
apps/dashboard/lib/env.ts (1)
env(3-52)internal/proto/generated/ctrl/v1/deployment_pb.ts (1)
DeploymentService(569-610)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/components/client.tsx (1)
internal/proto/generated/ctrl/v1/openapi_pb.ts (1)
ChangelogEntry(44-74)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/page.tsx (1)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/diff/[...compare]/components/client.tsx (1)
DiffViewer(32-986)
go/apps/ctrl/services/openapi/utils.go (1)
go/apps/ctrl/services/openapi/service.go (1)
Service(9-13)
go/apps/krane/backend/kubernetes/eviction.go (1)
go/pkg/repeat/every.go (1)
Every(11-37)
go/pkg/cli/flag.go (1)
go/pkg/cli/command.go (1)
Exit(325-329)
go/cmd/krane/main.go (1)
go/pkg/cli/flag.go (2)
Duration(454-490)EnvVar(320-339)
go/apps/ctrl/services/deployment/deploy_workflow.go (4)
go/pkg/hydra/step.go (2)
Step(67-277)StepVoid(303-311)go/pkg/db/deployment_update_openapi_spec.sql_generated.go (1)
UpdateDeploymentOpenapiSpecParams(19-23)go/gen/proto/partition/v1/gateway.pb.go (15)
GatewayConfig(26-37)GatewayConfig(50-50)GatewayConfig(65-67)Deployment(104-110)Deployment(123-123)Deployment(138-140)VM(208-213)VM(226-226)VM(241-243)AuthConfig(253-258)AuthConfig(271-271)AuthConfig(286-288)ValidationConfig(298-304)ValidationConfig(317-317)ValidationConfig(332-334)go/pkg/db/models_generated.go (1)
Deployment(569-587)
🪛 Checkov (3.2.334)
go/k8s/manifests/dashboard.yaml
[medium] 42-43: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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 API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (10)
go/proto/partition/v1/gateway.proto (1)
45-46: Documentation clarification appreciatedCalling out that
openapi_specexpects a Base64-encoded document (or empty string) aligns the proto with the new control plane contract. Thanks for tightening the docs.apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts (1)
25-41: Control plane auth precondition looks solidFailing fast when either
CTRL_URLorCTRL_API_KEYis missing and injecting the Bearer header in one interceptor keeps the temporary demo flow simple while tightening auth. Nice improvement.apps/dashboard/lib/trpc/routers/deploy/deployment/promote.ts (1)
25-43: Promotion endpoint now aligns with control-plane authConsistently requiring both env vars and applying the Bearer interceptor keeps the promote flow in lockstep with rollback—good hardening of the control-plane calls.
go/k8s/manifests/dashboard.yaml (2)
20-29: Verify busybox has nc -z, or switch readiness gating.busybox images can vary; nc may be missing or lack -z. Consider wget/curl or rely on app readiness instead.
33-33: Dev-only settings may bite in clusters.
- imagePullPolicy: Never will fail outside local clusters. Gate via dev overlay.
- Service type: LoadBalancer can incur cloud cost; ensure this is intended (or use ClusterIP + Ingress for dev).
Would you like a kustomize overlay diff to scope these to dev?
Also applies to: 69-85
apps/dashboard/lib/trpc/routers/deploy/deployment/getOpenApiDiff.ts (1)
44-74: Input validation + workspace scoping LGTM.Batch fetch + same-project check before calling control plane looks solid.
Confirm proto field names match TS client (oldDeploymentId/newDeploymentId).
go/apps/ctrl/services/openapi/get_diff.go (1)
48-52: LGTM: clear error mapping and response assemblyGood use of connect error codes, internal/public messages, and diff+checker pipeline. No action needed.
Consider unit tests for: invalid base64; invalid OpenAPI; diff with breaking vs non‑breaking to validate
HasBreakingChanges.Also applies to: 64-68, 71-77, 96-101
go/pkg/cli/flag_test.go (1)
189-197: Duration flag tests look solidCovers parse, default, zero value, and command integration. No change required.
Also applies to: 205-211, 212-217, 258-271
go/pkg/cli/flag.go (1)
320-338: LGTM: EnvVar now supports DurationFlagHandling mirrors other types and parses with
time.ParseDuration.go/apps/ctrl/services/deployment/deploy_workflow.go (1)
188-194: No changes needed
The rootgo/go.modspecifies Go 1.25, and other modules use Go 1.23–1.25, all ≥ 1.22, sofor i := range 300is fully supported.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
@Flo4604 :) |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (09/30/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
https://app.cushion.so/unkey/post/post_qMsRR1fEzWRN6dAjAg1n7
Refactors the OpenAPI diff viewer to use the new
ChangelogEntrytype from the proto package instead of the customDiffChangetype. This change integrates the frontend with the backend API for OpenAPI diff functionality, allowing users to compare API changes between deployments.Key changes:
DiffViewercomponent to acceptchangelog: ChangelogEntry[]instead of the customdiffDatatypeType of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin main