feat: generate proper wildcard domains#3987
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughPre-fetches workspace, project, environment, and deployment in the deploy workflow; replaces Git-based hostname logic with a slug-based buildDomains utility and creates wildcard domain entries; disables OpenAPI scraping; adds FindEnvironmentById SQL and generated code; exposes UNKEY_DEFAULT_DOMAIN in the ctrl manifest; updates MinIO image references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Ctrl as ctrl (deploy workflow)
participant DB as DB / Querier
participant Metald as metald backend
participant DNS as Domains store
rect rgba(230,245,255,0.6)
note over Ctrl: Initialize deploy workflow (metald_backend, UNKEY_DEFAULT_DOMAIN, is_running_docker)
end
User->>Ctrl: Trigger deploy
Ctrl->>DB: Fetch Workspace by ID
DB-->>Ctrl: Workspace (slug)
Ctrl->>DB: Fetch Project by ID
DB-->>Ctrl: Project (slug)
Ctrl->>DB: FindEnvironmentById
DB-->>Ctrl: Environment (slug)
Ctrl->>DB: Fetch Deployment by ID
DB-->>Ctrl: Deployment (git SHA, branch)
rect rgba(240,255,230,0.6)
note over Ctrl: Build domains via buildDomains(workspaceSlug, projectSlug, environmentSlug, gitSha, branch, defaultDomain)
end
Ctrl->>Metald: Create deployment
Metald-->>Ctrl: Deployment (VM IDs)
Ctrl->>DNS: Create domain entries (type: wildcard) for each built domain
DNS-->>Ctrl: OK
opt OpenAPI scraping
note right of Ctrl: OpenAPI scraping steps commented out (disabled)
end
Ctrl-->>User: Deployment pending/created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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). (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 |
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deployment/docker-compose.yaml (1)
167-176: Switch to bitnamilegacy image is fine; fix console port mapping to match env (3903).MINIO_CONSOLE_PORT_NUMBER is 3903, but ports map 2903:2903. Console won’t be reachable as configured. Use 3903:3903 to stay consistent with prior learnings and k8s manifests.
Apply:
s3: networks: - default container_name: s3 - image: bitnamilegacy/minio:2025.7.23 + image: bitnamilegacy/minio:2025.7.23 ports: - 3902:3902 - - 2903:2903 + - 3903:3903 environment: MINIO_ROOT_USER: minio_root_user MINIO_ROOT_PASSWORD: minio_root_password MINIO_API_PORT_NUMBER: 3902 MINIO_CONSOLE_PORT_NUMBER: 3903go/apps/ctrl/services/deployment/deploy_workflow.go (1)
201-206: Go compile bug: “for i := range 300” is invalid.This won’t compile. Use a classic counter loop.
Apply:
- for i := range 300 { + for i := 0; i < 300; i++ {
🧹 Nitpick comments (7)
deployment/docker-compose.yaml (1)
167-167: Confirm “bitnamilegacy/minio” choice and pin by digest.If “legacy” is intentional, pin to a digest to avoid supply‑chain drift and keep docker-compose and k8s images aligned exactly.
go/k8s/manifests/s3.yaml (1)
34-34: Confirm move to bitnamilegacy and consider digest pin.The image change is consistent with docker-compose. Please confirm the move is intentional and pin to an image digest for reproducibility.
go/apps/ctrl/services/deployment/domains.go (2)
45-70: Slugging rules: preserve separators and normalize more safely.Current sluggify drops characters like “/_.+” entirely, which can collapse distinct branch names. Prefer replacing non-alphanumerics with “-”, deduping “-”, and trimming both ends. Also consider capping to keep label <=63 in the caller.
Apply:
- var nonAlphanumericRegex = regexp.MustCompile(`[^a-zA-Z0-9\s]`) - var multipleSpacesRegex = regexp.MustCompile(`\s+`) + var nonAlphanumericRegex = regexp.MustCompile(`[^a-zA-Z0-9]+`) + var multipleHyphensRegex = regexp.MustCompile(`-+`) @@ - // Remove all non-alphanumeric characters except spaces - s = nonAlphanumericRegex.ReplaceAllString(s, "") - - // Replace multiple spaces with single space - s = multipleSpacesRegex.ReplaceAllString(s, " ") - - // Replace spaces with hyphens - s = strings.ReplaceAll(s, " ", "-") + // Replace any run of non-alphanumerics with a hyphen + s = nonAlphanumericRegex.ReplaceAllString(s, "-") + // Collapse multiple hyphens + s = multipleHyphensRegex.ReplaceAllString(s, "-") + // Trim hyphens + s = strings.Trim(s, "-")
16-16: Optional: normalize apex.Consider trimming scheme/trailing dot to avoid accidental inputs like “https://unkey.local.”.
go/apps/ctrl/services/deployment/deploy_workflow.go (3)
97-120: Use read-only handle for reads (if available).These steps only read; prefer w.db.RO() to leverage replicas and avoid write pool contention (you already use RO on partitionDB).
If RO exists:
- return db.Query.FindWorkspaceByID(stepCtx, w.db.RW(), req.WorkspaceID) + return db.Query.FindWorkspaceByID(stepCtx, w.db.RO(), req.WorkspaceID)Repeat for project, environment, deployment.
277-284: LGTM: centralizing domain generation. Also guard empty defaultDomain.If defaultDomain is empty, you’ll generate invalid FQDNs. Validate at workflow init and fail fast or set a sane default.
Apply (constructor):
return &DeployWorkflow{ db: cfg.DB, partitionDB: cfg.PartitionDB, logger: cfg.Logger, deploymentBackend: deploymentBackend, - defaultDomain: cfg.DefaultDomain, + defaultDomain: cfg.DefaultDomain, }And before use:
+ if w.defaultDomain == "" { + return fmt.Errorf("UNKEY_DEFAULT_DOMAIN is required") + }
321-331: Comment/code mismatch on “local” skipping.isLocalHostname treats defaultDomain (e.g., unkey.local) as NOT local, so these domains get configs (desired). Update the comment to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deployment/docker-compose.yaml(1 hunks)go/apps/ctrl/services/deployment/deploy_workflow.go(6 hunks)go/apps/ctrl/services/deployment/domains.go(1 hunks)go/k8s/manifests/ctrl.yaml(1 hunks)go/k8s/manifests/s3.yaml(1 hunks)go/pkg/db/environment_find_by_id.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/environment_find_by_id.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-08T14:55:11.981Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: deployment/docker-compose.yaml:179-184
Timestamp: 2025-08-08T14:55:11.981Z
Learning: In deployment/docker-compose.yaml (development only), MinIO is configured with API on 3902 and console on 3903; ports should map 3902:3902 and 3903:3903 to match MINIO_API_PORT_NUMBER and MINIO_CONSOLE_PORT_NUMBER.
Applied to files:
go/k8s/manifests/s3.yamldeployment/docker-compose.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/services/deployment/deploy_workflow.go
📚 Learning: 2025-09-01T01:57:42.227Z
Learnt from: imeyer
PR: unkeyed/unkey#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:
go/apps/ctrl/services/deployment/deploy_workflow.go
🧬 Code graph analysis (3)
go/pkg/db/querier_generated.go (1)
go/pkg/db/environment_find_by_id.sql_generated.go (1)
FindEnvironmentByIdRow(19-25)
go/pkg/db/environment_find_by_id.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/ctrl/services/deployment/deploy_workflow.go (6)
go/pkg/otel/logging/interface.go (1)
Logger(11-116)go/pkg/hydra/step.go (2)
Step(67-276)StepVoid(302-310)go/pkg/db/models_generated.go (2)
Workspace(770-788)DomainsTypeWildcard(243-243)go/pkg/db/project_find_by_id.sql_generated.go (1)
FindProjectByIdRow(28-38)go/pkg/db/environment_find_by_id.sql_generated.go (1)
FindEnvironmentByIdRow(19-25)go/pkg/db/deployment_find_by_id.sql_generated.go (1)
FindDeploymentByIdRow(36-53)
⏰ 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: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (8)
go/k8s/manifests/ctrl.yaml (1)
74-75: LGTM: default apex wired into ctrl.Matches the new domain-generation flow; also set in docker-compose. No further action.
go/pkg/db/querier_generated.go (1)
192-197: LGTM: Querier extended with FindEnvironmentById.Signature and docs align with the generated implementation and call sites.
go/pkg/db/queries/environment_find_by_id.sql (1)
1-4: LGTM: minimal query for environment lookups.Fields match generated row type and usage.
go/pkg/db/environment_find_by_id.sql_generated.go (3)
13-17: LGTM: generated SQL constant matches query.
19-25: LGTM: row type matches selected columns.
32-43: LGTM: method scans in correct order.go/apps/ctrl/services/deployment/deploy_workflow.go (2)
43-47: LGTM: startup log context helpful.
287-305: Confirm BulkQuery.InsertDomains performs upsert semantics.Branch/env domains must move between deployments. Ensure the bulk path mirrors InsertDomain’s ON DUPLICATE KEY UPDATE behavior.
If not, switch to per‑row InsertDomain or add upsert logic to the bulk query.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
Other than my slug comment and Flo's I think it looks great.
3 participants
What does this PR do?
Improves deployment domain generation by implementing a more structured approach to domain naming. This PR:
buildDomainsfunction that generates consistent domain patterns:<projectslug>-git-<gitsha>-<workspaceslug>.domain<projectslug>-git-<branchname>-<workspaceslug>.domain<projectslug>-<environmentslug>-<workspaceslug>.domainExample CLI output
wildcardinstead ofcustomType of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainSummary by CodeRabbit
New Features
Chores