Addressing PR comments#145
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
| | 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. |
| // 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 | ||
| } |
| 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)...") | ||
|
|
| // 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) | ||
| } |
| 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...)
| fmt.Println() | ||
| fmt.Println("❌ Alternate ports are also in use.") |
| // 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) |
| 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
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")
}
| 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." |
| | ❌ | 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`. |
| 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:
|
| 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.
| 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) |
| 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) | |
| } |
| **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. |
| 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).
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:21
- Import grouping/order doesn’t match the repo convention (stdlib → external → internal with blank lines).
github.com/spf13/cobrais currently grouped after internal imports; move it into the external section above the internal packages.
import (
"encoding/json"
"fmt"
"os"
"os/exec"
"path/filepath"
"regexp"
"strconv"
"strings"
"time"
"github.com/DevExpGBB/gh-devlake/internal/devlake"
dockerpkg "github.com/DevExpGBB/gh-devlake/internal/docker"
"github.com/DevExpGBB/gh-devlake/internal/download"
"github.com/DevExpGBB/gh-devlake/internal/gitclone"
"github.com/DevExpGBB/gh-devlake/internal/prompt"
"github.com/DevExpGBB/gh-devlake/internal/secrets"
"github.com/spf13/cobra"
)
| // 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 "", fmt.Errorf("docker compose up failed: %w", err) |
There was a problem hiding this comment.
This error path wraps an error that already includes the prefix "docker compose up failed", which can lead to duplicated text like "docker compose up failed: docker compose up failed: ...". Consider changing the wrapper context (e.g., "starting containers"/"running docker compose") or returning err directly here.
| return "", fmt.Errorf("docker compose up failed: %w", err) | |
| return "", fmt.Errorf("starting containers with docker compose: %w", err) |
| // 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") | ||
| } |
There was a problem hiding this comment.
findComposeFile is documented as returning the “active” compose file, but dockerpkg.ComposeUp runs docker compose up without a -f flag (so only the default compose filename is truly “active”). Consider updating the comment/name to clarify this is for introspection (ports/UX output) and that docker-compose-dev.yml may not be used by docker compose up unless explicitly specified.
| ``` | ||
|
|
||
| After setup, open Grafana at **http://localhost:3002** (admin / admin). DORA and Copilot dashboards will populate after the first sync completes. | ||
| After setup, open the URL bundle printed by `gh devlake deploy local`. Local deploys normally use `8080/4000/3002`, and automatically fall back once to `8085/4004/3004` when the default ports are already in use. DORA and Copilot dashboards will populate after the first sync completes. |
There was a problem hiding this comment.
The port bundle ordering here (8080/4000/3002 and 8085/4004/3004) is inconsistent with other docs/help text that use 8080/3002/4000 and 8085/3004/4004. Consider using the same ordering everywhere to reduce user confusion.
| After setup, open the URL bundle printed by `gh devlake deploy local`. Local deploys normally use `8080/4000/3002`, and automatically fall back once to `8085/4004/3004` when the default ports are already in use. DORA and Copilot dashboards will populate after the first sync completes. | |
| After setup, open the URL bundle printed by `gh devlake deploy local`. Local deploys normally use `8080/3002/4000`, and automatically fall back once to `8085/3004/4004` when the default ports are already in use. DORA and Copilot dashboards will populate after the first sync completes. |
|
@claude Please address the Code Review Agent findings from the latest review cycle:
Validation: |
|
@claude[agent] Please address the Code Review Agent findings from the latest review cycle on this PR:
Validation: |
Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/8ded09bd-a9d0-481b-a8e6-106e1098e6c9 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
All three findings addressed in 5c40e3a:
All three findings addressed in 5c40e3a:
|
| // Checks for docker-compose.yml first, then docker-compose-dev.yml. | ||
| // Note: docker.ComposeUp runs "docker compose up" without -f, so only docker-compose.yml | ||
| // (or compose.yml) is used at runtime. This function is for port extraction and UX output. | ||
| 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") |
| 1. Uses the existing `docker-compose.yml` in the target directory | ||
| 2. Generates or updates `.env` with `ENCRYPTION_SECRET` if needed | ||
| 3. Checks that Docker is available | ||
| 4. Starts containers (unless `--start=false`) | ||
|
|
||
| **Note**: The compose file must be named `docker-compose.yml`. If you only have `docker-compose-dev.yml`, either rename it or start containers manually with `docker compose -f docker-compose-dev.yml up -d`. |
|
@claude[agent] Please address the Code Review Agent findings from the latest review cycle:
Validation: |
- findComposeFile now checks compose.yaml, compose.yml, docker-compose.yaml, docker-compose.yml (in Docker's auto-detection order), plus docker-compose-dev.yml as a dev fallback for introspection - Port detection, rewriting, and URL inference now work correctly for users with compose.yml or other standard filenames - Added error handling when no compose file is found during port conflict recovery - Updated docs/deploy.md to document all supported compose filenames and clarify which ones can be auto-modified for port fallback - Updated docs/start.md and docs/stop.md to reflect compose file auto-detection order - Added comprehensive test coverage for findComposeFile precedence rules Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/d29dab8a-3634-4691-af39-92a2ff04e77e Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Both findings addressed in 5109932:
Added comprehensive test coverage for filename precedence. All validation passes. |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" |
| if err := os.Rename(tmpFile.Name(), composePath); err != nil { | ||
| return fmt.Errorf("replacing compose file atomically: %w", err) | ||
| } |
| // Checks for compose files in the order Docker Compose auto-detects them: | ||
| // compose.yaml → compose.yml → docker-compose.yaml → docker-compose.yml → docker-compose-dev.yml (dev fallback). | ||
| // Note: docker.ComposeUp runs "docker compose up" without -f, so it uses Docker's default | ||
| // auto-detection order. This function is for port extraction, port rewriting, and UX output. | ||
| func findComposeFile(dir string) string { | ||
| // Check in the same order Docker Compose auto-detects compose files | ||
| candidates := []string{ | ||
| "compose.yaml", | ||
| "compose.yml", | ||
| "docker-compose.yaml", | ||
| "docker-compose.yml", | ||
| "docker-compose-dev.yml", // Dev fallback for introspection |
| 2. `.devlake-azure.json` → Azure mode | ||
| 3. `.devlake-local.json` → Local mode | ||
| 4. `docker-compose.yml` in current directory → Local mode | ||
| 4. Standard compose file in current directory → Local mode (searches for `compose.yaml`, `compose.yml`, `docker-compose.yaml`, `docker-compose.yml` in that order) |
| 2. `.devlake-azure.json` → Azure mode | ||
| 3. `.devlake-local.json` → Local mode | ||
| 4. `docker-compose.yml` in current directory → Local mode | ||
| 4. Standard compose file in current directory → Local mode (searches for `compose.yaml`, `compose.yml`, `docker-compose.yaml`, `docker-compose.yml` in that order) |
|
@claude[agent] Round 2 — Code Review Agent found new issues introduced by the previous fix:
Validation: |
…omments Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/c0a37cc2-e116-42c3-b9cc-7553ae446093 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
All five findings addressed in 669efe4:
All validation passed ( |
| func inferCompanionURLs(backendURL string, composePath string) (grafanaURL, configUIURL string) { | ||
| // Try to extract actual ports from compose file | ||
| ports := extractServicePorts(composePath, "grafana", "config-ui") | ||
|
|
||
| if grafanaPort, ok := ports["grafana"]; ok { |
| // On Windows, os.Rename fails if destination exists, so remove it first | ||
| if err := os.Remove(composePath); err != nil { | ||
| return fmt.Errorf("removing original compose file: %w", err) | ||
| } | ||
|
|
||
| if err := os.Rename(tmpPath, composePath); err != nil { | ||
| return fmt.Errorf("replacing compose file atomically: %w", err) | ||
| } | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
💡 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.