Conversation
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughUpdates compose orchestration to add a default network and replace simple depends_on lists with conditional health/start gating and required flags; the Go Docker backend now attaches created containers to the compose network and adds compose labels. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant DC as docker-compose
participant Dep as Dependencies (mysql, redis, clickhouse, s3, planetscale, agent)
participant S as Services (apiv2, gw, ctrl, dashboard)
Dev->>DC: docker compose up
DC->>Dep: Start dependency containers
Dep-->>DC: Report health/start (service_healthy / service_started)
DC->>S: Start services when gated conditions satisfied (required: true)
note over DC,S: Start-order enforced by health/start conditions
sequenceDiagram
autonumber
participant CTRL as ctrl (Go)
participant DE as Docker Engine API
CTRL->>DE: ContainerCreate(config, hostConfig, networkingConfig{EndpointsConfig: {"unkey_default": {...}}})
DE-->>CTRL: Container ID
CTRL->>DE: StartContainer(Container ID)
DE-->>CTRL: Running
note over CTRL,DE: Container created with compose labels and attached to "unkey_default" network
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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: 0
🧹 Nitpick comments (1)
deployment/docker-compose.yaml (1)
311-316: Appropriate use ofservice_started; optionally add healthchecks and switch toservice_healthy.For
planetscale/agent(no healthchecks),service_startedis correct. If you add simple healthchecks later, consider switching toservice_healthyfor stronger readiness guarantees. (docs.docker.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deployment/docker-compose.yaml(4 hunks)go/apps/ctrl/services/deployment/backends/docker.go(1 hunks)
⏰ 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 API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (4)
deployment/docker-compose.yaml (3)
64-69: Good: health‑gated startup for apiv2.Using
condition: service_healthyfor mysql/redis/clickhouse ensures apiv2 starts after dependencies are ready. LGTM. (docs.docker.com)
258-263: Same note onrequired: true— redundant; remove it or require Compose ≥ v2.20.The
requiredkey defaults to true and was introduced in Docker Compose v2.20.0; keeping it is only for clarity but imposes Compose ≥ v2.20 in dev/CI. (docs.docker.com)Location: deployment/docker-compose.yaml (lines 258-263).
212-223: Drop redundant depends_onrequired: true(Compose ≥ v2.20.2)
depends_on.requireddefaults to true; support for the attribute was added in Docker Compose v2.20.2 — remove theserequired: trueentries to reduce noise or keep them for explicitness. (github.com)If you want the gw service to restart when a dependency is restarted via Compose operations, add
restart: trueunder the dependency mapping. (github.com)File: deployment/docker-compose.yaml lines 212–223
Couldn't verify locally here (docker: command not found). Run
docker compose versionlocally and paste the output for confirmation.go/apps/ctrl/services/deployment/backends/docker.go (1)
284-286: Don't hardcode the Docker network; make it configurable and validate it exists.Setting NetworkMode: "unkey_default" ties runtime to a specific Compose project name and will fail if that network doesn’t exist. Inject the network name via config (env), store it on DockerBackend, and preflight-check with NetworkInspect; log a warning or return a clear error.
Apply within this hunk:
- NetworkMode: "unkey_default", + NetworkMode: d.networkMode,And add these changes outside this hunk to wire config + validation:
--- a/go/apps/ctrl/services/deployment/backends/docker.go +++ b/go/apps/ctrl/services/deployment/backends/docker.go @@ -import ( +import ( "context" "fmt" "io" "strconv" "strings" "sync" "time" + "os" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/image" "github.com/docker/docker/client" "github.com/docker/docker/errdefs" "github.com/docker/go-connections/nat" @@ type DockerBackend struct { logger logging.Logger dockerClient *client.Client deployments map[string]*dockerDeployment mutex sync.RWMutex isRunningDocker bool // Whether this service is running in Docker + networkMode container.NetworkMode } @@ func NewDockerBackend(logger logging.Logger, isRunningDocker bool) (*DockerBackend, error) { @@ - return &DockerBackend{ + // Determine target network (configurable; default aligns with compose project "name: unkey") + network := os.Getenv("UNKEY_DOCKER_NETWORK") + if network == "" && isRunningDocker { + network = "unkey_default" + } + + // Best‑effort validation: warn if the configured network is missing + if network != "" { + if _, err := dockerClient.NetworkInspect(ctx, network, types.NetworkInspectOptions{}); err != nil { + logger.Warn("configured Docker network not found; containers may not be reachable", + "network", network, "error", err) + } + } + + return &DockerBackend{ logger: logger.With("backend", "docker"), dockerClient: dockerClient, deployments: make(map[string]*dockerDeployment), isRunningDocker: isRunningDocker, + networkMode: container.NetworkMode(network), }, nil }Verification: the provided script could not run here because the docker CLI is not available (docker: command not found). Run this locally or in CI to verify the configured network exists:
docker network ls --format '{{.Name}}' | grep -x 'unkey_default' || echo "warn: unkey_default not present" docker info >/dev/null && echo "docker reachable" docker compose version
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
go/deploy/ctrl/docker-compose.yml (1)
57-71: Healthcheck style is fine; confirm wget availability or switch to CMD-SHELL.Current array form is valid. Ensure the image actually contains wget; if not, use CMD-SHELL with curl or wget.
Example (keeps wget, fewer layers to parse):
- healthcheck: - test: - [ - "CMD", - "wget", - "--no-verbose", - "--tries=1", - "--spider", - "http://localhost:8084/_/health", - ] + healthcheck: + test: ["CMD-SHELL", "wget -q --tries=1 --spider http://localhost:8084/_/health || exit 1"] interval: 30s timeout: 5s retries: 3 start_period: 10sgo/apps/ctrl/services/deployment/backends/docker.go (1)
295-299: Avoid hard-coding the network name; make it configurable (ties to compose project name).Hard-coding "unkey_default" couples this to a specific compose name. Derive from COMPOSE_PROJECT_NAME (default: unkey) so environments don’t break.
- networkingConfig := &network.NetworkingConfig{ - EndpointsConfig: map[string]*network.EndpointSettings{ - "unkey_default": {}, - }, - } + // derive network from compose project name (e.g., "unkey" -> "unkey_default") + composeProjectName := getEnvDefault("COMPOSE_PROJECT_NAME", "unkey") + composeNetworkName := fmt.Sprintf("%s_default", composeProjectName) + networkingConfig := &network.NetworkingConfig{ + EndpointsConfig: map[string]*network.EndpointSettings{ + composeNetworkName: {}, + }, + }Add helper (outside this hunk):
import "os" func getEnvDefault(k, def string) string { if v := os.Getenv(k); v != "" { return v } return def }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployment/docker-compose.yaml(18 hunks)go/apps/ctrl/services/deployment/backends/docker.go(3 hunks)go/deploy/ctrl/docker-compose.yml(1 hunks)
⏰ 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 (4)
go/apps/ctrl/services/deployment/backends/docker.go (1)
301-301: LGTM: attach to network at create-time.Passing networkingConfig to ContainerCreate is the right call; avoids a second connect step.
deployment/docker-compose.yaml (3)
4-5: Default network wiring is a good improvement.Consistently placing services on the default network and declaring it top-level matches the backend’s expectations.
Minor ask: keep the compose project name (“name: unkey”) stable so the network remains “unkey_default”, which the backend currently targets.
Also applies to: 30-31, 49-50, 62-63, 92-93, 106-107, 130-131, 164-165, 186-187, 219-220, 268-269, 307-308, 318-319, 330-331, 408-410
72-77: Good: health-gated dependencies for apiv2.Switching to service_healthy improves startup determinism.
232-243: Incorrect — keeprequired: trueunder depends_on
Compose spec and Docker Compose v2.20.0+ supportdepends_onservice entries withconditionandrequired(default: true). Only removerequired: trueif you must support Compose versions older than v2.20.0.Likely an incorrect or invalid review comment.
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (09/15/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
4 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Bug Fixes
Chores