fix: address CodeRabbit review — sentinels, safety, docs, test robustness
- Add ErrPreviewNotFound sentinel, use errors.Is in admin handler - Replace http.Error with h.sendError for consistent JSON responses - Fix preview domain order in docs to match GeneratePreviewDomain impl - Add MkdirAll before atomic write in preview store - Log TTL cleanup errors instead of swallowing them - Refcount name locks to prevent map entry races - Sanitize explicit preview names, use exec.CommandContext - Add WaitForContainer to Runtime interface - Fix flaky test with single reference time, check ignored errors - Reorder imports (stdlib/external/internal), add edge case tests
- commit
- 13bd9a4e1dcaaf8030c078bb570e2e31284afbf9
- author
- brice · · 2026-03-19 06:18 UTC
- parent
- 7fa1856
- signature
- unsigned
19 files · +116 -56
M docs/config/preview.md +14 -14
| @@ -50,17 +50,17 @@ Patterns use standard glob syntax. A tag must match at least one pattern in the | |||
| 50 | 50 | ||
| 51 | 51 | Preview domains are derived from the base route for the deployed image. Gordon combines the base domain, the separator, and a URL-safe slug of the image tag. | |
| 52 | 52 | ||
| 53 | - | ``` | |
| 54 | - | <base-domain><separator><tag-slug> | |
| 53 | + | ```text | |
| 54 | + | <app><separator><preview-name><rest-of-domain> | |
| 55 | 55 | ``` | |
| 56 | 56 | ||
| 57 | - | For example, with `separator = "--"` and base route `app.example.com`: | |
| 57 | + | For example, with `separator = "--"` and base route `myapp.example.com`: | |
| 58 | 58 | ||
| 59 | 59 | | Image Tag | Preview Domain | | |
| 60 | 60 | |-----------|----------------| | |
| 61 | - | | `preview-my-feature` | `my-feature--app.example.com` | | |
| 62 | - | | `pr-42` | `42--app.example.com` | | |
| 63 | - | | `preview-fix-login` | `fix-login--app.example.com` | | |
| 61 | + | | `preview-my-feature` | `myapp--my-feature.example.com` | | |
| 62 | + | | `pr-42` | `myapp--42.example.com` | | |
| 63 | + | | `preview-fix-login` | `myapp--fix-login.example.com` | | |
| 64 | 64 | ||
| 65 | 65 | The tag prefix matched by `tag_patterns` is stripped to keep domains short. Slashes in tags are replaced with hyphens. | |
| 66 | 66 | ||
| @@ -87,8 +87,8 @@ gordon preview create app.example.com --tag preview-my-feature | |||
| 87 | 87 | Reset the TTL on an existing preview without redeploying: | |
| 88 | 88 | ||
| 89 | 89 | ```bash | |
| 90 | - | gordon preview extend my-feature--app.example.com | |
| 91 | - | gordon preview extend my-feature--app.example.com --ttl 24h | |
| 90 | + | gordon preview extend myapp--my-feature.example.com | |
| 91 | + | gordon preview extend myapp--my-feature.example.com --ttl 24h | |
| 92 | 92 | ``` | |
| 93 | 93 | ||
| 94 | 94 | Without `--ttl`, the configured default TTL is applied from the current time. | |
| @@ -96,7 +96,7 @@ Without `--ttl`, the configured default TTL is applied from the current time. | |||
| 96 | 96 | ### Delete a Preview | |
| 97 | 97 | ||
| 98 | 98 | ```bash | |
| 99 | - | gordon preview delete my-feature--app.example.com | |
| 99 | + | gordon preview delete myapp--my-feature.example.com | |
| 100 | 100 | ``` | |
| 101 | 101 | ||
| 102 | 102 | Stops the container, removes the route, and (unless `volumes.preserve = true`) removes any cloned volumes. | |
| @@ -138,7 +138,7 @@ jobs: | |||
| 138 | 138 | issue_number: context.issue.number, | |
| 139 | 139 | owner: context.repo.owner, | |
| 140 | 140 | repo: context.repo.repo, | |
| 141 | - | body: `Preview deployed: https://${{ github.event.pull_request.number }}--app.example.com` | |
| 141 | + | body: `Preview deployed: https://app--${{ github.event.pull_request.number }}.example.com` | |
| 142 | 142 | }) | |
| 143 | 143 | ``` | |
| 144 | 144 | ||
| @@ -168,7 +168,7 @@ jobs: | |||
| 168 | 168 | env: | |
| 169 | 169 | PR_NUMBER: ${{ github.event.pull_request.number }} | |
| 170 | 170 | run: | | |
| 171 | - | gordon --server ${{ vars.GORDON_SERVER }} preview delete ${PR_NUMBER}--app.example.com | |
| 171 | + | gordon --server ${{ vars.GORDON_SERVER }} preview delete app--${PR_NUMBER}.example.com | |
| 172 | 172 | ``` | |
| 173 | 173 | ||
| 174 | 174 | ## Lifecycle | |
| @@ -177,7 +177,7 @@ jobs: | |||
| 177 | 177 | ||
| 178 | 178 | When a push matches a `tag_patterns` entry: | |
| 179 | 179 | ||
| 180 | - | 1. Gordon creates a new route: `<slug>--<base-domain>` → pushed image | |
| 180 | + | 1. Gordon creates a new route: `<app><separator><preview-name><rest-of-domain>` → pushed image | |
| 181 | 181 | 2. If `data_copy = true`, volumes from the base route are cloned into the preview | |
| 182 | 182 | 3. The preview TTL timer starts | |
| 183 | 183 | 4. The container is deployed with zero-downtime rules disabled (previews are always cold-starts) | |
| @@ -227,8 +227,8 @@ preserve = false | |||
| 227 | 227 | ``` | |
| 228 | 228 | ||
| 229 | 229 | With this config: | |
| 230 | - | - Pushing `myapp:pr-99` creates `99--app.example.com` with cloned volumes | |
| 231 | - | - Pushing `myapi:preview-auth-refactor` creates `auth-refactor--api.example.com` | |
| 230 | + | - Pushing `myapp:pr-99` creates `myapp--99.example.com` with cloned volumes | |
| 231 | + | - Pushing `myapi:preview-auth-refactor` creates `myapi--auth-refactor.example.com` | |
| 232 | 232 | - Both previews expire after 48 hours and volumes are removed on teardown | |
| 233 | 233 | ||
| 234 | 234 | ## Related | |
| @@ -50,17 +50,17 @@ Patterns use standard glob syntax. A tag must match at least one pattern in the | |||||
| 50 | 50 | ||||
| 51 | Preview domains are derived from the base route for the deployed image. Gordon combines the base domain, the separator, and a URL-safe slug of the image tag. | 51 | Preview domains are derived from the base route for the deployed image. Gordon combines the base domain, the separator, and a URL-safe slug of the image tag. | ||
| 52 | 52 | ||||
| 53 | - | ``` | 53 | + | ```text |
| 54 | - | <base-domain><separator><tag-slug> | 54 | + | <app><separator><preview-name><rest-of-domain> |
| 55 | ``` | 55 | ``` | ||
| 56 | 56 | ||||
| 57 | - | For example, with `separator = "--"` and base route `app.example.com`: | 57 | + | For example, with `separator = "--"` and base route `myapp.example.com`: |
| 58 | 58 | ||||
| 59 | | Image Tag | Preview Domain | | 59 | | Image Tag | Preview Domain | | ||
| 60 | |-----------|----------------| | 60 | |-----------|----------------| | ||
| 61 | - | | `preview-my-feature` | `my-feature--app.example.com` | | 61 | + | | `preview-my-feature` | `myapp--my-feature.example.com` | |
| 62 | - | | `pr-42` | `42--app.example.com` | | 62 | + | | `pr-42` | `myapp--42.example.com` | |
| 63 | - | | `preview-fix-login` | `fix-login--app.example.com` | | 63 | + | | `preview-fix-login` | `myapp--fix-login.example.com` | |
| 64 | 64 | ||||
| 65 | The tag prefix matched by `tag_patterns` is stripped to keep domains short. Slashes in tags are replaced with hyphens. | 65 | The tag prefix matched by `tag_patterns` is stripped to keep domains short. Slashes in tags are replaced with hyphens. | ||
| 66 | 66 | ||||
| @@ -87,8 +87,8 @@ gordon preview create app.example.com --tag preview-my-feature | |||||
| 87 | Reset the TTL on an existing preview without redeploying: | 87 | Reset the TTL on an existing preview without redeploying: | ||
| 88 | 88 | ||||
| 89 | ```bash | 89 | ```bash | ||
| 90 | - | gordon preview extend my-feature--app.example.com | 90 | + | gordon preview extend myapp--my-feature.example.com |
| 91 | - | gordon preview extend my-feature--app.example.com --ttl 24h | 91 | + | gordon preview extend myapp--my-feature.example.com --ttl 24h |
| 92 | ``` | 92 | ``` | ||
| 93 | 93 | ||||
| 94 | Without `--ttl`, the configured default TTL is applied from the current time. | 94 | Without `--ttl`, the configured default TTL is applied from the current time. | ||
| @@ -96,7 +96,7 @@ Without `--ttl`, the configured default TTL is applied from the current time. | |||||
| 96 | ### Delete a Preview | 96 | ### Delete a Preview | ||
| 97 | 97 | ||||
| 98 | ```bash | 98 | ```bash | ||
| 99 | - | gordon preview delete my-feature--app.example.com | 99 | + | gordon preview delete myapp--my-feature.example.com |
| 100 | ``` | 100 | ``` | ||
| 101 | 101 | ||||
| 102 | Stops the container, removes the route, and (unless `volumes.preserve = true`) removes any cloned volumes. | 102 | Stops the container, removes the route, and (unless `volumes.preserve = true`) removes any cloned volumes. | ||
| @@ -138,7 +138,7 @@ jobs: | |||||
| 138 | issue_number: context.issue.number, | 138 | issue_number: context.issue.number, | ||
| 139 | owner: context.repo.owner, | 139 | owner: context.repo.owner, | ||
| 140 | repo: context.repo.repo, | 140 | repo: context.repo.repo, | ||
| 141 | - | body: `Preview deployed: https://${{ github.event.pull_request.number }}--app.example.com` | 141 | + | body: `Preview deployed: https://app--${{ github.event.pull_request.number }}.example.com` |
| 142 | }) | 142 | }) | ||
| 143 | ``` | 143 | ``` | ||
| 144 | 144 | ||||
| @@ -168,7 +168,7 @@ jobs: | |||||
| 168 | env: | 168 | env: | ||
| 169 | PR_NUMBER: ${{ github.event.pull_request.number }} | 169 | PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| 170 | run: | | 170 | run: | | ||
| 171 | - | gordon --server ${{ vars.GORDON_SERVER }} preview delete ${PR_NUMBER}--app.example.com | 171 | + | gordon --server ${{ vars.GORDON_SERVER }} preview delete app--${PR_NUMBER}.example.com |
| 172 | ``` | 172 | ``` | ||
| 173 | 173 | ||||
| 174 | ## Lifecycle | 174 | ## Lifecycle | ||
| @@ -177,7 +177,7 @@ jobs: | |||||
| 177 | 177 | ||||
| 178 | When a push matches a `tag_patterns` entry: | 178 | When a push matches a `tag_patterns` entry: | ||
| 179 | 179 | ||||
| 180 | - | 1. Gordon creates a new route: `<slug>--<base-domain>` → pushed image | 180 | + | 1. Gordon creates a new route: `<app><separator><preview-name><rest-of-domain>` → pushed image |
| 181 | 2. If `data_copy = true`, volumes from the base route are cloned into the preview | 181 | 2. If `data_copy = true`, volumes from the base route are cloned into the preview | ||
| 182 | 3. The preview TTL timer starts | 182 | 3. The preview TTL timer starts | ||
| 183 | 4. The container is deployed with zero-downtime rules disabled (previews are always cold-starts) | 183 | 4. The container is deployed with zero-downtime rules disabled (previews are always cold-starts) | ||
| @@ -227,8 +227,8 @@ preserve = false | |||||
| 227 | ``` | 227 | ``` | ||
| 228 | 228 | ||||
| 229 | With this config: | 229 | With this config: | ||
| 230 | - | - Pushing `myapp:pr-99` creates `99--app.example.com` with cloned volumes | 230 | + | - Pushing `myapp:pr-99` creates `myapp--99.example.com` with cloned volumes |
| 231 | - | - Pushing `myapi:preview-auth-refactor` creates `auth-refactor--api.example.com` | 231 | + | - Pushing `myapi:preview-auth-refactor` creates `myapi--auth-refactor.example.com` |
| 232 | - Both previews expire after 48 hours and volumes are removed on teardown | 232 | - Both previews expire after 48 hours and volumes are removed on teardown | ||
| 233 | 233 | ||||
| 234 | ## Related | 234 | ## Related | ||
M internal/adapters/in/cli/preview.go +15 -7
| @@ -57,7 +57,7 @@ func newPreviewCreateCmd() *cobra.Command { | |||
| 57 | 57 | out := cmd.OutOrStdout() | |
| 58 | 58 | ||
| 59 | 59 | // Resolve preview name from arg or git branch. | |
| 60 | - | name, err := resolvePreviewName(args) | |
| 60 | + | name, err := resolvePreviewName(ctx, args) | |
| 61 | 61 | if err != nil { | |
| 62 | 62 | return err | |
| 63 | 63 | } | |
| @@ -205,15 +205,23 @@ func printPreviewTable(out io.Writer, previews []domain.PreviewRoute) error { | |||
| 205 | 205 | return w.Flush() | |
| 206 | 206 | } | |
| 207 | 207 | ||
| 208 | - | func resolvePreviewName(args []string) (string, error) { | |
| 208 | + | func resolvePreviewName(ctx context.Context, args []string) (string, error) { | |
| 209 | 209 | if len(args) > 0 { | |
| 210 | - | return args[0], nil | |
| 210 | + | name := preview.SanitizeBranchName(args[0]) | |
| 211 | + | if name == "" { | |
| 212 | + | return "", fmt.Errorf("invalid preview name after sanitization: %q", args[0]) | |
| 213 | + | } | |
| 214 | + | return name, nil | |
| 211 | 215 | } | |
| 212 | - | branch, err := detectGitBranch() | |
| 216 | + | branch, err := detectGitBranch(ctx) | |
| 213 | 217 | if err != nil { | |
| 214 | 218 | return "", fmt.Errorf("no preview name provided and could not detect git branch: %w", err) | |
| 215 | 219 | } | |
| 216 | - | return preview.SanitizeBranchName(branch), nil | |
| 220 | + | name := preview.SanitizeBranchName(branch) | |
| 221 | + | if name == "" { | |
| 222 | + | return "", fmt.Errorf("git branch %q produced empty preview name after sanitization", branch) | |
| 223 | + | } | |
| 224 | + | return name, nil | |
| 217 | 225 | } | |
| 218 | 226 | ||
| 219 | 227 | func resolvePreviewImageRef(ctx context.Context, cp ControlPlane, out io.Writer, previewTag string) (string, error) { | |
| @@ -287,8 +295,8 @@ func printPreviewCreateSummary(out io.Writer, name, ttl string, noData bool) err | |||
| 287 | 295 | return nil | |
| 288 | 296 | } | |
| 289 | 297 | ||
| 290 | - | func detectGitBranch() (string, error) { | |
| 291 | - | out, err := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD").Output() // #nosec G204 | |
| 298 | + | func detectGitBranch(ctx context.Context) (string, error) { | |
| 299 | + | out, err := exec.CommandContext(ctx, "git", "rev-parse", "--abbrev-ref", "HEAD").Output() // #nosec G204 | |
| 292 | 300 | if err != nil { | |
| 293 | 301 | return "", fmt.Errorf("detect git branch: %w", err) | |
| 294 | 302 | } | |
| @@ -57,7 +57,7 @@ func newPreviewCreateCmd() *cobra.Command { | |||||
| 57 | out := cmd.OutOrStdout() | 57 | out := cmd.OutOrStdout() | ||
| 58 | 58 | ||||
| 59 | // Resolve preview name from arg or git branch. | 59 | // Resolve preview name from arg or git branch. | ||
| 60 | - | name, err := resolvePreviewName(args) | 60 | + | name, err := resolvePreviewName(ctx, args) |
| 61 | if err != nil { | 61 | if err != nil { | ||
| 62 | return err | 62 | return err | ||
| 63 | } | 63 | } | ||
| @@ -205,15 +205,23 @@ func printPreviewTable(out io.Writer, previews []domain.PreviewRoute) error { | |||||
| 205 | return w.Flush() | 205 | return w.Flush() | ||
| 206 | } | 206 | } | ||
| 207 | 207 | ||||
| 208 | - | func resolvePreviewName(args []string) (string, error) { | 208 | + | func resolvePreviewName(ctx context.Context, args []string) (string, error) { |
| 209 | if len(args) > 0 { | 209 | if len(args) > 0 { | ||
| 210 | - | return args[0], nil | 210 | + | name := preview.SanitizeBranchName(args[0]) |
| 211 | + | if name == "" { | |||
| 212 | + | return "", fmt.Errorf("invalid preview name after sanitization: %q", args[0]) | |||
| 213 | + | } | |||
| 214 | + | return name, nil | |||
| 211 | } | 215 | } | ||
| 212 | - | branch, err := detectGitBranch() | 216 | + | branch, err := detectGitBranch(ctx) |
| 213 | if err != nil { | 217 | if err != nil { | ||
| 214 | return "", fmt.Errorf("no preview name provided and could not detect git branch: %w", err) | 218 | return "", fmt.Errorf("no preview name provided and could not detect git branch: %w", err) | ||
| 215 | } | 219 | } | ||
| 216 | - | return preview.SanitizeBranchName(branch), nil | 220 | + | name := preview.SanitizeBranchName(branch) |
| 221 | + | if name == "" { | |||
| 222 | + | return "", fmt.Errorf("git branch %q produced empty preview name after sanitization", branch) | |||
| 223 | + | } | |||
| 224 | + | return name, nil | |||
| 217 | } | 225 | } | ||
| 218 | 226 | ||||
| 219 | func resolvePreviewImageRef(ctx context.Context, cp ControlPlane, out io.Writer, previewTag string) (string, error) { | 227 | func resolvePreviewImageRef(ctx context.Context, cp ControlPlane, out io.Writer, previewTag string) (string, error) { | ||
| @@ -287,8 +295,8 @@ func printPreviewCreateSummary(out io.Writer, name, ttl string, noData bool) err | |||||
| 287 | return nil | 295 | return nil | ||
| 288 | } | 296 | } | ||
| 289 | 297 | ||||
| 290 | - | func detectGitBranch() (string, error) { | 298 | + | func detectGitBranch(ctx context.Context) (string, error) { |
| 291 | - | out, err := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD").Output() // #nosec G204 | 299 | + | out, err := exec.CommandContext(ctx, "git", "rev-parse", "--abbrev-ref", "HEAD").Output() // #nosec G204 |
| 292 | if err != nil { | 300 | if err != nil { | ||
| 293 | return "", fmt.Errorf("detect git branch: %w", err) | 301 | return "", fmt.Errorf("detect git branch: %w", err) | ||
| 294 | } | 302 | } | ||
M internal/adapters/in/http/admin/preview_handler.go +7 -6
| @@ -3,6 +3,7 @@ package admin | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "encoding/json" | |
| 6 | + | "errors" | |
| 6 | 7 | "net/http" | |
| 7 | 8 | "strings" | |
| 8 | 9 | "time" | |
| @@ -92,10 +93,10 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||
| 92 | 93 | ||
| 93 | 94 | if err := h.previewSvc.Delete(ctx, name); err != nil { | |
| 94 | 95 | log.Error().Err(err).Str("name", name).Msg("failed to delete preview") | |
| 95 | - | if strings.Contains(err.Error(), "not found") { | |
| 96 | - | http.Error(w, err.Error(), http.StatusNotFound) | |
| 96 | + | if errors.Is(err, domain.ErrPreviewNotFound) { | |
| 97 | + | h.sendError(w, http.StatusNotFound, "preview not found") | |
| 97 | 98 | } else { | |
| 98 | - | http.Error(w, err.Error(), http.StatusInternalServerError) | |
| 99 | + | h.sendError(w, http.StatusInternalServerError, "failed to delete preview") | |
| 99 | 100 | } | |
| 100 | 101 | return | |
| 101 | 102 | } | |
| @@ -131,10 +132,10 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||
| 131 | 132 | ||
| 132 | 133 | if err := h.previewSvc.Extend(ctx, name, ttl); err != nil { | |
| 133 | 134 | log.Error().Err(err).Str("name", name).Dur("ttl", ttl).Msg("failed to extend preview TTL") | |
| 134 | - | if strings.Contains(err.Error(), "not found") { | |
| 135 | - | http.Error(w, err.Error(), http.StatusNotFound) | |
| 135 | + | if errors.Is(err, domain.ErrPreviewNotFound) { | |
| 136 | + | h.sendError(w, http.StatusNotFound, "preview not found") | |
| 136 | 137 | } else { | |
| 137 | - | http.Error(w, err.Error(), http.StatusInternalServerError) | |
| 138 | + | h.sendError(w, http.StatusInternalServerError, "failed to extend preview") | |
| 138 | 139 | } | |
| 139 | 140 | return | |
| 140 | 141 | } | |
| @@ -3,6 +3,7 @@ package admin | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "encoding/json" | 5 | "encoding/json" | ||
| 6 | + | "errors" | |||
| 6 | "net/http" | 7 | "net/http" | ||
| 7 | "strings" | 8 | "strings" | ||
| 8 | "time" | 9 | "time" | ||
| @@ -92,10 +93,10 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||||
| 92 | 93 | ||||
| 93 | if err := h.previewSvc.Delete(ctx, name); err != nil { | 94 | if err := h.previewSvc.Delete(ctx, name); err != nil { | ||
| 94 | log.Error().Err(err).Str("name", name).Msg("failed to delete preview") | 95 | log.Error().Err(err).Str("name", name).Msg("failed to delete preview") | ||
| 95 | - | if strings.Contains(err.Error(), "not found") { | 96 | + | if errors.Is(err, domain.ErrPreviewNotFound) { |
| 96 | - | http.Error(w, err.Error(), http.StatusNotFound) | 97 | + | h.sendError(w, http.StatusNotFound, "preview not found") |
| 97 | } else { | 98 | } else { | ||
| 98 | - | http.Error(w, err.Error(), http.StatusInternalServerError) | 99 | + | h.sendError(w, http.StatusInternalServerError, "failed to delete preview") |
| 99 | } | 100 | } | ||
| 100 | return | 101 | return | ||
| 101 | } | 102 | } | ||
| @@ -131,10 +132,10 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||||
| 131 | 132 | ||||
| 132 | if err := h.previewSvc.Extend(ctx, name, ttl); err != nil { | 133 | if err := h.previewSvc.Extend(ctx, name, ttl); err != nil { | ||
| 133 | log.Error().Err(err).Str("name", name).Dur("ttl", ttl).Msg("failed to extend preview TTL") | 134 | log.Error().Err(err).Str("name", name).Dur("ttl", ttl).Msg("failed to extend preview TTL") | ||
| 134 | - | if strings.Contains(err.Error(), "not found") { | 135 | + | if errors.Is(err, domain.ErrPreviewNotFound) { |
| 135 | - | http.Error(w, err.Error(), http.StatusNotFound) | 136 | + | h.sendError(w, http.StatusNotFound, "preview not found") |
| 136 | } else { | 137 | } else { | ||
| 137 | - | http.Error(w, err.Error(), http.StatusInternalServerError) | 138 | + | h.sendError(w, http.StatusInternalServerError, "failed to extend preview") |
| 138 | } | 139 | } | ||
| 139 | return | 140 | return | ||
| 140 | } | 141 | } | ||
M internal/adapters/out/filesystem/previewstore.go +3 -0
| @@ -65,6 +65,9 @@ func (s *PreviewStore) Save(_ context.Context, previews []domain.PreviewRoute) e | |||
| 65 | 65 | ||
| 66 | 66 | // Atomic write: temp file → rename | |
| 67 | 67 | dir := filepath.Dir(s.path) | |
| 68 | + | if err := os.MkdirAll(dir, 0o755); err != nil { | |
| 69 | + | return err | |
| 70 | + | } | |
| 68 | 71 | tmp, err := os.CreateTemp(dir, ".previews-*.json.tmp") | |
| 69 | 72 | if err != nil { | |
| 70 | 73 | return err | |
| @@ -65,6 +65,9 @@ func (s *PreviewStore) Save(_ context.Context, previews []domain.PreviewRoute) e | |||||
| 65 | 65 | ||||
| 66 | // Atomic write: temp file → rename | 66 | // Atomic write: temp file → rename | ||
| 67 | dir := filepath.Dir(s.path) | 67 | dir := filepath.Dir(s.path) | ||
| 68 | + | if err := os.MkdirAll(dir, 0o755); err != nil { | |||
| 69 | + | return err | |||
| 70 | + | } | |||
| 68 | tmp, err := os.CreateTemp(dir, ".previews-*.json.tmp") | 71 | tmp, err := os.CreateTemp(dir, ".previews-*.json.tmp") | ||
| 69 | if err != nil { | 72 | if err != nil { | ||
| 70 | return err | 73 | return err | ||
M internal/app/run.go +10 -3
| @@ -578,12 +578,19 @@ func initPreviewService(ctx context.Context, cfg Config, svc *services, log zero | |||
| 578 | 578 | } | |
| 579 | 579 | ||
| 580 | 580 | svc.previewService.StartTicker(ctx, 60*time.Second, func(ctx context.Context, p domain.PreviewRoute) { | |
| 581 | + | log := zerowrap.FromCtx(ctx) | |
| 581 | 582 | for _, containerName := range p.Containers { | |
| 582 | - | _ = svc.runtime.StopContainer(ctx, containerName) | |
| 583 | - | _ = svc.runtime.RemoveContainer(ctx, containerName, true) | |
| 583 | + | if err := svc.runtime.StopContainer(ctx, containerName); err != nil { | |
| 584 | + | log.Warn().Err(err).Str("container", containerName).Str("preview", p.Domain).Msg("failed to stop preview container") | |
| 585 | + | } | |
| 586 | + | if err := svc.runtime.RemoveContainer(ctx, containerName, true); err != nil { | |
| 587 | + | log.Warn().Err(err).Str("container", containerName).Str("preview", p.Domain).Msg("failed to remove preview container") | |
| 588 | + | } | |
| 584 | 589 | } | |
| 585 | 590 | for _, volName := range p.Volumes { | |
| 586 | - | _ = svc.runtime.RemoveVolume(ctx, volName, true) | |
| 591 | + | if err := svc.runtime.RemoveVolume(ctx, volName, true); err != nil { | |
| 592 | + | log.Warn().Err(err).Str("volume", volName).Str("preview", p.Domain).Msg("failed to remove preview volume") | |
| 593 | + | } | |
| 587 | 594 | } | |
| 588 | 595 | svc.proxySvc.InvalidateTarget(ctx, p.Domain) | |
| 589 | 596 | }) | |
| @@ -578,12 +578,19 @@ func initPreviewService(ctx context.Context, cfg Config, svc *services, log zero | |||||
| 578 | } | 578 | } | ||
| 579 | 579 | ||||
| 580 | svc.previewService.StartTicker(ctx, 60*time.Second, func(ctx context.Context, p domain.PreviewRoute) { | 580 | svc.previewService.StartTicker(ctx, 60*time.Second, func(ctx context.Context, p domain.PreviewRoute) { | ||
| 581 | + | log := zerowrap.FromCtx(ctx) | |||
| 581 | for _, containerName := range p.Containers { | 582 | for _, containerName := range p.Containers { | ||
| 582 | - | _ = svc.runtime.StopContainer(ctx, containerName) | 583 | + | if err := svc.runtime.StopContainer(ctx, containerName); err != nil { |
| 583 | - | _ = svc.runtime.RemoveContainer(ctx, containerName, true) | 584 | + | log.Warn().Err(err).Str("container", containerName).Str("preview", p.Domain).Msg("failed to stop preview container") |
| 585 | + | } | |||
| 586 | + | if err := svc.runtime.RemoveContainer(ctx, containerName, true); err != nil { | |||
| 587 | + | log.Warn().Err(err).Str("container", containerName).Str("preview", p.Domain).Msg("failed to remove preview container") | |||
| 588 | + | } | |||
| 584 | } | 589 | } | ||
| 585 | for _, volName := range p.Volumes { | 590 | for _, volName := range p.Volumes { | ||
| 586 | - | _ = svc.runtime.RemoveVolume(ctx, volName, true) | 591 | + | if err := svc.runtime.RemoveVolume(ctx, volName, true); err != nil { |
| 592 | + | log.Warn().Err(err).Str("volume", volName).Str("preview", p.Domain).Msg("failed to remove preview volume") | |||
| 593 | + | } | |||
| 587 | } | 594 | } | ||
| 588 | svc.proxySvc.InvalidateTarget(ctx, p.Domain) | 595 | svc.proxySvc.InvalidateTarget(ctx, p.Domain) | ||
| 589 | }) | 596 | }) | ||
M internal/domain/auto.go +1 -1
| @@ -10,7 +10,7 @@ type AutoConfig struct { | |||
| 10 | 10 | Preview PreviewConfig | |
| 11 | 11 | } | |
| 12 | 12 | ||
| 13 | - | // AutoRouteConfig holds auto-route specific settings. | |
| 13 | + | // AutoRouteConfig holds auto-route specific settings (reserved for future use). | |
| 14 | 14 | type AutoRouteConfig struct{} | |
| 15 | 15 | ||
| 16 | 16 | // PreviewConfig holds auto-preview specific settings. | |
| @@ -10,7 +10,7 @@ type AutoConfig struct { | |||||
| 10 | Preview PreviewConfig | 10 | Preview PreviewConfig | ||
| 11 | } | 11 | } | ||
| 12 | 12 | ||||
| 13 | - | // AutoRouteConfig holds auto-route specific settings. | 13 | + | // AutoRouteConfig holds auto-route specific settings (reserved for future use). |
| 14 | type AutoRouteConfig struct{} | 14 | type AutoRouteConfig struct{} | ||
| 15 | 15 | ||||
| 16 | // PreviewConfig holds auto-preview specific settings. | 16 | // PreviewConfig holds auto-preview specific settings. | ||
M internal/domain/errors.go +3 -0
| @@ -40,6 +40,9 @@ var ( | |||
| 40 | 40 | ErrVolumeNotFound = errors.New("volume not found") | |
| 41 | 41 | ErrVolumeExists = errors.New("volume already exists") | |
| 42 | 42 | ||
| 43 | + | // Preview errors | |
| 44 | + | ErrPreviewNotFound = errors.New("preview not found") | |
| 45 | + | ||
| 43 | 46 | // Config errors | |
| 44 | 47 | ErrConfigNotFound = errors.New("configuration not found") | |
| 45 | 48 | ErrInvalidConfig = errors.New("invalid configuration") | |
| @@ -40,6 +40,9 @@ var ( | |||||
| 40 | ErrVolumeNotFound = errors.New("volume not found") | 40 | ErrVolumeNotFound = errors.New("volume not found") | ||
| 41 | ErrVolumeExists = errors.New("volume already exists") | 41 | ErrVolumeExists = errors.New("volume already exists") | ||
| 42 | 42 | ||||
| 43 | + | // Preview errors | |||
| 44 | + | ErrPreviewNotFound = errors.New("preview not found") | |||
| 45 | + | ||||
| 43 | // Config errors | 46 | // Config errors | ||
| 44 | ErrConfigNotFound = errors.New("configuration not found") | 47 | ErrConfigNotFound = errors.New("configuration not found") | ||
| 45 | ErrInvalidConfig = errors.New("invalid configuration") | 48 | ErrInvalidConfig = errors.New("invalid configuration") | ||
M internal/domain/preview_test.go +5 -4
| @@ -8,19 +8,20 @@ import ( | |||
| 8 | 8 | ) | |
| 9 | 9 | ||
| 10 | 10 | func TestPreviewRoute_IsExpired(t *testing.T) { | |
| 11 | + | now := time.Now() | |
| 11 | 12 | tests := []struct { | |
| 12 | 13 | name string | |
| 13 | 14 | expiresAt time.Time | |
| 14 | 15 | want bool | |
| 15 | 16 | }{ | |
| 16 | - | {"expired", time.Now().Add(-1 * time.Hour), true}, | |
| 17 | - | {"not expired", time.Now().Add(1 * time.Hour), false}, | |
| 18 | - | {"just expired", time.Now().Add(-1 * time.Second), true}, | |
| 17 | + | {"expired", now.Add(-1 * time.Hour), true}, | |
| 18 | + | {"not expired", now.Add(1 * time.Hour), false}, | |
| 19 | + | {"just expired", now.Add(-1 * time.Second), true}, | |
| 19 | 20 | } | |
| 20 | 21 | for _, tt := range tests { | |
| 21 | 22 | t.Run(tt.name, func(t *testing.T) { | |
| 22 | 23 | p := PreviewRoute{ExpiresAt: tt.expiresAt} | |
| 23 | - | assert.Equal(t, tt.want, p.IsExpired(time.Now())) | |
| 24 | + | assert.Equal(t, tt.want, p.IsExpired(now)) | |
| 24 | 25 | }) | |
| 25 | 26 | } | |
| 26 | 27 | } | |
| @@ -8,19 +8,20 @@ import ( | |||||
| 8 | ) | 8 | ) | ||
| 9 | 9 | ||||
| 10 | func TestPreviewRoute_IsExpired(t *testing.T) { | 10 | func TestPreviewRoute_IsExpired(t *testing.T) { | ||
| 11 | + | now := time.Now() | |||
| 11 | tests := []struct { | 12 | tests := []struct { | ||
| 12 | name string | 13 | name string | ||
| 13 | expiresAt time.Time | 14 | expiresAt time.Time | ||
| 14 | want bool | 15 | want bool | ||
| 15 | }{ | 16 | }{ | ||
| 16 | - | {"expired", time.Now().Add(-1 * time.Hour), true}, | 17 | + | {"expired", now.Add(-1 * time.Hour), true}, |
| 17 | - | {"not expired", time.Now().Add(1 * time.Hour), false}, | 18 | + | {"not expired", now.Add(1 * time.Hour), false}, |
| 18 | - | {"just expired", time.Now().Add(-1 * time.Second), true}, | 19 | + | {"just expired", now.Add(-1 * time.Second), true}, |
| 19 | } | 20 | } | ||
| 20 | for _, tt := range tests { | 21 | for _, tt := range tests { | ||
| 21 | t.Run(tt.name, func(t *testing.T) { | 22 | t.Run(tt.name, func(t *testing.T) { | ||
| 22 | p := PreviewRoute{ExpiresAt: tt.expiresAt} | 23 | p := PreviewRoute{ExpiresAt: tt.expiresAt} | ||
| 23 | - | assert.Equal(t, tt.want, p.IsExpired(time.Now())) | 24 | + | assert.Equal(t, tt.want, p.IsExpired(now)) |
| 24 | }) | 25 | }) | ||
| 25 | } | 26 | } | ||
| 26 | } | 27 | } | ||
M internal/usecase/auto/dispatcher_test.go +2 -1
| @@ -4,8 +4,9 @@ import ( | |||
| 4 | 4 | "context" | |
| 5 | 5 | "testing" | |
| 6 | 6 | ||
| 7 | - | "github.com/bnema/gordon/internal/domain" | |
| 8 | 7 | "github.com/stretchr/testify/assert" | |
| 8 | + | ||
| 9 | + | "github.com/bnema/gordon/internal/domain" | |
| 9 | 10 | ) | |
| 10 | 11 | ||
| 11 | 12 | type mockHandler struct { | |
| @@ -4,8 +4,9 @@ import ( | |||||
| 4 | "context" | 4 | "context" | ||
| 5 | "testing" | 5 | "testing" | ||
| 6 | 6 | ||||
| 7 | - | "github.com/bnema/gordon/internal/domain" | |||
| 8 | "github.com/stretchr/testify/assert" | 7 | "github.com/stretchr/testify/assert" | ||
| 8 | + | ||||
| 9 | + | "github.com/bnema/gordon/internal/domain" | |||
| 9 | ) | 10 | ) | ||
| 10 | 11 | ||||
| 11 | type mockHandler struct { | 12 | type mockHandler struct { | ||
M internal/usecase/auto/labels.go +1 -0
| @@ -88,6 +88,7 @@ func ParseImageLabels(configData []byte) (*domain.ImageLabels, error) { | |||
| 88 | 88 | // ExtractLabels extracts Gordon labels from an image manifest by fetching the | |
| 89 | 89 | // config blob from blobStorage and parsing it. It is the shared implementation | |
| 90 | 90 | // used by both AutoRouteHandler and AutoPreviewHandler. | |
| 91 | + | // ctx is accepted for future use when BlobStorage gains context support. | |
| 91 | 92 | func ExtractLabels(ctx context.Context, manifestData []byte, blobStorage out.BlobStorage) (*domain.ImageLabels, error) { | |
| 92 | 93 | configDigest, err := ParseConfigDigest(manifestData) | |
| 93 | 94 | if err != nil { | |
| @@ -88,6 +88,7 @@ func ParseImageLabels(configData []byte) (*domain.ImageLabels, error) { | |||||
| 88 | // ExtractLabels extracts Gordon labels from an image manifest by fetching the | 88 | // ExtractLabels extracts Gordon labels from an image manifest by fetching the | ||
| 89 | // config blob from blobStorage and parsing it. It is the shared implementation | 89 | // config blob from blobStorage and parsing it. It is the shared implementation | ||
| 90 | // used by both AutoRouteHandler and AutoPreviewHandler. | 90 | // used by both AutoRouteHandler and AutoPreviewHandler. | ||
| 91 | + | // ctx is accepted for future use when BlobStorage gains context support. | |||
| 91 | func ExtractLabels(ctx context.Context, manifestData []byte, blobStorage out.BlobStorage) (*domain.ImageLabels, error) { | 92 | func ExtractLabels(ctx context.Context, manifestData []byte, blobStorage out.BlobStorage) (*domain.ImageLabels, error) { | ||
| 92 | configDigest, err := ParseConfigDigest(manifestData) | 93 | configDigest, err := ParseConfigDigest(manifestData) | ||
| 93 | if err != nil { | 94 | if err != nil { | ||
M internal/usecase/auto/labels_test.go +7 -4
| @@ -226,7 +226,8 @@ func TestExtractLabels(t *testing.T) { | |||
| 226 | 226 | "digest": "sha256:configdigest123", | |
| 227 | 227 | }, | |
| 228 | 228 | } | |
| 229 | - | manifestData, _ := json.Marshal(manifest) | |
| 229 | + | manifestData, err := json.Marshal(manifest) | |
| 230 | + | require.NoError(t, err) | |
| 230 | 231 | ||
| 231 | 232 | imageConf := map[string]any{ | |
| 232 | 233 | "config": map[string]any{ | |
| @@ -236,7 +237,8 @@ func TestExtractLabels(t *testing.T) { | |||
| 236 | 237 | }, | |
| 237 | 238 | }, | |
| 238 | 239 | } | |
| 239 | - | configData, _ := json.Marshal(imageConf) | |
| 240 | + | configData, err := json.Marshal(imageConf) | |
| 241 | + | require.NoError(t, err) | |
| 240 | 242 | ||
| 241 | 243 | blobStorage := mocks.NewMockBlobStorage(t) | |
| 242 | 244 | blobStorage.EXPECT().GetBlob("sha256:configdigest123").Return(io.NopCloser(strings.NewReader(string(configData))), nil) | |
| @@ -264,11 +266,12 @@ func TestExtractLabels_EmptyConfigDigest(t *testing.T) { | |||
| 264 | 266 | "digest": "", | |
| 265 | 267 | }, | |
| 266 | 268 | } | |
| 267 | - | manifestData, _ := json.Marshal(manifest) | |
| 269 | + | manifestData, err := json.Marshal(manifest) | |
| 270 | + | require.NoError(t, err) | |
| 268 | 271 | ||
| 269 | 272 | blobStorage := mocks.NewMockBlobStorage(t) | |
| 270 | 273 | ||
| 271 | - | _, err := ExtractLabels(ctx, manifestData, blobStorage) | |
| 274 | + | _, err = ExtractLabels(ctx, manifestData, blobStorage) | |
| 272 | 275 | assert.Error(t, err) | |
| 273 | 276 | assert.Contains(t, err.Error(), "no config digest") | |
| 274 | 277 | } | |
| @@ -226,7 +226,8 @@ func TestExtractLabels(t *testing.T) { | |||||
| 226 | "digest": "sha256:configdigest123", | 226 | "digest": "sha256:configdigest123", | ||
| 227 | }, | 227 | }, | ||
| 228 | } | 228 | } | ||
| 229 | - | manifestData, _ := json.Marshal(manifest) | 229 | + | manifestData, err := json.Marshal(manifest) |
| 230 | + | require.NoError(t, err) | |||
| 230 | 231 | ||||
| 231 | imageConf := map[string]any{ | 232 | imageConf := map[string]any{ | ||
| 232 | "config": map[string]any{ | 233 | "config": map[string]any{ | ||
| @@ -236,7 +237,8 @@ func TestExtractLabels(t *testing.T) { | |||||
| 236 | }, | 237 | }, | ||
| 237 | }, | 238 | }, | ||
| 238 | } | 239 | } | ||
| 239 | - | configData, _ := json.Marshal(imageConf) | 240 | + | configData, err := json.Marshal(imageConf) |
| 241 | + | require.NoError(t, err) | |||
| 240 | 242 | ||||
| 241 | blobStorage := mocks.NewMockBlobStorage(t) | 243 | blobStorage := mocks.NewMockBlobStorage(t) | ||
| 242 | blobStorage.EXPECT().GetBlob("sha256:configdigest123").Return(io.NopCloser(strings.NewReader(string(configData))), nil) | 244 | blobStorage.EXPECT().GetBlob("sha256:configdigest123").Return(io.NopCloser(strings.NewReader(string(configData))), nil) | ||
| @@ -264,11 +266,12 @@ func TestExtractLabels_EmptyConfigDigest(t *testing.T) { | |||||
| 264 | "digest": "", | 266 | "digest": "", | ||
| 265 | }, | 267 | }, | ||
| 266 | } | 268 | } | ||
| 267 | - | manifestData, _ := json.Marshal(manifest) | 269 | + | manifestData, err := json.Marshal(manifest) |
| 270 | + | require.NoError(t, err) | |||
| 268 | 271 | ||||
| 269 | blobStorage := mocks.NewMockBlobStorage(t) | 272 | blobStorage := mocks.NewMockBlobStorage(t) | ||
| 270 | 273 | ||||
| 271 | - | _, err := ExtractLabels(ctx, manifestData, blobStorage) | 274 | + | _, err = ExtractLabels(ctx, manifestData, blobStorage) |
| 272 | assert.Error(t, err) | 275 | assert.Error(t, err) | ||
| 273 | assert.Contains(t, err.Error(), "no config digest") | 276 | assert.Contains(t, err.Error(), "no config digest") | ||
| 274 | } | 277 | } | ||
M internal/usecase/auto/preview/handler.go +2 -0
| @@ -2,6 +2,7 @@ package preview | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "fmt" | |
| 5 | 6 | ||
| 6 | 7 | "github.com/bnema/zerowrap" | |
| 7 | 8 | ||
| @@ -47,6 +48,7 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||
| 47 | 48 | ||
| 48 | 49 | payload, ok := event.Data.(domain.ImagePushedPayload) | |
| 49 | 50 | if !ok { | |
| 51 | + | log.Debug().Str("actual_type", fmt.Sprintf("%T", event.Data)).Msg("unexpected event data type, skipping") | |
| 50 | 52 | return nil | |
| 51 | 53 | } | |
| 52 | 54 | ||
| @@ -2,6 +2,7 @@ package preview | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "fmt" | |||
| 5 | 6 | ||||
| 6 | "github.com/bnema/zerowrap" | 7 | "github.com/bnema/zerowrap" | ||
| 7 | 8 | ||||
| @@ -47,6 +48,7 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||||
| 47 | 48 | ||||
| 48 | payload, ok := event.Data.(domain.ImagePushedPayload) | 49 | payload, ok := event.Data.(domain.ImagePushedPayload) | ||
| 49 | if !ok { | 50 | if !ok { | ||
| 51 | + | log.Debug().Str("actual_type", fmt.Sprintf("%T", event.Data)).Msg("unexpected event data type, skipping") | |||
| 50 | return nil | 52 | return nil | ||
| 51 | } | 53 | } | ||
| 52 | 54 | ||||
M internal/usecase/auto/preview/integration_test.go +6 -3
| @@ -5,9 +5,10 @@ import ( | |||
| 5 | 5 | "testing" | |
| 6 | 6 | "time" | |
| 7 | 7 | ||
| 8 | - | "github.com/bnema/gordon/internal/domain" | |
| 9 | 8 | "github.com/stretchr/testify/assert" | |
| 10 | 9 | "github.com/stretchr/testify/require" | |
| 10 | + | ||
| 11 | + | "github.com/bnema/gordon/internal/domain" | |
| 11 | 12 | ) | |
| 12 | 13 | ||
| 13 | 14 | func TestPreviewLifecycle_Integration(t *testing.T) { | |
| @@ -17,7 +18,7 @@ func TestPreviewLifecycle_Integration(t *testing.T) { | |||
| 17 | 18 | ctx := context.Background() | |
| 18 | 19 | ||
| 19 | 20 | // Create | |
| 20 | - | svc.CreatePreview(ctx, CreatePreviewRequest{ | |
| 21 | + | err := svc.CreatePreview(ctx, CreatePreviewRequest{ | |
| 21 | 22 | Name: "test-feat", | |
| 22 | 23 | Domain: "myapp--test-feat.example.com", | |
| 23 | 24 | BaseRoute: "myapp.example.com", | |
| @@ -29,6 +30,7 @@ func TestPreviewLifecycle_Integration(t *testing.T) { | |||
| 29 | 30 | DataCopy: false, | |
| 30 | 31 | }, | |
| 31 | 32 | }) | |
| 33 | + | require.NoError(t, err) | |
| 32 | 34 | ||
| 33 | 35 | // Verify created | |
| 34 | 36 | all, err := svc.List(ctx) | |
| @@ -62,7 +64,7 @@ func TestPreviewLifecycle_CleanupExpired(t *testing.T) { | |||
| 62 | 64 | ctx := context.Background() | |
| 63 | 65 | ||
| 64 | 66 | // Create with very short TTL | |
| 65 | - | svc.CreatePreview(ctx, CreatePreviewRequest{ | |
| 67 | + | err := svc.CreatePreview(ctx, CreatePreviewRequest{ | |
| 66 | 68 | Name: "short-lived", | |
| 67 | 69 | Domain: "myapp--short-lived.example.com", | |
| 68 | 70 | BaseRoute: "myapp.example.com", | |
| @@ -74,6 +76,7 @@ func TestPreviewLifecycle_CleanupExpired(t *testing.T) { | |||
| 74 | 76 | DataCopy: false, | |
| 75 | 77 | }, | |
| 76 | 78 | }) | |
| 79 | + | require.NoError(t, err) | |
| 77 | 80 | ||
| 78 | 81 | // Wait for expiry | |
| 79 | 82 | time.Sleep(10 * time.Millisecond) | |
| @@ -5,9 +5,10 @@ import ( | |||||
| 5 | "testing" | 5 | "testing" | ||
| 6 | "time" | 6 | "time" | ||
| 7 | 7 | ||||
| 8 | - | "github.com/bnema/gordon/internal/domain" | |||
| 9 | "github.com/stretchr/testify/assert" | 8 | "github.com/stretchr/testify/assert" | ||
| 10 | "github.com/stretchr/testify/require" | 9 | "github.com/stretchr/testify/require" | ||
| 10 | + | ||||
| 11 | + | "github.com/bnema/gordon/internal/domain" | |||
| 11 | ) | 12 | ) | ||
| 12 | 13 | ||||
| 13 | func TestPreviewLifecycle_Integration(t *testing.T) { | 14 | func TestPreviewLifecycle_Integration(t *testing.T) { | ||
| @@ -17,7 +18,7 @@ func TestPreviewLifecycle_Integration(t *testing.T) { | |||||
| 17 | ctx := context.Background() | 18 | ctx := context.Background() | ||
| 18 | 19 | ||||
| 19 | // Create | 20 | // Create | ||
| 20 | - | svc.CreatePreview(ctx, CreatePreviewRequest{ | 21 | + | err := svc.CreatePreview(ctx, CreatePreviewRequest{ |
| 21 | Name: "test-feat", | 22 | Name: "test-feat", | ||
| 22 | Domain: "myapp--test-feat.example.com", | 23 | Domain: "myapp--test-feat.example.com", | ||
| 23 | BaseRoute: "myapp.example.com", | 24 | BaseRoute: "myapp.example.com", | ||
| @@ -29,6 +30,7 @@ func TestPreviewLifecycle_Integration(t *testing.T) { | |||||
| 29 | DataCopy: false, | 30 | DataCopy: false, | ||
| 30 | }, | 31 | }, | ||
| 31 | }) | 32 | }) | ||
| 33 | + | require.NoError(t, err) | |||
| 32 | 34 | ||||
| 33 | // Verify created | 35 | // Verify created | ||
| 34 | all, err := svc.List(ctx) | 36 | all, err := svc.List(ctx) | ||
| @@ -62,7 +64,7 @@ func TestPreviewLifecycle_CleanupExpired(t *testing.T) { | |||||
| 62 | ctx := context.Background() | 64 | ctx := context.Background() | ||
| 63 | 65 | ||||
| 64 | // Create with very short TTL | 66 | // Create with very short TTL | ||
| 65 | - | svc.CreatePreview(ctx, CreatePreviewRequest{ | 67 | + | err := svc.CreatePreview(ctx, CreatePreviewRequest{ |
| 66 | Name: "short-lived", | 68 | Name: "short-lived", | ||
| 67 | Domain: "myapp--short-lived.example.com", | 69 | Domain: "myapp--short-lived.example.com", | ||
| 68 | BaseRoute: "myapp.example.com", | 70 | BaseRoute: "myapp.example.com", | ||
| @@ -74,6 +76,7 @@ func TestPreviewLifecycle_CleanupExpired(t *testing.T) { | |||||
| 74 | DataCopy: false, | 76 | DataCopy: false, | ||
| 75 | }, | 77 | }, | ||
| 76 | }) | 78 | }) | ||
| 79 | + | require.NoError(t, err) | |||
| 77 | 80 | ||||
| 78 | // Wait for expiry | 81 | // Wait for expiry | ||
| 79 | time.Sleep(10 * time.Millisecond) | 82 | time.Sleep(10 * time.Millisecond) | ||
M internal/usecase/auto/preview/naming.go +2 -0
| @@ -7,6 +7,8 @@ import ( | |||
| 7 | 7 | ) | |
| 8 | 8 | ||
| 9 | 9 | // ExtractPreviewName strips the matched pattern prefix from a tag. | |
| 10 | + | // Only simple trailing-wildcard patterns like "preview-*" or "pr-*" are supported. | |
| 11 | + | // Complex globs (e.g., "pr-*-test", "*-preview") are not supported. | |
| 10 | 12 | // "preview-login-redesign" with pattern "preview-*" → "login-redesign" | |
| 11 | 13 | func ExtractPreviewName(tag string, patterns []string) string { | |
| 12 | 14 | for _, pattern := range patterns { | |
| @@ -7,6 +7,8 @@ import ( | |||||
| 7 | ) | 7 | ) | ||
| 8 | 8 | ||||
| 9 | // ExtractPreviewName strips the matched pattern prefix from a tag. | 9 | // ExtractPreviewName strips the matched pattern prefix from a tag. | ||
| 10 | + | // Only simple trailing-wildcard patterns like "preview-*" or "pr-*" are supported. | |||
| 11 | + | // Complex globs (e.g., "pr-*-test", "*-preview") are not supported. | |||
| 10 | // "preview-login-redesign" with pattern "preview-*" → "login-redesign" | 12 | // "preview-login-redesign" with pattern "preview-*" → "login-redesign" | ||
| 11 | func ExtractPreviewName(tag string, patterns []string) string { | 13 | func ExtractPreviewName(tag string, patterns []string) string { | ||
| 12 | for _, pattern := range patterns { | 14 | for _, pattern := range patterns { | ||
M internal/usecase/auto/preview/naming_test.go +2 -0
| @@ -69,6 +69,8 @@ func TestSanitizeBranchName(t *testing.T) { | |||
| 69 | 69 | {"special chars", "feat/hello@world!", "helloworld"}, | |
| 70 | 70 | {"long branch", strings.Repeat("a", 70), strings.Repeat("a", 63)}, | |
| 71 | 71 | {"consecutive dashes", "feat/a--b--c", "a-b-c"}, | |
| 72 | + | {"empty branch", "", ""}, | |
| 73 | + | {"whitespace branch", " ", ""}, | |
| 72 | 74 | } | |
| 73 | 75 | for _, tt := range tests { | |
| 74 | 76 | t.Run(tt.name, func(t *testing.T) { | |
| @@ -69,6 +69,8 @@ func TestSanitizeBranchName(t *testing.T) { | |||||
| 69 | {"special chars", "feat/hello@world!", "helloworld"}, | 69 | {"special chars", "feat/hello@world!", "helloworld"}, | ||
| 70 | {"long branch", strings.Repeat("a", 70), strings.Repeat("a", 63)}, | 70 | {"long branch", strings.Repeat("a", 70), strings.Repeat("a", 63)}, | ||
| 71 | {"consecutive dashes", "feat/a--b--c", "a-b-c"}, | 71 | {"consecutive dashes", "feat/a--b--c", "a-b-c"}, | ||
| 72 | + | {"empty branch", "", ""}, | |||
| 73 | + | {"whitespace branch", " ", ""}, | |||
| 72 | } | 74 | } | ||
| 73 | for _, tt := range tests { | 75 | for _, tt := range tests { | ||
| 74 | t.Run(tt.name, func(t *testing.T) { | 76 | t.Run(tt.name, func(t *testing.T) { | ||
M internal/usecase/auto/preview/service.go +31 -11
| @@ -12,13 +12,18 @@ import ( | |||
| 12 | 12 | "github.com/bnema/gordon/internal/domain" | |
| 13 | 13 | ) | |
| 14 | 14 | ||
| 15 | + | type nameLockEntry struct { | |
| 16 | + | mu *sync.Mutex | |
| 17 | + | refs int | |
| 18 | + | } | |
| 19 | + | ||
| 15 | 20 | // Service manages preview lifecycle: CRUD, TTL, persistence. | |
| 16 | 21 | type Service struct { | |
| 17 | 22 | store out.PreviewStore | |
| 18 | 23 | defaultTTL time.Duration | |
| 19 | 24 | previews []domain.PreviewRoute | |
| 20 | 25 | mu sync.RWMutex | |
| 21 | - | nameLocks map[string]*sync.Mutex | |
| 26 | + | nameLocks map[string]*nameLockEntry | |
| 22 | 27 | nameLockMu sync.Mutex | |
| 23 | 28 | } | |
| 24 | 29 | ||
| @@ -26,7 +31,7 @@ func NewService(store out.PreviewStore, defaultTTL time.Duration) *Service { | |||
| 26 | 31 | return &Service{ | |
| 27 | 32 | store: store, | |
| 28 | 33 | defaultTTL: defaultTTL, | |
| 29 | - | nameLocks: make(map[string]*sync.Mutex), | |
| 34 | + | nameLocks: make(map[string]*nameLockEntry), | |
| 30 | 35 | } | |
| 31 | 36 | } | |
| 32 | 37 | ||
| @@ -52,12 +57,19 @@ func (s *Service) Add(ctx context.Context, p domain.PreviewRoute) error { | |||
| 52 | 57 | ||
| 53 | 58 | func (s *Service) Delete(ctx context.Context, name string) error { | |
| 54 | 59 | s.mu.Lock() | |
| 60 | + | found := false | |
| 55 | 61 | filtered := make([]domain.PreviewRoute, 0, len(s.previews)) | |
| 56 | 62 | for _, p := range s.previews { | |
| 57 | - | if p.Name != name { | |
| 63 | + | if p.Name == name { | |
| 64 | + | found = true | |
| 65 | + | } else { | |
| 58 | 66 | filtered = append(filtered, p) | |
| 59 | 67 | } | |
| 60 | 68 | } | |
| 69 | + | if !found { | |
| 70 | + | s.mu.Unlock() | |
| 71 | + | return fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) | |
| 72 | + | } | |
| 61 | 73 | s.previews = filtered | |
| 62 | 74 | cp := make([]domain.PreviewRoute, len(s.previews)) | |
| 63 | 75 | copy(cp, s.previews) | |
| @@ -74,7 +86,7 @@ func (s *Service) Get(_ context.Context, name string) (*domain.PreviewRoute, err | |||
| 74 | 86 | return &p, nil | |
| 75 | 87 | } | |
| 76 | 88 | } | |
| 77 | - | return nil, fmt.Errorf("preview %q not found", name) | |
| 89 | + | return nil, fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) | |
| 78 | 90 | } | |
| 79 | 91 | ||
| 80 | 92 | func (s *Service) List(_ context.Context) ([]domain.PreviewRoute, error) { | |
| @@ -97,7 +109,7 @@ func (s *Service) Extend(ctx context.Context, name string, ttl time.Duration) er | |||
| 97 | 109 | } | |
| 98 | 110 | } | |
| 99 | 111 | s.mu.Unlock() | |
| 100 | - | return fmt.Errorf("preview %q not found", name) | |
| 112 | + | return fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) | |
| 101 | 113 | } | |
| 102 | 114 | ||
| 103 | 115 | func (s *Service) GetExpired() []domain.PreviewRoute { | |
| @@ -163,19 +175,27 @@ func (s *Service) StartTicker(ctx context.Context, interval time.Duration, teard | |||
| 163 | 175 | func (s *Service) AcquireNameLock(name string) *sync.Mutex { | |
| 164 | 176 | s.nameLockMu.Lock() | |
| 165 | 177 | defer s.nameLockMu.Unlock() | |
| 166 | - | mu, ok := s.nameLocks[name] | |
| 178 | + | entry, ok := s.nameLocks[name] | |
| 167 | 179 | if !ok { | |
| 168 | - | mu = &sync.Mutex{} | |
| 169 | - | s.nameLocks[name] = mu | |
| 180 | + | entry = &nameLockEntry{mu: &sync.Mutex{}} | |
| 181 | + | s.nameLocks[name] = entry | |
| 170 | 182 | } | |
| 171 | - | return mu | |
| 183 | + | entry.refs++ | |
| 184 | + | return entry.mu | |
| 172 | 185 | } | |
| 173 | 186 | ||
| 174 | - | // ReleaseNameLock removes the per-name mutex from the map after use. | |
| 187 | + | // ReleaseNameLock decrements the refcount and removes the per-name mutex when no longer in use. | |
| 175 | 188 | func (s *Service) ReleaseNameLock(name string) { | |
| 176 | 189 | s.nameLockMu.Lock() | |
| 177 | 190 | defer s.nameLockMu.Unlock() | |
| 178 | - | delete(s.nameLocks, name) | |
| 191 | + | entry, ok := s.nameLocks[name] | |
| 192 | + | if !ok { | |
| 193 | + | return | |
| 194 | + | } | |
| 195 | + | entry.refs-- | |
| 196 | + | if entry.refs <= 0 { | |
| 197 | + | delete(s.nameLocks, name) | |
| 198 | + | } | |
| 179 | 199 | } | |
| 180 | 200 | ||
| 181 | 201 | // CreatePreviewRequest contains parameters for creating a preview. | |
| @@ -12,13 +12,18 @@ import ( | |||||
| 12 | "github.com/bnema/gordon/internal/domain" | 12 | "github.com/bnema/gordon/internal/domain" | ||
| 13 | ) | 13 | ) | ||
| 14 | 14 | ||||
| 15 | + | type nameLockEntry struct { | |||
| 16 | + | mu *sync.Mutex | |||
| 17 | + | refs int | |||
| 18 | + | } | |||
| 19 | + | ||||
| 15 | // Service manages preview lifecycle: CRUD, TTL, persistence. | 20 | // Service manages preview lifecycle: CRUD, TTL, persistence. | ||
| 16 | type Service struct { | 21 | type Service struct { | ||
| 17 | store out.PreviewStore | 22 | store out.PreviewStore | ||
| 18 | defaultTTL time.Duration | 23 | defaultTTL time.Duration | ||
| 19 | previews []domain.PreviewRoute | 24 | previews []domain.PreviewRoute | ||
| 20 | mu sync.RWMutex | 25 | mu sync.RWMutex | ||
| 21 | - | nameLocks map[string]*sync.Mutex | 26 | + | nameLocks map[string]*nameLockEntry |
| 22 | nameLockMu sync.Mutex | 27 | nameLockMu sync.Mutex | ||
| 23 | } | 28 | } | ||
| 24 | 29 | ||||
| @@ -26,7 +31,7 @@ func NewService(store out.PreviewStore, defaultTTL time.Duration) *Service { | |||||
| 26 | return &Service{ | 31 | return &Service{ | ||
| 27 | store: store, | 32 | store: store, | ||
| 28 | defaultTTL: defaultTTL, | 33 | defaultTTL: defaultTTL, | ||
| 29 | - | nameLocks: make(map[string]*sync.Mutex), | 34 | + | nameLocks: make(map[string]*nameLockEntry), |
| 30 | } | 35 | } | ||
| 31 | } | 36 | } | ||
| 32 | 37 | ||||
| @@ -52,12 +57,19 @@ func (s *Service) Add(ctx context.Context, p domain.PreviewRoute) error { | |||||
| 52 | 57 | ||||
| 53 | func (s *Service) Delete(ctx context.Context, name string) error { | 58 | func (s *Service) Delete(ctx context.Context, name string) error { | ||
| 54 | s.mu.Lock() | 59 | s.mu.Lock() | ||
| 60 | + | found := false | |||
| 55 | filtered := make([]domain.PreviewRoute, 0, len(s.previews)) | 61 | filtered := make([]domain.PreviewRoute, 0, len(s.previews)) | ||
| 56 | for _, p := range s.previews { | 62 | for _, p := range s.previews { | ||
| 57 | - | if p.Name != name { | 63 | + | if p.Name == name { |
| 64 | + | found = true | |||
| 65 | + | } else { | |||
| 58 | filtered = append(filtered, p) | 66 | filtered = append(filtered, p) | ||
| 59 | } | 67 | } | ||
| 60 | } | 68 | } | ||
| 69 | + | if !found { | |||
| 70 | + | s.mu.Unlock() | |||
| 71 | + | return fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) | |||
| 72 | + | } | |||
| 61 | s.previews = filtered | 73 | s.previews = filtered | ||
| 62 | cp := make([]domain.PreviewRoute, len(s.previews)) | 74 | cp := make([]domain.PreviewRoute, len(s.previews)) | ||
| 63 | copy(cp, s.previews) | 75 | copy(cp, s.previews) | ||
| @@ -74,7 +86,7 @@ func (s *Service) Get(_ context.Context, name string) (*domain.PreviewRoute, err | |||||
| 74 | return &p, nil | 86 | return &p, nil | ||
| 75 | } | 87 | } | ||
| 76 | } | 88 | } | ||
| 77 | - | return nil, fmt.Errorf("preview %q not found", name) | 89 | + | return nil, fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) |
| 78 | } | 90 | } | ||
| 79 | 91 | ||||
| 80 | func (s *Service) List(_ context.Context) ([]domain.PreviewRoute, error) { | 92 | func (s *Service) List(_ context.Context) ([]domain.PreviewRoute, error) { | ||
| @@ -97,7 +109,7 @@ func (s *Service) Extend(ctx context.Context, name string, ttl time.Duration) er | |||||
| 97 | } | 109 | } | ||
| 98 | } | 110 | } | ||
| 99 | s.mu.Unlock() | 111 | s.mu.Unlock() | ||
| 100 | - | return fmt.Errorf("preview %q not found", name) | 112 | + | return fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) |
| 101 | } | 113 | } | ||
| 102 | 114 | ||||
| 103 | func (s *Service) GetExpired() []domain.PreviewRoute { | 115 | func (s *Service) GetExpired() []domain.PreviewRoute { | ||
| @@ -163,19 +175,27 @@ func (s *Service) StartTicker(ctx context.Context, interval time.Duration, teard | |||||
| 163 | func (s *Service) AcquireNameLock(name string) *sync.Mutex { | 175 | func (s *Service) AcquireNameLock(name string) *sync.Mutex { | ||
| 164 | s.nameLockMu.Lock() | 176 | s.nameLockMu.Lock() | ||
| 165 | defer s.nameLockMu.Unlock() | 177 | defer s.nameLockMu.Unlock() | ||
| 166 | - | mu, ok := s.nameLocks[name] | 178 | + | entry, ok := s.nameLocks[name] |
| 167 | if !ok { | 179 | if !ok { | ||
| 168 | - | mu = &sync.Mutex{} | 180 | + | entry = &nameLockEntry{mu: &sync.Mutex{}} |
| 169 | - | s.nameLocks[name] = mu | 181 | + | s.nameLocks[name] = entry |
| 170 | } | 182 | } | ||
| 171 | - | return mu | 183 | + | entry.refs++ |
| 184 | + | return entry.mu | |||
| 172 | } | 185 | } | ||
| 173 | 186 | ||||
| 174 | - | // ReleaseNameLock removes the per-name mutex from the map after use. | 187 | + | // ReleaseNameLock decrements the refcount and removes the per-name mutex when no longer in use. |
| 175 | func (s *Service) ReleaseNameLock(name string) { | 188 | func (s *Service) ReleaseNameLock(name string) { | ||
| 176 | s.nameLockMu.Lock() | 189 | s.nameLockMu.Lock() | ||
| 177 | defer s.nameLockMu.Unlock() | 190 | defer s.nameLockMu.Unlock() | ||
| 178 | - | delete(s.nameLocks, name) | 191 | + | entry, ok := s.nameLocks[name] |
| 192 | + | if !ok { | |||
| 193 | + | return | |||
| 194 | + | } | |||
| 195 | + | entry.refs-- | |||
| 196 | + | if entry.refs <= 0 { | |||
| 197 | + | delete(s.nameLocks, name) | |||
| 198 | + | } | |||
| 179 | } | 199 | } | ||
| 180 | 200 | ||||
| 181 | // CreatePreviewRequest contains parameters for creating a preview. | 201 | // CreatePreviewRequest contains parameters for creating a preview. | ||
M internal/usecase/auto/preview/service_test.go +2 -1
| @@ -5,9 +5,10 @@ import ( | |||
| 5 | 5 | "testing" | |
| 6 | 6 | "time" | |
| 7 | 7 | ||
| 8 | - | "github.com/bnema/gordon/internal/domain" | |
| 9 | 8 | "github.com/stretchr/testify/assert" | |
| 10 | 9 | "github.com/stretchr/testify/require" | |
| 10 | + | ||
| 11 | + | "github.com/bnema/gordon/internal/domain" | |
| 11 | 12 | ) | |
| 12 | 13 | ||
| 13 | 14 | type fakeStore struct { | |
| @@ -5,9 +5,10 @@ import ( | |||||
| 5 | "testing" | 5 | "testing" | ||
| 6 | "time" | 6 | "time" | ||
| 7 | 7 | ||||
| 8 | - | "github.com/bnema/gordon/internal/domain" | |||
| 9 | "github.com/stretchr/testify/assert" | 8 | "github.com/stretchr/testify/assert" | ||
| 10 | "github.com/stretchr/testify/require" | 9 | "github.com/stretchr/testify/require" | ||
| 10 | + | ||||
| 11 | + | "github.com/bnema/gordon/internal/domain" | |||
| 11 | ) | 12 | ) | ||
| 12 | 13 | ||||
| 13 | type fakeStore struct { | 14 | type fakeStore struct { | ||
M internal/usecase/auto/preview/ticker_test.go +2 -1
| @@ -5,9 +5,10 @@ import ( | |||
| 5 | 5 | "testing" | |
| 6 | 6 | "time" | |
| 7 | 7 | ||
| 8 | - | "github.com/bnema/gordon/internal/domain" | |
| 9 | 8 | "github.com/stretchr/testify/assert" | |
| 10 | 9 | "github.com/stretchr/testify/require" | |
| 10 | + | ||
| 11 | + | "github.com/bnema/gordon/internal/domain" | |
| 11 | 12 | ) | |
| 12 | 13 | ||
| 13 | 14 | func TestPreviewService_CleanupExpired(t *testing.T) { | |
| @@ -5,9 +5,10 @@ import ( | |||||
| 5 | "testing" | 5 | "testing" | ||
| 6 | "time" | 6 | "time" | ||
| 7 | 7 | ||||
| 8 | - | "github.com/bnema/gordon/internal/domain" | |||
| 9 | "github.com/stretchr/testify/assert" | 8 | "github.com/stretchr/testify/assert" | ||
| 10 | "github.com/stretchr/testify/require" | 9 | "github.com/stretchr/testify/require" | ||
| 10 | + | ||||
| 11 | + | "github.com/bnema/gordon/internal/domain" | |||
| 11 | ) | 12 | ) | ||
| 12 | 13 | ||||
| 13 | func TestPreviewService_CleanupExpired(t *testing.T) { | 14 | func TestPreviewService_CleanupExpired(t *testing.T) { | ||
M pkg/runtime/interface.go +1 -0
| @@ -67,6 +67,7 @@ type Runtime interface { | |||
| 67 | 67 | CreateContainer(ctx context.Context, config *ContainerConfig) (*Container, error) | |
| 68 | 68 | StartContainer(ctx context.Context, containerID string) error | |
| 69 | 69 | StopContainer(ctx context.Context, containerID string) error | |
| 70 | + | WaitForContainer(ctx context.Context, containerID string) error | |
| 70 | 71 | RestartContainer(ctx context.Context, containerID string) error | |
| 71 | 72 | RemoveContainer(ctx context.Context, containerID string, force bool) error | |
| 72 | 73 | ||
| @@ -67,6 +67,7 @@ type Runtime interface { | |||||
| 67 | CreateContainer(ctx context.Context, config *ContainerConfig) (*Container, error) | 67 | CreateContainer(ctx context.Context, config *ContainerConfig) (*Container, error) | ||
| 68 | StartContainer(ctx context.Context, containerID string) error | 68 | StartContainer(ctx context.Context, containerID string) error | ||
| 69 | StopContainer(ctx context.Context, containerID string) error | 69 | StopContainer(ctx context.Context, containerID string) error | ||
| 70 | + | WaitForContainer(ctx context.Context, containerID string) error | |||
| 70 | RestartContainer(ctx context.Context, containerID string) error | 71 | RestartContainer(ctx context.Context, containerID string) error | ||
| 71 | RemoveContainer(ctx context.Context, containerID string, force bool) error | 72 | RemoveContainer(ctx context.Context, containerID string, force bool) error | ||
| 72 | 73 | ||||