Open
Conversation
8 tasks
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
Contributor
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. |
Comment on lines
+179
to
+197
| // 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
Comment on lines
+468
to
+479
| 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
Comment on lines
+541
to
+565
| // 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
Comment on lines
+252
to
+256
| 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.") |
Contributor
|
@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. |
Contributor
|
@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>
Contributor
Author
All findings addressed in e358254:
All tests pass, checks green. |
Claude
AI
changed the title
Contributor
|
@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. |
Contributor
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
Comment on lines
+502
to
+503
| fmt.Println() | ||
| fmt.Println("❌ Alternate ports are also in use.") |
cmd/deploy_local.go
Outdated
Comment on lines
+481
to
+490
| // 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 |
Contributor
|
@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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates deployment UX/documentation and adds more robust handling/testing around Docker port conflicts and Azure “bounded recovery” flows.
Changes:
- Document
deploy localsources (official/fork/custom),--start, and port-fallback behavior; expand Azure resilience docs. - Add port-conflict classification + remediation helper and update local deploy to do a single bounded retry with alternate ports.
- Add/expand tests for compose port rewriting, port bundle detection, service port extraction, and error classification.
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 | Updates docs to reflect new flags, default auto-start behavior, and resilience/port-fallback behavior. |
| cmd/deploy_local.go | Adds bounded port-fallback logic, compose port rewriting/detection, service port extraction, and companion URL inference. |
| cmd/deploy_local_test.go | Adds tests covering port rewrite/detection and service port extraction behaviors. |
| cmd/deploy_errors.go | Introduces docker-compose port conflict classification, port extraction, port owner lookup, and user-facing remediation output. |
| cmd/deploy_errors_test.go | Adds unit tests for error classification and port extraction patterns (including non-ASCII prefix case). |
| cmd/deploy_azure.go | Refines Azure bounded-recovery terminal output for login/MySQL/Key Vault recovery flows. |
Comments suppressed due to low confidence (1)
cmd/deploy_local.go:496
- In the
portBundleAlternatebranch, the function prints an alternate-ports conflict header and then callsprintDockerPortConflictError, which prints its own❌ Port conflict detected...header. This leads to duplicated/conflicting output for the same failure. Consider lettingprintDockerPortConflictErrorfully own the header, or add a flag/variant to print only the remediation details here.
fmt.Println("\n❌ Port conflict detected on alternate ports (8085/3004/4004)")
if deployErr.Port != "" {
fmt.Printf(" Port %s is in use", deployErr.Port)
if deployErr.Container != "" {
fmt.Printf(" by container: %s", deployErr.Container)
}
fmt.Println()
}
printDockerPortConflictError(deployErr)
fmt.Println("\n The alternate port bundle is already in use.")
cmd/deploy_local.go
Outdated
Comment on lines
+500
to
+511
| case portBundleCustom: | ||
| // Custom ports - don't attempt automatic rewrite | ||
| fmt.Println("\n❌ Port conflict detected on custom ports") | ||
| if deployErr.Port != "" { | ||
| fmt.Printf(" Port %s is in use", deployErr.Port) | ||
| if deployErr.Container != "" { | ||
| fmt.Printf(" by container: %s", deployErr.Container) | ||
| } | ||
|
|
||
| // Ask Docker which container owns the port | ||
| conflictCmd := "" | ||
| if port != "" { | ||
| out, dockerErr := exec.Command( | ||
| "docker", | ||
| "ps", | ||
| "--filter", | ||
| "publish="+port, | ||
| "--format", | ||
| "{{.Names}}\t{{.Label \"com.docker.compose.project.config_files\"}}\t{{.Label \"com.docker.compose.project.working_dir\"}}", | ||
| ).Output() | ||
| if dockerErr == nil && len(strings.TrimSpace(string(out))) > 0 { | ||
| lines := strings.Split(strings.TrimSpace(string(out)), "\n") | ||
| // Use the first match | ||
| parts := strings.SplitN(lines[0], "\t", 3) | ||
| containerName := parts[0] | ||
| configFiles := "" | ||
| workDir := "" | ||
| if len(parts) >= 2 { | ||
| configFiles = strings.TrimSpace(parts[1]) | ||
| } | ||
| if len(parts) == 3 { | ||
| workDir = strings.TrimSpace(parts[2]) | ||
| } | ||
| fmt.Printf(" Container holding the port: %s\n", containerName) | ||
| // Prefer the exact compose file path Docker recorded (most reliable). | ||
| if configFiles != "" { | ||
| configFile := strings.Split(configFiles, ";")[0] | ||
| configFile = strings.TrimSpace(configFile) | ||
| if configFile != "" { | ||
| if _, statErr := os.Stat(configFile); statErr == nil { | ||
| fmt.Println("\n Stop it with:") | ||
| fmt.Printf(" docker compose -f \"%s\" down\n", configFile) | ||
| conflictCmd = fmt.Sprintf("docker compose -f \"%s\" down", configFile) | ||
| } else { | ||
| fmt.Println("\n Stop it with:") | ||
| fmt.Printf(" docker stop %s\n", containerName) | ||
| fmt.Printf("\n ⚠️ Compose file not found at: %s\n", configFile) | ||
| fmt.Println(" (It may have been moved/deleted since the container was created.)") | ||
| conflictCmd = "docker stop " + containerName | ||
| } | ||
| } | ||
| } else if workDir != "" { | ||
| // Fallback for older Docker versions: assume docker-compose.yml under working_dir. | ||
| composePath := filepath.Join(workDir, "docker-compose.yml") | ||
| if _, statErr := os.Stat(composePath); statErr == nil { | ||
| fmt.Println("\n Stop it with:") | ||
| fmt.Printf(" docker compose -f \"%s\" down\n", composePath) | ||
| conflictCmd = fmt.Sprintf("docker compose -f \"%s\" down", composePath) | ||
| } | ||
| } | ||
| if conflictCmd == "" { | ||
| fmt.Println("\n Stop it with:") | ||
| fmt.Printf(" docker stop %s\n", containerName) | ||
| conflictCmd = "docker stop " + containerName | ||
| } | ||
| } | ||
| fmt.Println() | ||
| } | ||
| printDockerPortConflictError(deployErr) | ||
| return "", fmt.Errorf("port conflict on custom ports") |
Comment on lines
+468
to
+472
| if !allowPortFallback { | ||
| // Custom deployments don't get auto-fallback - print friendly error | ||
| printDockerPortConflictError(deployErr) | ||
| return "", fmt.Errorf("port conflict — stop the conflicting container and retry") | ||
| } |
cmd/deploy_errors.go
Outdated
Comment on lines
+281
to
+282
| fmt.Println(" Then re-run:") | ||
| fmt.Println(" gh devlake deploy local") |
Comment on lines
+1
to
+5
| package cmd | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" |
Comment on lines
+1
to
+6
| package cmd | ||
|
|
||
| import ( | ||
| "errors" | ||
| "testing" | ||
| ) |
Contributor
|
@claude[agent] I reviewed the latest Copilot cycle on commit
The CRLF line-ending comments on the two Go files are noted, but I am not treating them as blockers for this cycle. Please push a follow-up commit for the 2 items above, keep checks green, and then I’ll request one more fresh Copilot pass. |
…eserve error context Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Contributor
Author
Both actionable items addressed in 9d996ca:
All tests pass. |
Contributor
|
@copilot review the latest updates on this PR, focusing only on commit |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates local/azure deployment UX and docs, primarily by adding bounded port-conflict recovery for deploy local (default → alternate port bundle) and clarifying endpoint discovery behavior across commands.
Changes:
- Add Docker Compose port-conflict classification + user-friendly remediation output, and implement a single automatic retry using alternate local ports for
deploy local(official/fork only). - Improve endpoint reporting by inferring Grafana/Config UI URLs from the compose file (with fallback) and update docs/README to describe both port bundles.
- Add/expand test coverage for port rewriting, port bundle detection, companion URL inference, and Docker error parsing/classification.
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 adds example output for the alternate localhost bundle. |
| docs/state-files.md | Updates discovery docs to include localhost 8085 as a well-known fallback. |
| docs/deploy.md | Refreshes deploy local/deploy azure docs (start-by-default, source modes, bounded recovery, port fallback). |
| cmd/deploy_local.go | Implements bounded port fallback + companion URL inference; adds compose port parsing/rewriting helpers. |
| cmd/deploy_local_test.go | Adds tests for compose port rewriting/detection and companion URL inference; validates flags/help text. |
| cmd/deploy_errors.go | Introduces Docker Compose error classification, port extraction, port-owner lookup, and friendly port-conflict output. |
| cmd/deploy_errors_test.go | Adds unit tests for port extraction and Docker Compose error classification patterns. |
| cmd/deploy_azure.go | Updates Azure deploy output to reflect bounded recovery phases (auto-login, MySQL start, Key Vault purge). |
| README.md | Updates quickstart guidance and endpoint table to include the alternate local port bundle. |
Comments suppressed due to low confidence (2)
cmd/deploy_local.go:516
nextStepsincludes a leading\n, which introduces a blank line before the indented follow-up lines and violates the repo’s terminal output spacing rules (sub-items should not start with\n). Remove the leading newline and keep the next-steps lines tight under the header.
nextSteps := "\n Edit your docker-compose.yml to use different host ports, or stop the conflicting container."
printDockerPortConflictError(deployErr, header, nextSteps)
return "", fmt.Errorf("port conflict on custom ports: %w", err)
cmd/deploy_local.go:548
nextStepsis constructed with a leading\n, which results in an extra blank line when printed and breaks the “sub-items stay tight under their parent” terminal output convention. Prefer buildingnextStepswithout a leading newline and print blank lines intentionally at the call site if needed.
header := "\n❌ Alternate ports are also in use."
nextSteps := "\n Both default (8080/3002/4000) and alternate (8085/3004/4004) port bundles are occupied.\n Free at least one bundle, then retry deployment."
printDockerPortConflictError(retryErr, header, nextSteps)
cmd/deploy_local.go
Outdated
Comment on lines
+500
to
+501
| nextSteps := "\n The alternate port bundle is already in use.\n Free ports 8085/3004/4004, then retry deployment." | ||
| printDockerPortConflictError(deployErr, header, nextSteps) |
cmd/deploy_local.go
Outdated
| } | ||
|
|
||
| // composeFileHasDefaultPorts checks if a compose file contains the default port bundle (8080/3002/4000). | ||
| // Returns true if any of the default ports are found, false if the file is already on alternate ports. |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates local deployment behavior and documentation to incorporate bounded recovery paths (notably port-fallback for Docker Compose) and improves test coverage around the new error/port-handling logic.
Changes:
- Add bounded local port-fallback (8080/3002/4000 → 8085/3004/4004) with richer port-conflict classification and companion-URL inference.
- Add/expand tests for port rewriting, port-bundle detection, companion URL inference, and docker error classification.
- Update docs/README to reflect
deploy local --startdefaults, port bundles, and recovery behavior; refine Azure deploy recovery messaging.
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 | Documents alternate localhost bundle and companion URL inference behavior. |
| docs/state-files.md | Updates discovery-chain docs to include localhost:8085. |
| docs/deploy.md | Updates deploy local/azure docs for --start, --source, endpoints, and bounded recovery. |
| cmd/deploy_local.go | Implements port-fallback flow, compose port extraction, and companion URL inference. |
| cmd/deploy_local_test.go | Adds tests for compose-port rewriting, bundle detection, command flags, and companion URL inference. |
| cmd/deploy_errors.go | Adds docker-compose error classification, port extraction, and friendly port-conflict output. |
| cmd/deploy_errors_test.go | Adds unit tests for port parsing and compose error classification patterns. |
| cmd/deploy_azure.go | Refines bounded-recovery output for az login, MySQL stopped servers, and Key Vault soft-delete conflicts. |
| README.md | Updates local endpoint documentation to reflect port-fallback bundles. |
Comments suppressed due to low confidence (1)
cmd/deploy_local.go:570
waitAndDetectBackendURLhardcodesdocker-compose.ymlwhen extracting the backend port. This has the same mismatch as the success summary: if the deployment is using a different compose filename (e.g.docker-compose-dev.ymlin custom mode), backend-port extraction will always fail and you’ll fall back to probing 8080/8085, which can be wrong for custom ports. Consider passing the resolved compose path into this helper (or having it discover the active compose file consistently).
composePath := filepath.Join(dir, "docker-compose.yml")
// Try to extract the actual backend port from the compose file
ports := extractServicePorts(composePath, "devlake")
var backendURLCandidates []string
cmd/deploy_local.go
Outdated
| fmt.Println(" Config UI: http://localhost:4000") | ||
| fmt.Println(" Grafana: http://localhost:3002 (admin/admin)") | ||
| // Infer companion URLs based on compose file ports | ||
| composePath := filepath.Join(absDir, "docker-compose.yml") |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates local/azure deployment UX and documentation, primarily by formalizing port-conflict detection/recovery and improving endpoint/health reporting guidance across the CLI docs.
Changes:
- Add docker-compose port-conflict classification + user-friendly remediation output, with bounded retry to an alternate local port bundle.
- Infer and print companion service URLs (Grafana/Config UI) based on compose configuration, plus expanded test coverage for port rewrite/bundle detection and error parsing.
- Refresh docs/README to reflect
deploy localdefaults (--start=true), alternate port bundle support, and resilience behaviors.
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 | Documents companion URL inference and status output for alternate localhost bundle. |
| docs/state-files.md | Updates discovery chain to include localhost:8085 and clarifies state-file behavior. |
| docs/deploy.md | Rewrites deploy local/deploy azure docs to reflect current flags, defaults, endpoints, and bounded recovery behaviors. |
| README.md | Updates quick-start endpoint guidance for default + alternate local port bundles. |
| cmd/deploy_local.go | Implements port-conflict classification, bounded port fallback via compose rewrite, and companion URL inference via compose parsing. |
| cmd/deploy_local_test.go | Adds unit tests for compose port rewriting, bundle detection, compose parsing fallbacks, and deploy local command help/flags. |
| cmd/deploy_errors.go | Introduces docker-compose error classification, port extraction, container/compose ownership detection, and standardized conflict messaging. |
| cmd/deploy_errors_test.go | Adds tests for port extraction and compose-error classification patterns. |
| cmd/deploy_azure.go | Improves bounded recovery messaging for Azure login/MySQL/Key Vault soft-delete scenarios. |
Comments suppressed due to low confidence (1)
cmd/deploy_local.go:569
waitAndDetectBackendURLalways tries to parse ports fromdocker-compose.yml. If the deployment is using a different compose filename (e.g.,docker-compose-dev.yml), port extraction will silently fail and the readiness probe may miss the actual backend port. Consider passing the resolved compose path into this function (or doing the same filename fallback here) so readiness checks are aligned with the compose file that was started/re-written.
func waitAndDetectBackendURL(dir string) (string, error) {
composePath := filepath.Join(dir, "docker-compose.yml")
// Try to extract the actual backend port from the compose file
ports := extractServicePorts(composePath, "devlake")
cmd/deploy_local.go
Outdated
| fmt.Println(" Config UI: http://localhost:4000") | ||
| fmt.Println(" Grafana: http://localhost:3002 (admin/admin)") | ||
| // Infer companion URLs based on compose file ports | ||
| composePath := filepath.Join(absDir, "docker-compose.yml") |
Contributor
|
@claude[agent] Please address the remaining actionable review feedback on this PR before merge:
Please push the fixes to this PR branch and keep the existing scope unchanged. |
- Add findComposeFile() helper to detect docker-compose.yml vs docker-compose-dev.yml - Update waitAndDetectBackendURL, inferCompanionURLs, and startLocalContainers to use findComposeFile() - Fix docs/deploy.md to consistently describe auto-start behavior across all sources - Clarify custom source accepts docker-compose-dev.yml Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Contributor
Author
All four items addressed in 11ef152:
All tests pass ( |
Contributor
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. |
| | ❌ | 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`. |
| // 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 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.