fix: address code review findings in DeployIntent, probe sleep, drain delay, and docs
- DeployIntent: validate imageName is non-empty before sending request - tcpProbe/httpProbe: clamp sleepInterval to probe-specific deadline (not just ctx deadline) so probes cannot oversleep past their own timeout; return the probe timeout error (not ctx.Err()) when the probe deadline expires - testMinDelayConfig: set DrainDelayConfigured: true so waitForDrain honours the 1ms drain delay in tests instead of falling back to the 2s default - docs/reference/docker-labels.md: document gordon.health, gordon.domains, gordon.port, and gordon.env-file image labels alongside gordon.proxy.port
- commit
- 5c05b34149c6e936e147d703b9c04aca2ff71997
- author
- brice · · 2026-03-04 19:26 UTC
- parent
- e150f7b
- signature
- unsigned
4 files · +48 -12
M docs/reference/docker-labels.md +21 -1
| @@ -41,7 +41,27 @@ Labels you can set in your Dockerfile: | |||
| 41 | 41 | ||
| 42 | 42 | | Label | Example | Description | | |
| 43 | 43 | |-------|---------|-------------| | |
| 44 | - | | `gordon.proxy.port` | `"3000"` | Port to proxy HTTP traffic to | | |
| 44 | + | | `gordon.domains` | `"app.example.com,www.app.example.com"` | Comma-separated domains for auto-route | | |
| 45 | + | | `gordon.port` | `"3000"` | Port to proxy HTTP traffic to | | |
| 46 | + | | `gordon.proxy.port` | `"3000"` | Port to proxy HTTP traffic to (legacy alias for `gordon.port`) | | |
| 47 | + | | `gordon.health` | `"/healthz"` | HTTP health check endpoint path for readiness probing | | |
| 48 | + | | `gordon.env-file` | `"/app/.env.example"` | Path to env template file inside the image | | |
| 49 | + | ||
| 50 | + | ### Health Check Label | |
| 51 | + | ||
| 52 | + | When `gordon.health` is set, Gordon performs HTTP GET requests to the specified | |
| 53 | + | path during deployment and waits for a 2xx or 3xx response before routing traffic | |
| 54 | + | to the new container: | |
| 55 | + | ||
| 56 | + | ```dockerfile | |
| 57 | + | FROM node:20-alpine | |
| 58 | + | LABEL gordon.health="/api/health" | |
| 59 | + | EXPOSE 3000 | |
| 60 | + | CMD ["node", "server.js"] | |
| 61 | + | ``` | |
| 62 | + | ||
| 63 | + | Gordon probes `http://<container-ip>:3000/api/health` until it gets a successful | |
| 64 | + | response or the `deploy.http_probe_timeout` is reached. | |
| 45 | 65 | ||
| 46 | 66 | ### Proxy Port Label | |
| 47 | 67 | ||
| @@ -41,7 +41,27 @@ Labels you can set in your Dockerfile: | |||||
| 41 | 41 | ||||
| 42 | | Label | Example | Description | | 42 | | Label | Example | Description | | ||
| 43 | |-------|---------|-------------| | 43 | |-------|---------|-------------| | ||
| 44 | - | | `gordon.proxy.port` | `"3000"` | Port to proxy HTTP traffic to | | 44 | + | | `gordon.domains` | `"app.example.com,www.app.example.com"` | Comma-separated domains for auto-route | |
| 45 | + | | `gordon.port` | `"3000"` | Port to proxy HTTP traffic to | | |||
| 46 | + | | `gordon.proxy.port` | `"3000"` | Port to proxy HTTP traffic to (legacy alias for `gordon.port`) | | |||
| 47 | + | | `gordon.health` | `"/healthz"` | HTTP health check endpoint path for readiness probing | | |||
| 48 | + | | `gordon.env-file` | `"/app/.env.example"` | Path to env template file inside the image | | |||
| 49 | + | ||||
| 50 | + | ### Health Check Label | |||
| 51 | + | ||||
| 52 | + | When `gordon.health` is set, Gordon performs HTTP GET requests to the specified | |||
| 53 | + | path during deployment and waits for a 2xx or 3xx response before routing traffic | |||
| 54 | + | to the new container: | |||
| 55 | + | ||||
| 56 | + | ```dockerfile | |||
| 57 | + | FROM node:20-alpine | |||
| 58 | + | LABEL gordon.health="/api/health" | |||
| 59 | + | EXPOSE 3000 | |||
| 60 | + | CMD ["node", "server.js"] | |||
| 61 | + | ``` | |||
| 62 | + | ||||
| 63 | + | Gordon probes `http://<container-ip>:3000/api/health` until it gets a successful | |||
| 64 | + | response or the `deploy.http_probe_timeout` is reached. | |||
| 45 | 65 | ||||
| 46 | ### Proxy Port Label | 66 | ### Proxy Port Label | ||
| 47 | 67 | ||||
M internal/adapters/in/cli/remote/client.go +3 -0
| @@ -727,6 +727,9 @@ func (c *Client) Deploy(ctx context.Context, deployDomain string) (*DeployResult | |||
| 727 | 727 | // DeployIntent tells the server that a CLI-managed push is about to happen, | |
| 728 | 728 | // suppressing event-based deploys for this image. | |
| 729 | 729 | func (c *Client) DeployIntent(ctx context.Context, imageName string) error { | |
| 730 | + | if strings.TrimSpace(imageName) == "" { | |
| 731 | + | return fmt.Errorf("image name cannot be empty") | |
| 732 | + | } | |
| 730 | 733 | resp, err := c.requestWithRetry(ctx, http.MethodPost, "/deploy-intent/"+url.PathEscape(imageName), nil) | |
| 731 | 734 | if err != nil { | |
| 732 | 735 | return err | |
| @@ -727,6 +727,9 @@ func (c *Client) Deploy(ctx context.Context, deployDomain string) (*DeployResult | |||||
| 727 | // DeployIntent tells the server that a CLI-managed push is about to happen, | 727 | // DeployIntent tells the server that a CLI-managed push is about to happen, | ||
| 728 | // suppressing event-based deploys for this image. | 728 | // suppressing event-based deploys for this image. | ||
| 729 | func (c *Client) DeployIntent(ctx context.Context, imageName string) error { | 729 | func (c *Client) DeployIntent(ctx context.Context, imageName string) error { | ||
| 730 | + | if strings.TrimSpace(imageName) == "" { | |||
| 731 | + | return fmt.Errorf("image name cannot be empty") | |||
| 732 | + | } | |||
| 730 | resp, err := c.requestWithRetry(ctx, http.MethodPost, "/deploy-intent/"+url.PathEscape(imageName), nil) | 733 | resp, err := c.requestWithRetry(ctx, http.MethodPost, "/deploy-intent/"+url.PathEscape(imageName), nil) | ||
| 731 | if err != nil { | 734 | if err != nil { | ||
| 732 | return err | 735 | return err | ||
M internal/usecase/container/readiness.go +20 -8
| @@ -47,13 +47,19 @@ func tcpProbe(ctx context.Context, addr string, timeout time.Duration) error { | |||
| 47 | 47 | } | |
| 48 | 48 | ||
| 49 | 49 | sleepInterval := 500 * time.Millisecond | |
| 50 | - | if deadline, ok := ctx.Deadline(); ok { | |
| 51 | - | if rem := time.Until(deadline); rem < sleepInterval { | |
| 52 | - | sleepInterval = rem | |
| 50 | + | if remProbe := time.Until(deadline); remProbe < sleepInterval { | |
| 51 | + | sleepInterval = remProbe | |
| 52 | + | } | |
| 53 | + | if ctxDeadline, ok := ctx.Deadline(); ok { | |
| 54 | + | if remCtx := time.Until(ctxDeadline); remCtx < sleepInterval { | |
| 55 | + | sleepInterval = remCtx | |
| 53 | 56 | } | |
| 54 | 57 | } | |
| 55 | 58 | if sleepInterval <= 0 { | |
| 56 | - | return ctx.Err() | |
| 59 | + | if ctx.Err() != nil { | |
| 60 | + | return ctx.Err() | |
| 61 | + | } | |
| 62 | + | return fmt.Errorf("TCP probe timeout after %s: %s not reachable (attempts=%d, last_error=%v)", timeout, addr, attempts, lastErr) | |
| 57 | 63 | } | |
| 58 | 64 | t := time.NewTimer(sleepInterval) | |
| 59 | 65 | select { | |
| @@ -107,13 +113,19 @@ func httpProbe(ctx context.Context, url string, timeout time.Duration) error { | |||
| 107 | 113 | } | |
| 108 | 114 | ||
| 109 | 115 | sleepInterval := time.Second | |
| 110 | - | if deadline, ok := ctx.Deadline(); ok { | |
| 111 | - | if rem := time.Until(deadline); rem < sleepInterval { | |
| 112 | - | sleepInterval = rem | |
| 116 | + | if remProbe := time.Until(deadline); remProbe < sleepInterval { | |
| 117 | + | sleepInterval = remProbe | |
| 118 | + | } | |
| 119 | + | if ctxDeadline, ok := ctx.Deadline(); ok { | |
| 120 | + | if remCtx := time.Until(ctxDeadline); remCtx < sleepInterval { | |
| 121 | + | sleepInterval = remCtx | |
| 113 | 122 | } | |
| 114 | 123 | } | |
| 115 | 124 | if sleepInterval <= 0 { | |
| 116 | - | return ctx.Err() | |
| 125 | + | if ctx.Err() != nil { | |
| 126 | + | return ctx.Err() | |
| 127 | + | } | |
| 128 | + | return fmt.Errorf("HTTP probe timeout after %s: last status %d from %s", timeout, lastStatus, url) | |
| 117 | 129 | } | |
| 118 | 130 | t := time.NewTimer(sleepInterval) | |
| 119 | 131 | select { | |
| @@ -47,13 +47,19 @@ func tcpProbe(ctx context.Context, addr string, timeout time.Duration) error { | |||||
| 47 | } | 47 | } | ||
| 48 | 48 | ||||
| 49 | sleepInterval := 500 * time.Millisecond | 49 | sleepInterval := 500 * time.Millisecond | ||
| 50 | - | if deadline, ok := ctx.Deadline(); ok { | 50 | + | if remProbe := time.Until(deadline); remProbe < sleepInterval { |
| 51 | - | if rem := time.Until(deadline); rem < sleepInterval { | 51 | + | sleepInterval = remProbe |
| 52 | - | sleepInterval = rem | 52 | + | } |
| 53 | + | if ctxDeadline, ok := ctx.Deadline(); ok { | |||
| 54 | + | if remCtx := time.Until(ctxDeadline); remCtx < sleepInterval { | |||
| 55 | + | sleepInterval = remCtx | |||
| 53 | } | 56 | } | ||
| 54 | } | 57 | } | ||
| 55 | if sleepInterval <= 0 { | 58 | if sleepInterval <= 0 { | ||
| 56 | - | return ctx.Err() | 59 | + | if ctx.Err() != nil { |
| 60 | + | return ctx.Err() | |||
| 61 | + | } | |||
| 62 | + | return fmt.Errorf("TCP probe timeout after %s: %s not reachable (attempts=%d, last_error=%v)", timeout, addr, attempts, lastErr) | |||
| 57 | } | 63 | } | ||
| 58 | t := time.NewTimer(sleepInterval) | 64 | t := time.NewTimer(sleepInterval) | ||
| 59 | select { | 65 | select { | ||
| @@ -107,13 +113,19 @@ func httpProbe(ctx context.Context, url string, timeout time.Duration) error { | |||||
| 107 | } | 113 | } | ||
| 108 | 114 | ||||
| 109 | sleepInterval := time.Second | 115 | sleepInterval := time.Second | ||
| 110 | - | if deadline, ok := ctx.Deadline(); ok { | 116 | + | if remProbe := time.Until(deadline); remProbe < sleepInterval { |
| 111 | - | if rem := time.Until(deadline); rem < sleepInterval { | 117 | + | sleepInterval = remProbe |
| 112 | - | sleepInterval = rem | 118 | + | } |
| 119 | + | if ctxDeadline, ok := ctx.Deadline(); ok { | |||
| 120 | + | if remCtx := time.Until(ctxDeadline); remCtx < sleepInterval { | |||
| 121 | + | sleepInterval = remCtx | |||
| 113 | } | 122 | } | ||
| 114 | } | 123 | } | ||
| 115 | if sleepInterval <= 0 { | 124 | if sleepInterval <= 0 { | ||
| 116 | - | return ctx.Err() | 125 | + | if ctx.Err() != nil { |
| 126 | + | return ctx.Err() | |||
| 127 | + | } | |||
| 128 | + | return fmt.Errorf("HTTP probe timeout after %s: last status %d from %s", timeout, lastStatus, url) | |||
| 117 | } | 129 | } | ||
| 118 | t := time.NewTimer(sleepInterval) | 130 | t := time.NewTimer(sleepInterval) | ||
| 119 | select { | 131 | select { | ||
M internal/usecase/container/service_test.go +4 -3
| @@ -29,9 +29,10 @@ func testContext() context.Context { | |||
| 29 | 29 | // suitable for unit tests that don't want to wait for real timeouts. | |
| 30 | 30 | func testMinDelayConfig() Config { | |
| 31 | 31 | return Config{ | |
| 32 | - | ReadinessDelay: time.Millisecond, | |
| 33 | - | DrainDelay: time.Millisecond, | |
| 34 | - | StabilizationDelay: time.Millisecond, | |
| 32 | + | ReadinessDelay: time.Millisecond, | |
| 33 | + | DrainDelay: time.Millisecond, | |
| 34 | + | DrainDelayConfigured: true, | |
| 35 | + | StabilizationDelay: time.Millisecond, | |
| 35 | 36 | } | |
| 36 | 37 | } | |
| 37 | 38 | ||
| @@ -29,9 +29,10 @@ func testContext() context.Context { | |||||
| 29 | // suitable for unit tests that don't want to wait for real timeouts. | 29 | // suitable for unit tests that don't want to wait for real timeouts. | ||
| 30 | func testMinDelayConfig() Config { | 30 | func testMinDelayConfig() Config { | ||
| 31 | return Config{ | 31 | return Config{ | ||
| 32 | - | ReadinessDelay: time.Millisecond, | 32 | + | ReadinessDelay: time.Millisecond, |
| 33 | - | DrainDelay: time.Millisecond, | 33 | + | DrainDelay: time.Millisecond, |
| 34 | - | StabilizationDelay: time.Millisecond, | 34 | + | DrainDelayConfigured: true, |
| 35 | + | StabilizationDelay: time.Millisecond, | |||
| 35 | } | 36 | } | ||
| 36 | } | 37 | } | ||
| 37 | 38 | ||||