Conversation
- Add detection of existing local deployments with resume/restart/abort options - Add detection of existing Azure deployments with intelligent recovery - Preserve existing ENCRYPTION_SECRET and .env files when resuming - Improve Docker availability error with step-by-step recovery guide - Enhance DevLake readiness timeout error with troubleshooting steps - Improve Azure login and Bicep deployment failure messages - Export PingURL function for deployment status checks Co-authored-by: ewega <26189114+ewega@users.noreply.github.com> Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/40eca0b0-6be0-4722-8bd1-148642db1d45
Claude
AI
changed the title
There was a problem hiding this comment.
Pull request overview
Adds recovery/resume detection to deploy local and deploy azure runs when deployment artifacts/state already exist, and upgrades deployment error output to include actionable recovery steps, improving UX for reruns after partial failures.
Changes:
- Exported a reusable
devlake.PingURL()reachability helper for deployment status checks. - Added existing-deployment detection + resume/restart/abort prompts for both local and Azure deploy flows.
- Enhanced deploy-time error messages (Docker availability, readiness timeout, Azure login/Bicep failures) with structured troubleshooting guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/devlake/discovery.go | Exposes PingURL() so commands can reuse the backend /ping health check. |
| cmd/deploy_local.go | Adds local artifact/state detection with resume/restart/abort, adds quiet cleanup, and improves Docker/readiness error messaging. |
| cmd/deploy_azure.go | Adds Azure state detection with resume/restart/abort and improves Azure login/Bicep failure guidance. |
Comments suppressed due to low confidence (2)
cmd/deploy_azure.go:501
- This treats any os.ReadFile error as “no state file found”. For permission errors or other I/O issues, that will silently skip the recovery prompt and proceed as if there’s no existing deployment. Consider checking os.IsNotExist(err) to ignore missing files, but warning/returning for other errors.
// Check for state file
data, err := os.ReadFile(stateFile)
if err != nil {
// No state file found - proceed normally
return nil, ""
}
cmd/deploy_local.go:588
- detectExistingLocalDeployment treats any LoadState error (corrupt JSON, permission error, etc.) the same as “no state file” and doesn’t surface that an error occurred. This can hide actionable problems and also changes the control flow into the artifacts-only path. Consider handling os.IsNotExist separately (no state) and printing a warning (or returning an error) for other LoadState failures, similar to detectExistingAzureDeployment’s parse warning behavior.
// Check for state file
state, err := devlake.LoadState(stateFile)
if err != nil || state == nil {
// No state file or failed to load - check for docker-compose.yml + .env
cmd/deploy_azure.go
Outdated
| fmt.Println("\n🧹 Cleaning up existing Azure deployment...") | ||
| fmt.Println(" Note: This will delete all Azure resources in the resource group") | ||
| if !prompt.Confirm("Proceed with cleanup?") { | ||
| return nil | ||
| } | ||
| // User should run cleanup manually - we don't want to auto-delete Azure resources | ||
| fmt.Println("\n Please run: gh devlake cleanup --azure") | ||
| fmt.Println(" Then re-run: gh devlake deploy azure") | ||
| return nil |
There was a problem hiding this comment.
In the Azure “restart” flow, the prompt asks “Proceed with cleanup?” but the command does not actually perform cleanup; it only prints manual instructions and returns. Consider rewording the prompt to reflect that this will show instructions / exit, or alternatively invoke the cleanup logic directly (if safe) after confirmation.
| // ── Check for existing deployment ── | ||
| if existingState, resumeAction := detectExistingLocalDeployment(deployLocalDir); existingState != nil { | ||
| switch resumeAction { | ||
| case "abort": | ||
| return nil | ||
| case "restart": | ||
| fmt.Println("\n🧹 Cleaning up existing deployment...") | ||
| if err := cleanupLocalQuiet(deployLocalDir); err != nil { | ||
| fmt.Printf(" ⚠️ Cleanup encountered issues: %v\n", err) | ||
| fmt.Println(" Continuing with deployment...") | ||
| } | ||
| case "resume": | ||
| // Continue with the deployment - existing artifacts will be reused | ||
| fmt.Println("\n Continuing with existing deployment artifacts...") | ||
| } | ||
| } |
There was a problem hiding this comment.
The existing-deployment prompt can be shown even when no state file is loaded (state==nil), but runDeployLocal only enters the switch when existingState != nil. In the common “artifacts exist but .devlake-local.json is missing/couldn’t load” case, the user’s resume/restart/abort choice is ignored and the deploy proceeds regardless. Consider changing the condition to key off a separate “foundArtifacts” flag or the returned action (e.g., resumeAction != "") so restart/abort are honored even without a state file.
This issue also appears on line 585 of the same file.
| if _, err := os.Stat(composePath); err == nil { | ||
| if err := dockerpkg.ComposeDown(absDir); err != nil { | ||
| return fmt.Errorf("docker compose down failed: %w", err) | ||
| } | ||
| } else if _, err := os.Stat(devComposePath); err == nil { | ||
| if err := dockerpkg.ComposeDown(absDir); err != nil { | ||
| return fmt.Errorf("docker compose down failed: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
cleanupLocalQuiet checks for docker-compose-dev.yml, but then calls dockerpkg.ComposeDown(absDir) without specifying the compose file. docker compose down won’t use docker-compose-dev.yml by default, so the restart cleanup path will fail when only docker-compose-dev.yml exists. Consider either (1) running docker compose -f docker-compose-dev.yml down in this branch, or (2) standardizing by renaming/copying docker-compose-dev.yml to docker-compose.yml before calling ComposeDown.
| if hasCompose || hasEnv { | ||
| fmt.Println("\n📋 Found existing deployment artifacts:") | ||
| if hasCompose { | ||
| fmt.Println(" • docker-compose.yml") | ||
| } | ||
| if hasEnv { | ||
| fmt.Println(" • .env file") | ||
| } |
There was a problem hiding this comment.
When the compose file found is docker-compose-dev.yml, the output still lists it as “docker-compose.yml”. This can mislead users about what file is actually present and which file to edit/run commands against. Consider tracking which compose filename was detected and printing the correct one.
| } | ||
|
|
||
| // PingURL checks if a DevLake backend is reachable at the given URL. | ||
| func PingURL(baseURL string) error { |
There was a problem hiding this comment.
PingURL forwards baseURL directly to pingURL, but pingURL always appends "/ping". If callers pass a baseURL with a trailing slash (common when URLs are copied/pasted), the request becomes "//ping" which can lead to redirects or 404s depending on the server/proxy. Consider normalizing with strings.TrimRight(baseURL, "/") inside PingURL (or pingURL) to match the normalization used in Discover().
| func PingURL(baseURL string) error { | |
| func PingURL(baseURL string) error { | |
| baseURL = strings.TrimRight(baseURL, "/") |
cmd/deploy_azure.go
Outdated
| stateFile := filepath.Join(absDir, ".devlake-azure.json") | ||
|
|
||
| // Check for state file | ||
| data, err := os.ReadFile(stateFile) | ||
| if err != nil { | ||
| // No state file found - proceed normally |
There was a problem hiding this comment.
detectExistingAzureDeployment only checks for .devlake-azure.json under deployAzureDir. However, the partial checkpoint writer (savePartialAzureState) writes to ".devlake-azure.json" in the current working directory, not deployAzureDir. If the user deploys with --dir != "." and the deployment fails mid-way, the resume/restart prompt won’t detect the partial state. Consider aligning both to the same directory (e.g., pass deployAzureDir into savePartialAzureState and write there) or have detection check both locations.
This issue also appears on line 496 of the same file.
| stateFile := filepath.Join(absDir, ".devlake-azure.json") | |
| // Check for state file | |
| data, err := os.ReadFile(stateFile) | |
| if err != nil { | |
| // No state file found - proceed normally | |
| // Look for .devlake-azure.json in both the deploy directory and the current working directory. | |
| candidatePaths := []string{filepath.Join(absDir, ".devlake-azure.json")} | |
| if cwd, err := os.Getwd(); err == nil { | |
| if cwdAbs, err := filepath.Abs(cwd); err == nil && cwdAbs != absDir { | |
| candidatePaths = append(candidatePaths, filepath.Join(cwdAbs, ".devlake-azure.json")) | |
| } | |
| } | |
| var ( | |
| data []byte | |
| err error | |
| ) | |
| for _, stateFile := range candidatePaths { | |
| data, err = os.ReadFile(stateFile) | |
| if err == nil { | |
| break | |
| } | |
| } | |
| if err != nil { | |
| // No state file found in any candidate location - proceed normally |
|
@claude[agent] apply changes based on the comments in this thread |
- Fix Azure restart prompt to not ask for cleanup confirmation when it doesn't perform cleanup - Fix local deployment detection to honor user choice even when only artifacts exist (no state file) - Fix docker-compose-dev.yml cleanup by explicitly using -f flag - Fix misleading compose file name display to show actual file found - Add trailing slash normalization in PingURL to match Discover() behavior - Fix Azure partial state path mismatch by writing to deployAzureDir consistently Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/66b97b76-9583-4ab4-826e-ade3b8a58ac1 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Applied all review feedback in commit 45a8fef:
|
Claude
AI
changed the title
cmd/deploy_azure.go
Outdated
| // Check for state file | ||
| data, err := os.ReadFile(stateFile) | ||
| if err != nil { | ||
| // No state file found - proceed normally |
There was a problem hiding this comment.
When os.ReadFile(stateFile) fails, this treats all errors as “no state file” and proceeds. For non-IsNotExist errors (e.g., permission issues), it would be safer to surface a warning or return an error, otherwise the deploy may continue without recovery context and may later fail again when attempting to write the same file.
| // No state file found - proceed normally | |
| if !os.IsNotExist(err) { | |
| fmt.Printf("\n⚠️ Could not read Azure state file %s: %v\n", stateFile, err) | |
| } | |
| // No state file found or unreadable - proceed without state |
cmd/deploy_local.go
Outdated
| fmt.Println(" 2. Start Docker Desktop and wait for it to fully initialize") | ||
| fmt.Println(" 3. Verify Docker is running: docker ps") | ||
| fmt.Println(" 4. Re-run this command: gh devlake deploy local") | ||
| return fmt.Errorf("Docker is not available — follow recovery steps above") |
There was a problem hiding this comment.
The Docker availability failure path drops the underlying error from dockerpkg.CheckAvailable(). Keeping a %w-wrapped error (while still printing the recovery steps) helps callers/debug logs distinguish “docker not installed” vs “daemon not running” vs other failures.
| return fmt.Errorf("Docker is not available — follow recovery steps above") | |
| return fmt.Errorf("Docker is not available — follow recovery steps above: %w", err) |
cmd/deploy_local.go
Outdated
| fmt.Println(" • MySQL takes longer on first run (database initialization)") | ||
| fmt.Println(" • Insufficient Docker resources (increase memory in Docker Desktop settings)") | ||
| fmt.Println(" • Port conflicts (check docker compose logs for 'address already in use')") | ||
| return "", fmt.Errorf("DevLake not ready — check logs for details") |
There was a problem hiding this comment.
The readiness-timeout return loses the underlying waitForReadyAny error (it previously carried details like the timeout/attempt count). Consider wrapping the original err with %w so the error retains diagnostic context in logs/tests while still printing the user-facing troubleshooting output above.
| return "", fmt.Errorf("DevLake not ready — check logs for details") | |
| return "", fmt.Errorf("DevLake not ready — check logs for details: %w", err) |
| stateFile := filepath.Join(absDir, ".devlake-local.json") | ||
|
|
||
| // Check for state file | ||
| state, err := devlake.LoadState(stateFile) |
There was a problem hiding this comment.
devlake.LoadState can fail due to JSON parse errors or permission issues; currently those cases are treated the same as “no state file” and no warning is shown. It’d be helpful to distinguish os.IsNotExist from other errors and surface a warning (or return an error for unreadable state) so users understand why recovery can’t use the state file.
| state, err := devlake.LoadState(stateFile) | |
| state, err := devlake.LoadState(stateFile) | |
| if err != nil && !os.IsNotExist(err) { | |
| fmt.Printf("\n⚠️ Unable to read local deployment state from %s: %v\n", stateFile, err) | |
| } |
cmd/deploy_local.go
Outdated
| // Remove state file | ||
| stateFile := filepath.Join(absDir, ".devlake-local.json") | ||
| if _, err := os.Stat(stateFile); err == nil { | ||
| os.Remove(stateFile) |
There was a problem hiding this comment.
os.Remove(stateFile) ignores its returned error. If the state file can’t be removed (permissions, read-only FS), the subsequent deploy may still be treated as “existing” on the next run. Consider checking and returning or at least warning on remove failures.
| os.Remove(stateFile) | |
| if err := os.Remove(stateFile); err != nil { | |
| fmt.Printf("\n⚠️ Failed to remove local state file %s: %v\n", stateFile, err) | |
| } |
cmd/deploy_local.go
Outdated
| fmt.Println(" gh devlake deploy local") | ||
| fmt.Println("\n💡 To clean up partial artifacts:") | ||
| fmt.Println(" gh devlake cleanup --local --force") | ||
| return "", fmt.Errorf("port conflict — stop the conflicting container and retry") |
There was a problem hiding this comment.
In the port-conflict branch, the returned error doesn’t wrap the original docker compose up error, which makes debugging harder and diverges from the pattern used elsewhere in this function (preserving the compose output/error). Consider returning a contextual error that includes %w with the original err.
| return "", fmt.Errorf("port conflict — stop the conflicting container and retry") | |
| return "", fmt.Errorf("port conflict — stop the conflicting container and retry: %w", err) |
cmd/deploy_local.go
Outdated
| fmt.Printf(" 1. Check container logs: docker compose -f \"%s/docker-compose.yml\" logs devlake\n", absDir) | ||
| fmt.Println(" 2. Verify all containers are running: docker compose ps") | ||
| fmt.Println(" 3. Check MySQL initialization: docker compose logs mysql") | ||
| fmt.Println(" 4. If containers keep restarting, check: docker compose logs") |
There was a problem hiding this comment.
The troubleshooting command hardcodes docker-compose.yml, but this command supports deployments where the compose file may be docker-compose-dev.yml (and the port-conflict guidance already handles arbitrary compose file paths). Prefer detecting which compose file exists (or tracking which file was used) and printing commands that target that file (or instruct cd into the target dir before running docker compose ...).
| fmt.Printf(" 1. Check container logs: docker compose -f \"%s/docker-compose.yml\" logs devlake\n", absDir) | |
| fmt.Println(" 2. Verify all containers are running: docker compose ps") | |
| fmt.Println(" 3. Check MySQL initialization: docker compose logs mysql") | |
| fmt.Println(" 4. If containers keep restarting, check: docker compose logs") | |
| composeFile := "docker-compose.yml" | |
| if _, err := os.Stat(filepath.Join(absDir, "docker-compose-dev.yml")); err == nil { | |
| composeFile = "docker-compose-dev.yml" | |
| } | |
| composePath := filepath.Join(absDir, composeFile) | |
| fmt.Printf(" 1. Check container logs: docker compose -f \"%s\" logs devlake\n", composePath) | |
| fmt.Printf(" 2. Verify all containers are running: docker compose -f \"%s\" ps\n", composePath) | |
| fmt.Printf(" 3. Check MySQL initialization: docker compose -f \"%s\" logs mysql\n", composePath) | |
| fmt.Printf(" 4. If containers keep restarting, check: docker compose -f \"%s\" logs\n", composePath) |
|
@claude[agent] Adress the comments in this thread #146 (review) |
- Add warning for non-IsNotExist errors when reading Azure state file - Wrap underlying errors with %w for Docker availability, readiness timeout, and port conflicts - Add warning for non-IsNotExist errors when loading local deployment state - Add warning when state file removal fails during cleanup - Detect actual compose file (docker-compose.yml vs docker-compose-dev.yml) in troubleshooting output Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/96938f2c-308a-42bb-a21d-6de79e11ddd1 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Applied all review feedback in commit cddbf92:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
cmd/deploy_local.go:685
cleanupLocalQuietloses error wrapping whendocker compose ... downfails fordocker-compose-dev.ymlbecause it formats the error with%sinstead of%w. Wrapping the original error makes troubleshooting and higher-level error handling consistent (and you can still include the command output in the message).
cmd := exec.Command("docker", "compose", "-f", devComposePath, "down", "--rmi", "local")
cmd.Dir = absDir
if out, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("docker compose down failed: %s\n%s", err, string(out))
}
cmd/deploy_azure.go:91
- The "resume" branch prints an indented line ("\n Continuing...") without a clear parent step line at column 0. To match the terminal output standards (steps at column 0, sub-items only under a parent and kept tight), consider emitting a proper step line (emoji-prefixed with leading \n) or removing the indent/leading newline.
case "resume":
// Continue with the deployment - may update existing resources
fmt.Println("\n Continuing with deployment (will update existing resources)...")
}
| // detectExistingLocalDeployment checks for existing deployment artifacts and prompts for action. | ||
| // Returns the existing state (if found) and the user's choice: "resume", "restart", or "abort". | ||
| func detectExistingLocalDeployment(dir string) (*devlake.State, string) { | ||
| if deployLocalQuiet { |
There was a problem hiding this comment.
The new existing-deployment detection/prompting logic isn’t covered by tests. Since cmd/deploy_local.go already has a test file (cmd/deploy_local_test.go), consider adding unit coverage for the artifact/state detection logic by splitting prompting from detection (e.g., return a structured detection result and inject the selection function) so it can be tested without interactive stdin.
This issue also appears on line 681 of the same file.
cmd/deploy_azure.go
Outdated
| fmt.Println("\n Please run: gh devlake cleanup --azure") | ||
| fmt.Println(" Then re-run: gh devlake deploy azure") | ||
| return nil | ||
| case "resume": | ||
| // Continue with the deployment - may update existing resources | ||
| fmt.Println("\n Continuing with deployment (will update existing resources)...") |
There was a problem hiding this comment.
In the restart branch, fmt.Println("\n Please run: ...") inserts a blank line between sub-items under the "🧹" step, which violates the "sub-items stay tight under their parent (no blank lines)" rule in .github/instructions/terminal-output.instructions.md. Consider removing the leading \n (or making it a separate top-level step at column 0).
This issue also appears on line 88 of the same file.
| fmt.Println("\n Please run: gh devlake cleanup --azure") | |
| fmt.Println(" Then re-run: gh devlake deploy azure") | |
| return nil | |
| case "resume": | |
| // Continue with the deployment - may update existing resources | |
| fmt.Println("\n Continuing with deployment (will update existing resources)...") | |
| fmt.Println(" Please run: gh devlake cleanup --azure") | |
| fmt.Println(" Then re-run: gh devlake deploy azure") | |
| return nil | |
| case "resume": | |
| // Continue with the deployment - may update existing resources | |
| fmt.Println("\n🚀 Continuing with deployment (will update existing resources)...") |
|
@claude Please address these review comments:
|
|
@claude[agent] Please address these review comments:
|
Remove leading newline from sub-item prints in Azure restart branch to comply with terminal output rules (sub-items stay tight under parent). Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/90bd3b50-b12a-4ade-83d8-2179478a7638 Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
cmd/deploy_local.go
Outdated
| cmd := exec.Command("docker", "compose", "-f", devComposePath, "down", "--rmi", "local") | ||
| cmd.Dir = absDir | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("docker compose down failed: %s\n%s", err, string(out)) |
There was a problem hiding this comment.
In cleanupLocalQuiet's docker-compose-dev.yml branch, the error returned from CombinedOutput isn't wrapped with %w (it uses %s), which breaks error chaining compared to other errors in this file. Consider wrapping the underlying error with %w and include the command output as additional context so callers can still unwrap the original error.
| return fmt.Errorf("docker compose down failed: %s\n%s", err, string(out)) | |
| return fmt.Errorf("docker compose down failed: %w\n%s", err, string(out)) |
| // detectExistingLocalDeployment checks for existing deployment artifacts and prompts for action. | ||
| // Returns the existing state (if found) and the user's choice: "resume", "restart", or "abort". | ||
| func detectExistingLocalDeployment(dir string) (*devlake.State, string) { | ||
| if deployLocalQuiet { | ||
| // When called from init wizard, don't prompt | ||
| return nil, "" | ||
| } | ||
|
|
||
| absDir, _ := filepath.Abs(dir) | ||
| stateFile := filepath.Join(absDir, ".devlake-local.json") | ||
|
|
||
| // Check for state file | ||
| state, err := devlake.LoadState(stateFile) | ||
| if err != nil && !os.IsNotExist(err) { | ||
| fmt.Printf("\n⚠️ Unable to read local deployment state from %s: %v\n", stateFile, err) | ||
| } | ||
| if err != nil || state == nil { | ||
| // No state file or failed to load - check for docker-compose.yml + .env | ||
| composePath := filepath.Join(absDir, "docker-compose.yml") | ||
| devComposePath := filepath.Join(absDir, "docker-compose-dev.yml") | ||
| envPath := filepath.Join(absDir, ".env") | ||
|
|
||
| hasCompose := false | ||
| composeFileName := "" | ||
| if _, err := os.Stat(composePath); err == nil { | ||
| hasCompose = true | ||
| composeFileName = "docker-compose.yml" | ||
| } else if _, err := os.Stat(devComposePath); err == nil { | ||
| hasCompose = true | ||
| composeFileName = "docker-compose-dev.yml" | ||
| } | ||
|
|
||
| hasEnv := false | ||
| if _, err := os.Stat(envPath); err == nil { | ||
| hasEnv = true | ||
| } | ||
|
|
||
| // If we have artifacts but no state file, it might be a partial deployment | ||
| if hasCompose || hasEnv { | ||
| fmt.Println("\n📋 Found existing deployment artifacts:") | ||
| if hasCompose { | ||
| fmt.Printf(" • %s\n", composeFileName) | ||
| } | ||
| if hasEnv { | ||
| fmt.Println(" • .env file") | ||
| } | ||
| } else { | ||
| // No artifacts found - proceed normally | ||
| return nil, "" | ||
| } | ||
| } else { | ||
| // State file exists - check if deployment is running | ||
| fmt.Println("\n📋 Found existing deployment:") | ||
| fmt.Printf(" Deployed: %s\n", state.DeployedAt) | ||
| if state.Endpoints.Backend != "" { | ||
| fmt.Printf(" Backend: %s\n", state.Endpoints.Backend) | ||
|
|
||
| // Check if backend is still running | ||
| if err := devlake.PingURL(state.Endpoints.Backend); err == nil { | ||
| fmt.Println(" Status: ✅ Running") | ||
| } else { | ||
| fmt.Println(" Status: ⚠️ Not responding (may be stopped)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fmt.Println() | ||
| choices := []string{ | ||
| "resume - Continue with existing artifacts (recommended for recovery)", | ||
| "restart - Clean up and start fresh", | ||
| "abort - Exit without making changes", | ||
| } | ||
| choice := prompt.Select("What would you like to do?", choices) | ||
| if choice == "" { | ||
| return state, "abort" | ||
| } | ||
|
|
||
| action := strings.SplitN(choice, " ", 2)[0] | ||
| return state, action | ||
| } | ||
|
|
||
| // cleanupLocalQuiet performs cleanup of local deployment without prompts (used for restart). | ||
| func cleanupLocalQuiet(dir string) error { | ||
| absDir, _ := filepath.Abs(dir) | ||
|
|
||
| // Stop containers if compose file exists | ||
| composePath := filepath.Join(absDir, "docker-compose.yml") | ||
| devComposePath := filepath.Join(absDir, "docker-compose-dev.yml") | ||
|
|
||
| if _, err := os.Stat(composePath); err == nil { | ||
| if err := dockerpkg.ComposeDown(absDir); err != nil { | ||
| return fmt.Errorf("docker compose down failed: %w", err) | ||
| } | ||
| } else if _, err := os.Stat(devComposePath); err == nil { | ||
| // For docker-compose-dev.yml, we need to run docker compose explicitly | ||
| // since ComposeDown expects docker-compose.yml by default | ||
| cmd := exec.Command("docker", "compose", "-f", devComposePath, "down", "--rmi", "local") | ||
| cmd.Dir = absDir | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("docker compose down failed: %s\n%s", err, string(out)) | ||
| } | ||
| } | ||
|
|
||
| // Remove state file | ||
| stateFile := filepath.Join(absDir, ".devlake-local.json") | ||
| if _, err := os.Stat(stateFile); err == nil { | ||
| if err := os.Remove(stateFile); err != nil { | ||
| fmt.Printf("\n⚠️ Failed to remove local state file %s: %v\n", stateFile, err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
New existing-deployment detection/cleanup behavior (detectExistingLocalDeployment + cleanupLocalQuiet) isn’t covered by tests. Since this package already has cmd/deploy_local_test.go, adding unit tests for the decision logic (artifact detection, state-file handling, action parsing) would help prevent regressions.
|
@claude[agent] One last fix:
|
Use %w instead of %s for error wrapping in docker-compose-dev.yml cleanup to maintain error chain consistency with project conventions. Agent-Logs-Url: https://github.com/DevExpGbb/gh-devlake/sessions/e30e7315-18a1-4487-a5ce-e9040332e4ae Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Fixed in f8ff86b — changed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
cmd/deploy_local.go:578
- The troubleshooting output inserts a blank line before an indented sub-item (
"\n Common issues:"). This creates uneven spacing inside the same troubleshooting block; either keep it as a tight sub-item (no leading newline) or promote it to its own step heading so the layout remains consistent.
fmt.Printf(" 1. Check container logs: docker compose -f \"%s\" logs devlake\n", composePath)
fmt.Printf(" 2. Verify all containers are running: docker compose -f \"%s\" ps\n", composePath)
fmt.Printf(" 3. Check MySQL initialization: docker compose -f \"%s\" logs mysql\n", composePath)
fmt.Printf(" 4. If containers keep restarting, check: docker compose -f \"%s\" logs\n", composePath)
fmt.Println("\n Common issues:")
fmt.Println(" • MySQL takes longer on first run (database initialization)")
fmt.Println(" • Insufficient Docker resources (increase memory in Docker Desktop settings)")
cmd/deploy_azure.go:331
- This troubleshooting hint (
gh devlake cleanup --azure, then deploy again) also assumes the user is in the directory containing.devlake-azure.json. If--dirwas used, suggestcd <dir>first or provide an explicit--state-file <dir>/.devlake-azure.json(or--resource-group) so the cleanup command works reliably.
fmt.Println("\n To retry:")
fmt.Println(" • If partial deployment exists, re-run will attempt to continue")
fmt.Println(" • To start fresh: gh devlake cleanup --azure, then deploy again")
return fmt.Errorf("Bicep deployment failed: %w", err)
| case "restart": | ||
| fmt.Println("\n🧹 To restart, you need to clean up the existing deployment first") | ||
| fmt.Println(" Note: This will delete all Azure resources in the resource group") | ||
| fmt.Println(" Please run: gh devlake cleanup --azure") |
There was a problem hiding this comment.
The restart guidance assumes gh devlake cleanup --azure will find the Azure state file in the current working directory, but this command writes .devlake-azure.json into --dir. If the user deployed with --dir and is not in that directory, cleanup will fail unless they cd there, pass --state-file, or provide --resource-group. Update the instructions to include the correct directory/state-file path.
This issue also appears on line 328 of the same file.
| fmt.Println(" Please run: gh devlake cleanup --azure") | |
| fmt.Printf(" Please run: cd %s && gh devlake cleanup --azure\n", deployAzureDir) |
| // PingURL checks if a DevLake backend is reachable at the given URL. | ||
| func PingURL(baseURL string) error { | ||
| baseURL = strings.TrimRight(baseURL, "/") | ||
| return pingURL(baseURL) | ||
| } |
There was a problem hiding this comment.
PingURL is a new exported helper, but there are no direct tests covering its trimming/behavior. Since this is now used by deploy flows to check backend reachability, add a small unit test (e.g., with an httptest server and a trailing-slash URL) to prevent regressions.
| // detectExistingLocalDeployment checks for existing deployment artifacts and prompts for action. | ||
| // Returns the existing state (if found) and the user's choice: "resume", "restart", or "abort". | ||
| func detectExistingLocalDeployment(dir string) (*devlake.State, string) { | ||
| if deployLocalQuiet { | ||
| // When called from init wizard, don't prompt | ||
| return nil, "" |
There was a problem hiding this comment.
New interactive recovery logic (detectExistingLocalDeployment / cleanupLocalQuiet) changes deploy behavior significantly, but cmd/deploy_local_test.go currently only covers the poetry-line rewrite helper. Add unit tests for the detection paths (state file present, artifacts-only, quiet mode) and ensure the chosen action is handled correctly.
| // detectExistingAzureDeployment checks for existing Azure deployment state and prompts for action. | ||
| // Returns any existing state data and the user's choice: "resume", "restart", or "abort". | ||
| func detectExistingAzureDeployment(dir string) (map[string]any, string) { | ||
| if deployAzureQuiet { | ||
| // When called from init wizard, don't prompt | ||
| return nil, "" | ||
| } | ||
|
|
There was a problem hiding this comment.
New interactive resume/restart logic (detectExistingAzureDeployment) affects control flow and user guidance, but there are no tests for Azure deploy behaviors in this package. Consider adding unit tests around state-file parsing and action selection (including partial state) to catch regressions, especially since this code now influences whether deployments proceed or exit.
Original prompt