Conversation
Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Claude
AI
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves gh devlake deploy resilience by introducing deploy-time error classification (focused on Docker port conflicts) and adding bounded recovery paths for both local and Azure deployments.
Changes:
- Added
DeployErrorclassification helpers for Docker Compose failures, including port extraction and conflicting container detection. - Implemented a bounded local deploy recovery: on detected port conflict, rewrite compose ports to an alternate bundle and retry once.
- Normalized Azure deploy recovery messaging for login, stopped MySQL servers, and soft-deleted Key Vault conflicts; updated deployment docs accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/deploy.md | Documents local port fallback behavior and Azure bounded recovery messaging. |
| cmd/deploy_local.go | Adds bounded port-fallback retry logic and compose port rewriting for local deploys. |
| cmd/deploy_local_test.go | Adds tests for compose port rewriting behavior and error cases. |
| cmd/deploy_errors.go | Introduces Docker Compose error classification, port extraction, and port-owner discovery. |
| cmd/deploy_errors_test.go | Adds unit tests for port extraction and Docker Compose error classification patterns. |
| cmd/deploy_azure.go | Updates Azure recovery output to a consistent detection → recovery → outcome format. |
Comments suppressed due to low confidence (3)
docs/deploy.md:74
- The documented port-conflict patterns list omits
bind for, butclassifyDockerComposeErrortreatsbind foras one of the recognized patterns. Please align the docs with the actual classifier so users know all the strings that trigger the recovery path.
- **Port conflicts**: When deploying with official or fork images, the CLI detects port conflicts (patterns: `port is already allocated`, `ports are not available`, `address already in use`, `failed programming external connectivity`) and automatically retries with alternate ports (`8085/3004/4004`). Recovery is bounded to a single retry.
- **Custom deployments**: Port conflicts in custom deployments require manual resolution — the CLI will identify the conflicting container and suggest remediation commands.
docs/deploy.md:76
- The local section claims “State checkpointing” saves deployment state early for mid-flight cleanup, but
deploy localdoesn’t appear to write.devlake-local.jsonat all (cleanup falls back todocker-compose.ymlwhen no state exists). Please clarify what state is checkpointed for local deploys, or update the implementation/docs so the claim matches actual behavior.
- **State checkpointing**: Deployment state is saved early to enable cleanup even when deployment fails mid-flight.
cmd/deploy_local.go:200
- With port fallback,
backendURLcan now behttp://localhost:8085, but the later user-facing summary still prints Config UI/Grafana URLs using the default ports (4000/3002). That will mislead users after a successful fallback because the compose file ports are rewritten to 4004/3004. Consider deriving the full endpoint bundle from the selected backend port (8080→{3002,4000}, 8085→{3004,4004}) and printing the matching URLs.
// Allow alternate port bundle for official/fork (not custom)
allowPortFallback := deployLocalSource != "custom"
backendURL, err := startLocalContainers(absDir, buildImages, allowPortFallback, services...)
if err != nil {
return err
}
cfgURL = backendURL
docs/deploy.md
Outdated
| | Config UI | http://localhost:4000 or http://localhost:4004 | — | | ||
| | Grafana | http://localhost:3002 or http://localhost:3004 | admin / admin | | ||
|
|
||
| **Port Fallback**: When deploying with `--source official` or `--source fork`, the CLI automatically recovers from port conflicts by retrying with alternate ports (`8085/3004/4004`). Custom deployments require manual port conflict resolution. |
cmd/deploy_errors.go
Outdated
| // isValidPort checks if a string looks like a valid port number (all digits, 1-65535). | ||
| func isValidPort(s string) bool { | ||
| if len(s) < 1 || len(s) > 5 { | ||
| return false | ||
| } | ||
| for _, ch := range s { | ||
| if ch < '0' || ch > '9' { | ||
| return false | ||
| } | ||
| } | ||
| // Basic range check (ports are 1-65535) | ||
| if len(s) == 5 { | ||
| // Quick check: if > 65535, invalid | ||
| if s > "65535" { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
cmd/deploy_local.go
Outdated
| fmt.Println() | ||
| fmt.Printf("🔧 Port conflict detected on default ports (8080/3002/4000)\n") | ||
| if deployErr.Port != "" { | ||
| fmt.Printf(" Port %s is in use", deployErr.Port) | ||
| if deployErr.Container != "" { | ||
| fmt.Printf(" by container: %s", deployErr.Container) | ||
| } | ||
| fmt.Println() | ||
| } | ||
| fmt.Println() | ||
| fmt.Println("🔄 Retrying with alternate ports (8085/3004/4004)...") | ||
|
|
cmd/deploy_local.go
Outdated
| // Port mapping patterns: | ||
| // - "8080:8080" -> "8085:8080" (external:internal) | ||
| // - "3002:3002" -> "3004:3002" | ||
| // - "4000:4000" -> "4004:4000" | ||
| portMappings := map[string]string{ | ||
| "8080:8080": "8085:8080", | ||
| "- 8080:8080": "- 8085:8080", | ||
| "\"8080:8080\"": "\"8085:8080\"", | ||
| "'8080:8080'": "'8085:8080'", | ||
|
|
||
| "3002:3002": "3004:3002", | ||
| "- 3002:3002": "- 3004:3002", | ||
| "\"3002:3002\"": "\"3004:3002\"", | ||
| "'3002:3002'": "'3004:3002'", | ||
|
|
||
| "4000:4000": "4004:4000", | ||
| "- 4000:4000": "- 4004:4000", | ||
| "\"4000:4000\"": "\"4004:4000\"", | ||
| "'4000:4000'": "'4004:4000'", | ||
| } | ||
|
|
||
| modified := content | ||
| for old, new := range portMappings { | ||
| modified = strings.ReplaceAll(modified, old, new) | ||
| } |
cmd/deploy_errors.go
Outdated
| fmt.Println() | ||
| if de.Port != "" { | ||
| fmt.Printf("❌ Port conflict detected: port %s is already in use.\n", de.Port) | ||
| } else { | ||
| fmt.Println("❌ Port conflict detected: a required port is already in use.") |
|
@claude[agent] Please address the actionable findings from the fresh Copilot review on this PR:
Please push a follow-up commit and keep checks green. |
|
@claude[agent] please address the actionable review feedback on this PR:
After pushing the fixes, I’ll re-run the review/check pass. |
…RLs, docs, and terminal output Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
All findings addressed in e358254:
All tests pass, checks green. |
Claude
AI
changed the title
|
@copilot review the latest updates on this PR, especially the fixes for port validation, exact compose-port rewriting, fallback endpoint reporting, and deploy docs alignment. |
There was a problem hiding this comment.
Pull request overview
This PR updates gh devlake local and Azure deployment UX/documentation to reflect bounded recovery behaviors (port fallback locally; login/MySQL/Key Vault recovery on Azure) and adds test coverage for the new deployment error classification and compose port rewriting.
Changes:
- Add local deploy port-fallback support (single retry) by rewriting compose ports and detecting which backend port is healthy.
- Introduce Docker Compose error classification + port-owner detection helpers, with accompanying unit tests.
- Update deployment docs to document new flags and resilience behaviors for local and Azure deployments.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/deploy.md | Documents new local flags, port fallback behavior, and bounded recovery behavior for local/Azure deploy flows. |
| cmd/deploy_local.go | Implements bounded local port fallback, companion URL inference, and compose port rewriting. |
| cmd/deploy_local_test.go | Adds tests for compose port rewriting behavior across multiple compose port syntaxes. |
| cmd/deploy_errors.go | Adds Docker Compose error classification, port extraction, and port-owner lookup helpers. |
| cmd/deploy_errors_test.go | Adds unit tests for port extraction + compose error classification patterns. |
| cmd/deploy_azure.go | Adjusts Azure deploy output to clearly surface bounded recovery steps for login/MySQL/Key Vault. |
Comments suppressed due to low confidence (1)
cmd/deploy_local.go:497
- This sub-item print includes a leading blank line (
\n ...), which introduces extra vertical spacing within the recovery block and breaks the “sub-items stay tight under their parent” rule. Consider removing the leading\nhere and rely on the surrounding step banners for spacing.
fmt.Println("\n Starting containers with alternate ports...")
err = dockerpkg.ComposeUp(absDir, build, services...)
cmd/deploy_local.go
Outdated
| fmt.Println() | ||
| fmt.Println("❌ Alternate ports are also in use.") |
cmd/deploy_local.go
Outdated
| // Rewrite port mappings in docker-compose.yml or docker-compose-dev.yml | ||
| composePath := filepath.Join(absDir, "docker-compose.yml") | ||
| if _, err := os.Stat(composePath); os.IsNotExist(err) { | ||
| composePath = filepath.Join(absDir, "docker-compose-dev.yml") | ||
| } | ||
|
|
||
| if err := rewriteComposePorts(composePath); err != nil { | ||
| fmt.Printf(" ⚠️ Could not rewrite ports: %v\n", err) | ||
| printDockerPortConflictError(deployErr) | ||
| return "", fmt.Errorf("port conflict and failed to apply alternate ports: %w", err) |
cmd/deploy_local.go
Outdated
| modified := content | ||
|
|
||
| // Port mapping patterns with regex boundaries | ||
| // Match: "- 8080:8080" or "- "8080:8080"" or "- '8080:8080'" at start of list item |
|
@claude[agent] Fresh Copilot follow-up review landed with one substantive fix request plus one output-consistency follow-up. Please address these before the next review pass:
Please push the follow-up commit, then I’ll trigger another Copilot review/check pass. |
There was a problem hiding this comment.
Pull request overview
Updates local/azure deployment UX and documentation, primarily to clarify default startup behavior and add resilient handling of local port conflicts (single retry onto an alternate port bundle).
Changes:
- Add bounded local port-fallback (8080/3002/4000 → 8085/3004/4004) with compose rewriting + improved port-conflict classification.
- Infer/print companion service URLs (Grafana/Config UI) based on the compose configuration and/or backend URL.
- Refresh docs/README for the new port bundle behavior and add targeted unit tests for the new helpers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/status.md | Documents companion URL inference and alternate-bundle behavior for status. |
| docs/state-files.md | Updates discovery chain to include localhost 8085. |
| docs/deploy.md | Updates deploy local docs for --start, --source, and port-fallback resilience; updates Azure resilience notes. |
| cmd/deploy_local_test.go | Adds tests for compose port rewriting, port bundle detection, and URL inference fallback. |
| cmd/deploy_local.go | Implements bounded port fallback, compose port rewrite helpers, and companion URL inference for printed endpoints. |
| cmd/deploy_errors_test.go | Adds unit tests for docker compose error classification and port extraction. |
| cmd/deploy_errors.go | Introduces docker compose error classification + friendly port-conflict remediation output. |
| cmd/deploy_azure.go | Improves bounded-recovery messaging (login/MySQL/Key Vault soft-delete). |
| README.md | Updates quick start endpoint guidance to reflect port bundles and fallback. |
docs/status.md
Outdated
| | ❌ | Connection refused or timeout | | ||
|
|
||
| Grafana is checked at `/api/health`. Backend and Config UI are checked at their root URL. | ||
| Grafana is checked at `/api/health`. Backend and Config UI are checked at their root URL. When auto-discovery lands on `http://localhost:8080`, companion URLs infer to `http://localhost:3002` and `http://localhost:4000`. When it lands on `http://localhost:8085`, companion URLs infer to `http://localhost:3004` and `http://localhost:4004`. |
cmd/deploy_local.go
Outdated
| // Not a port conflict or unknown error - print general cleanup and fail | ||
| fmt.Println("\n💡 To clean up partial artifacts:") | ||
| fmt.Println(" gh devlake cleanup --local --force") | ||
| return "", err |
|
@claude[agent] apply changes based on the comments in this thread |
- Update docs/status.md to correctly document Backend health check at /ping - Wrap non-port-conflict docker compose errors with context for consistency Co-authored-by: ewega <26189114+ewega@users.noreply.github.com> Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/de3f03b9-067f-4f4b-9e01-1b211ae5100c
Both review items addressed in 0df339b:
All tests pass. |
There was a problem hiding this comment.
Pull request overview
Updates local/azure deployment behavior and documentation to reflect recent PR feedback, including bounded recovery paths and more accurate service URL/port handling across commands.
Changes:
- Add port-conflict classification + user-friendly remediation output, with bounded auto-fallback to the alternate local port bundle for official/fork deploys.
- Infer Grafana/Config UI URLs from the active compose file (with fallback heuristics) and expand test coverage for port rewriting/bundle detection.
- Refresh docs/README to reflect
/pinghealth checks, localhost discovery on8080/8085, and the port-fallback behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/status.md | Clarifies health-check endpoints and documents companion URL inference for 8080 vs 8085 discovery. |
| docs/state-files.md | Updates discovery chain documentation to include localhost 8085. |
| docs/deploy.md | Rewrites deploy documentation to match --start default behavior and describes bounded recovery behaviors (local + Azure). |
| cmd/deploy_local_test.go | Adds tests for compose port rewriting, port bundle detection, compose-port extraction fallback, and command help/flags. |
| cmd/deploy_local.go | Implements bounded port-fallback flow, compose-based companion URL inference, and compose-port extraction via docker compose config. |
| cmd/deploy_errors_test.go | Adds unit tests for docker compose error classification and port extraction edge cases. |
| cmd/deploy_errors.go | Introduces docker compose error classification, port extraction, port-owner detection, and friendly conflict output. |
| cmd/deploy_azure.go | Adjusts output to explicitly label bounded recovery actions (login, MySQL start, Key Vault purge). |
| README.md | Updates local endpoint guidance to reflect printed bundle + one-time port fallback behavior. |
cmd/deploy_local.go
Outdated
| // composeFileHasDefaultPorts checks if a compose file contains the default port bundle (8080/3002/4000). | ||
| // Returns true only for the default bundle, and false for alternate or other custom port mappings. | ||
| func composeFileHasDefaultPorts(composePath string) bool { | ||
| return detectPortBundle(composePath) == portBundleDefault | ||
| } | ||
|
|
There was a problem hiding this comment.
composeFileHasDefaultPorts is introduced here but is never called anywhere in the repo. Keeping unused helpers in cmd/deploy_local.go makes the deploy flow harder to follow; consider removing it or using it in the port-fallback decision logic so the intent is clear.
| // composeFileHasDefaultPorts checks if a compose file contains the default port bundle (8080/3002/4000). | |
| // Returns true only for the default bundle, and false for alternate or other custom port mappings. | |
| func composeFileHasDefaultPorts(composePath string) bool { | |
| return detectPortBundle(composePath) == portBundleDefault | |
| } |
|
@claude[agent] please address fixes in this comment #145 (review) |
The function was never called; detectPortBundle is used directly instead. Co-authored-by: ewega <26189114+ewega@users.noreply.github.com> Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/661889e8-068e-46dc-88b3-7a2106030ce9
Removed unused |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
cmd/deploy_local.go:568
findComposeFilecan returndocker-compose-dev.yml, butstartLocalContainersultimately runsdocker compose upviainternal/docker.ComposeUp, which doesn’t pass-fand therefore won’t usedocker-compose-dev.yml. This makes custom deployments that only providedocker-compose-dev.ymlfail, despite being accepted earlier. Consider either (1) ensuring adocker-compose.ymlexists (copy/rename dev → yml) before calling ComposeUp, or (2) extending the docker helper to accept an explicit compose file path and always using the result offindComposeFile.
// findComposeFile returns the path to the active docker compose file in the given directory.
// Checks for docker-compose.yml first, then docker-compose-dev.yml.
func findComposeFile(dir string) string {
composePath := filepath.Join(dir, "docker-compose.yml")
if _, err := os.Stat(composePath); err == nil {
return composePath
}
return filepath.Join(dir, "docker-compose-dev.yml")
}
cmd/deploy_local.go
Outdated
| fmt.Println("\n💡 To clean up partial artifacts:") | ||
| fmt.Println(" gh devlake cleanup --local --force") | ||
| return "", err | ||
| nextSteps := " Edit your docker-compose.yml to use different host ports, or stop the conflicting container." |
There was a problem hiding this comment.
The remediation text hard-codes docker-compose.yml, but findComposeFile (and the earlier custom-mode validation) allow docker-compose-dev.yml too. To avoid sending users to the wrong file, consider referencing the detected compose file path (or saying “your compose file”) here.
This issue also appears on line 560 of the same file.
| nextSteps := " Edit your docker-compose.yml to use different host ports, or stop the conflicting container." | |
| nextSteps := " Edit your compose file to use different host ports, or stop the conflicting container." |
docs/status.md
Outdated
| | ❌ | Connection refused or timeout | | ||
|
|
||
| Grafana is checked at `/api/health`. Backend and Config UI are checked at their root URL. | ||
| Backend is checked at `/ping`, Grafana at `/api/health`, and Config UI at its root URL. When auto-discovery lands on `http://localhost:8080`, companion URLs infer to `http://localhost:3002` and `http://localhost:4000`. When it lands on `http://localhost:8085`, companion URLs infer to `http://localhost:3004` and `http://localhost:4004`. |
There was a problem hiding this comment.
The sentence listing inferred companion URLs doesn’t indicate which URL is Grafana vs Config UI, which can be confusing (it just lists two URLs per bundle). Consider explicitly labeling them (e.g., “Grafana → …, Config UI → …”) for both the 8080 and 8085 cases.
| Backend is checked at `/ping`, Grafana at `/api/health`, and Config UI at its root URL. When auto-discovery lands on `http://localhost:8080`, companion URLs infer to `http://localhost:3002` and `http://localhost:4000`. When it lands on `http://localhost:8085`, companion URLs infer to `http://localhost:3004` and `http://localhost:4004`. | |
| Backend is checked at `/ping`, Grafana at `/api/health`, and Config UI at its root URL. When auto-discovery lands on `http://localhost:8080`, companion URLs infer to Grafana → `http://localhost:3002`, Config UI → `http://localhost:4000`. When it lands on `http://localhost:8085`, companion URLs infer to Grafana → `http://localhost:3004`, Config UI → `http://localhost:4004`. |
docs/deploy.md
Outdated
| 5. Starts containers (unless `--start=false`) | ||
|
|
||
| **custom**: | ||
| 1. Uses the existing `docker-compose.yml` (or `docker-compose-dev.yml`) in the target directory |
There was a problem hiding this comment.
This section states that --source custom can use docker-compose-dev.yml, but the deploy code starts containers via docker compose up in the directory without selecting a compose file (no -f docker-compose-dev.yml). If the directory only contains docker-compose-dev.yml, deploy local will fail. Either document that custom requires docker-compose.yml, or update the implementation to use the detected compose file (e.g., pass -f or copy/rename dev → docker-compose.yml).
| 1. Uses the existing `docker-compose.yml` (or `docker-compose-dev.yml`) in the target directory | |
| 1. Uses the existing `docker-compose.yml` in the target directory (if you only have `docker-compose-dev.yml`, rename it to `docker-compose.yml` or start containers manually with `docker compose -f docker-compose-dev.yml up -d`) |
|
@claude[agent] apply changes based on the comments in this thread |
- Use generic "compose file" instead of hardcoded "docker-compose.yml" in error messages - Add explicit Grafana/Config UI labels to status docs URL mappings - Clarify that custom mode requires docker-compose.yml (not docker-compose-dev.yml) Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/5b89b918-ab13-4890-82b6-7f25c5e85ae2 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
All three review items addressed in afdb558:
|
cmd/deploy_errors_test.go
Outdated
| package cmd | ||
|
|
There was a problem hiding this comment.
This file appears to have Windows CRLF line endings (line 1 shows a trailing \r in searches). Please convert to LF to match the repository’s standard line endings and avoid cross-platform issues.
cmd/deploy_errors.go
Outdated
| func printDockerPortConflictError(de *DeployError, customHeader string, nextSteps string) { | ||
| // Print header | ||
| if customHeader != "" { | ||
| fmt.Println(customHeader) |
There was a problem hiding this comment.
When customHeader is provided, it’s printed verbatim. To keep terminal output spacing consistent (blank line before emoji-prefixed steps), consider normalizing customHeader inside this function (e.g., prefixing a newline if missing) rather than relying on all callers to include \n correctly.
| fmt.Println(customHeader) | |
| normalizedHeader := customHeader | |
| if !strings.HasPrefix(normalizedHeader, "\n") { | |
| normalizedHeader = "\n" + normalizedHeader | |
| } | |
| fmt.Println(normalizedHeader) |
cmd/deploy_local.go
Outdated
| if err := os.WriteFile(composePath, []byte(modified), 0644); err != nil { | ||
| return fmt.Errorf("writing compose file: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
rewriteComposePorts overwrites the user’s compose file in-place. Since this runs as part of an automated recovery path, consider writing a backup (e.g., docker-compose.yml.bak) and/or doing an atomic write (temp file + rename) so users can easily revert and to reduce risk of partial writes if the process is interrupted.
| if err := os.WriteFile(composePath, []byte(modified), 0644); err != nil { | |
| return fmt.Errorf("writing compose file: %w", err) | |
| } | |
| // Create a backup of the original compose file so users can revert if needed. | |
| backupPath := composePath + ".bak" | |
| if _, statErr := os.Stat(backupPath); os.IsNotExist(statErr) { | |
| if err := os.WriteFile(backupPath, data, 0644); err != nil { | |
| return fmt.Errorf("creating compose backup %s: %w", backupPath, err) | |
| } | |
| } | |
| // Write updated content atomically via temp file + rename to avoid partial writes. | |
| dir := filepath.Dir(composePath) | |
| tmpFile, err := os.CreateTemp(dir, ".compose-*.tmp") | |
| if err != nil { | |
| return fmt.Errorf("creating temp compose file: %w", err) | |
| } | |
| // Ensure temp file is removed on failure paths. | |
| defer os.Remove(tmpFile.Name()) | |
| if _, err := tmpFile.Write([]byte(modified)); err != nil { | |
| tmpFile.Close() | |
| return fmt.Errorf("writing temp compose file: %w", err) | |
| } | |
| if err := tmpFile.Close(); err != nil { | |
| return fmt.Errorf("closing temp compose file: %w", err) | |
| } | |
| if err := os.Rename(tmpFile.Name(), composePath); err != nil { | |
| return fmt.Errorf("replacing compose file atomically: %w", err) | |
| } |
docs/deploy.md
Outdated
| **Port Fallback**: When deploying with `--source official` or `--source fork`, the CLI automatically recovers from port conflicts by retrying with alternate ports (`8085/3004/4004`). Custom deployments require manual port conflict resolution. | ||
|
|
There was a problem hiding this comment.
The doc mentions port fallback, but it doesn’t note that the CLI rewrites docker-compose.yml port mappings on disk as part of the recovery. Please document that side effect (what gets rewritten, and how to revert) so users aren’t surprised when their compose file changes.
| **Port Fallback**: When deploying with `--source official` or `--source fork`, the CLI automatically recovers from port conflicts by retrying with alternate ports (`8085/3004/4004`). Custom deployments require manual port conflict resolution. | |
| **Port Fallback**: When deploying with `--source official` or `--source fork`, the CLI automatically recovers from port conflicts by retrying with alternate ports (`8085/3004/4004`). As part of this recovery, it updates the port mappings in `docker-compose.yml` on disk so that future `docker compose up` runs use the fallback ports. Custom deployments (`--source custom`) are not modified automatically and require manual port conflict resolution. | |
| To revert fallback changes, edit `docker-compose.yml` and restore your preferred host ports (for example, change the mapped ports back to `8080/3002/4000` if they were adjusted), or revert the file via version control / re-run the deployment in a clean directory. |
cmd/deploy_errors.go
Outdated
| package cmd | ||
|
|
There was a problem hiding this comment.
This file appears to have Windows CRLF line endings (e.g., lines show trailing \r in searches). The repo is predominantly LF; please convert this file to LF to avoid noisy diffs and potential tooling issues on Unix environments.
|
@claude[agent] please address comments in this thread: #145 (review) |
- Convert deploy_errors.go and deploy_errors_test.go from CRLF to LF - Normalize customHeader in printDockerPortConflictError for consistent spacing - Add backup (.bak) and atomic write (temp+rename) to rewriteComposePorts - Document port rewrite side effect and backup file in deploy.md Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/cbca78e8-02a6-4a80-b698-12d2fb258ca9 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
All five review items addressed in 78c70a3:
All tests pass. |
| // Checks for docker-compose.yml first, then docker-compose-dev.yml. | ||
| func findComposeFile(dir string) string { | ||
| composePath := filepath.Join(dir, "docker-compose.yml") | ||
| if _, err := os.Stat(composePath); err == nil { | ||
| return composePath | ||
| } | ||
| return filepath.Join(dir, "docker-compose-dev.yml") | ||
| } | ||
|
|
||
| // waitAndDetectBackendURL polls the backend URL extracted from the compose file. | ||
| // Falls back to probing both 8080 and 8085 if extraction fails. | ||
| func waitAndDetectBackendURL(dir string) (string, error) { | ||
| composePath := findComposeFile(dir) | ||
|
|
||
| // Try to extract the actual backend port from the compose file | ||
| ports := extractServicePorts(composePath, "devlake") | ||
| var backendURLCandidates []string | ||
|
|
There was a problem hiding this comment.
findComposeFile falls back to docker-compose-dev.yml, but the deploy path starts containers via internal/docker.ComposeUp, which runs docker compose up -d without a -f flag (so it only uses docker-compose.yml / compose.yml defaults). If docker-compose.yml is missing, deploy will still fail to start even though findComposeFile returns a path. Consider either (a) requiring/ensuring docker-compose.yml exists before starting containers (copy/rename dev file), or (b) extending ComposeUp to accept a compose filename and use -f consistently across start + port extraction.
| // Checks for docker-compose.yml first, then docker-compose-dev.yml. | |
| func findComposeFile(dir string) string { | |
| composePath := filepath.Join(dir, "docker-compose.yml") | |
| if _, err := os.Stat(composePath); err == nil { | |
| return composePath | |
| } | |
| return filepath.Join(dir, "docker-compose-dev.yml") | |
| } | |
| // waitAndDetectBackendURL polls the backend URL extracted from the compose file. | |
| // Falls back to probing both 8080 and 8085 if extraction fails. | |
| func waitAndDetectBackendURL(dir string) (string, error) { | |
| composePath := findComposeFile(dir) | |
| // Try to extract the actual backend port from the compose file | |
| ports := extractServicePorts(composePath, "devlake") | |
| var backendURLCandidates []string | |
| // For local deploys, we only consider docker-compose.yml, because docker.ComposeUp | |
| // runs "docker compose up" without a -f flag and therefore only uses default filenames. | |
| // If docker-compose.yml is not present, this returns an empty string. | |
| func findComposeFile(dir string) string { | |
| composePath := filepath.Join(dir, "docker-compose.yml") | |
| if _, err := os.Stat(composePath); err == nil { | |
| return composePath | |
| } | |
| return "" | |
| } | |
| // waitAndDetectBackendURL polls the backend URL extracted from the compose file. | |
| // Falls back to probing both 8080 and 8085 if extraction fails or no compose file is found. | |
| func waitAndDetectBackendURL(dir string) (string, error) { | |
| composePath := findComposeFile(dir) | |
| var ports map[string]int | |
| if composePath != "" { | |
| // Try to extract the actual backend port from the compose file (when present) | |
| ports = extractServicePorts(composePath, "devlake") | |
| } else { | |
| // No compose file detected that Docker would use; rely on default port probing | |
| ports = map[string]int{} | |
| } | |
| var backendURLCandidates []string |
|
|
||
| ### Notes | ||
|
|
||
| - If `.env` already exists in the target directory, it is backed up to `.env.bak` before being replaced. |
There was a problem hiding this comment.
The note about always backing up .env to .env.bak before replacement doesn’t match current behavior across all --source modes: fork/custom flows can write/update .env (e.g., ENCRYPTION_SECRET injection) without creating a backup, and fork cloning can overwrite an existing .env. Consider tightening the doc to the specific cases where a backup is actually created, or updating the implementation to consistently back up .env before any overwrite.
| - If `.env` already exists in the target directory, it is backed up to `.env.bak` before being replaced. | |
| - Deployments may overwrite an existing `.env` without creating a `.env.bak` backup — back up your `.env` manually if you have important local changes. |
|
@claude Please address these review comments:
|
|
@claude[agent] Please address these review comments:
|
Restructured "After Running" section to lead with the default auto-start behavior. Manual docker compose up -d is now clearly framed as the exception case when using --start=false. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Fixed in 30e443a. The "After Running" section now leads with the default auto-start behavior, and manual
Fixed in 30e443a. The "After Running" section now leads with the default auto-start behavior, and manual |
| // Write updated content atomically via temp file + rename to avoid partial writes | ||
| dir := filepath.Dir(composePath) | ||
| tmpFile, err := os.CreateTemp(dir, ".compose-*.tmp") | ||
| if err != nil { | ||
| return fmt.Errorf("creating temp compose file: %w", err) | ||
| } | ||
| // Ensure temp file is removed on failure paths | ||
| defer os.Remove(tmpFile.Name()) | ||
|
|
||
| if _, err := tmpFile.Write([]byte(modified)); err != nil { | ||
| tmpFile.Close() | ||
| return fmt.Errorf("writing temp compose file: %w", err) | ||
| } | ||
|
|
||
| if err := tmpFile.Close(); err != nil { | ||
| return fmt.Errorf("closing temp compose file: %w", err) | ||
| } | ||
|
|
||
| if err := os.Rename(tmpFile.Name(), composePath); err != nil { | ||
| return fmt.Errorf("replacing compose file atomically: %w", err) | ||
| } |
There was a problem hiding this comment.
rewriteComposePorts writes the updated compose content via os.CreateTemp + os.Rename, but os.CreateTemp creates the temp file with 0600 permissions. After rename, docker-compose.yml will inherit those permissions, potentially changing the original file mode unexpectedly. Preserve the original compose file permissions (e.g., stat the existing file and chmod the temp file to the same mode before renaming, or write the temp file with the original perms).
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.