-
Notifications
You must be signed in to change notification settings - Fork 4
Addressing PR comments #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Claude
wants to merge
18
commits into
main
Choose a base branch
from
claude/improve-deploy-time-error-classification
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,308
−119
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
f84a609
Initial plan
Claude a14bd06
Add error classification and bounded port fallback for local deploy
Claude 86a3eb0
Add port rewriting tests, normalize Azure recovery, update docs
Claude e358254
Address PR review: Fix port validation, regex boundaries, companion U…
Claude 2aa3731
Fix port-fallback detection and terminal output formatting
Claude c9b4401
Fix deploy-time error recovery messaging and docs
Claude 714f39b
Fix extractPortFromError byte-slice bug and add custom port bundle de…
Claude 52de0b1
Update deploy local docs intro to reflect auto-start default
Claude 33d2e45
Fix custom-port readiness check and docs; update detectPortBundle com…
Claude 9d996ca
Refactor port-conflict error handling to eliminate duplication and pr…
Claude 1ee34da
Align deploy local help/docs and cover port fallback
ewega ce9a0f5
fix deploy local port conflict messaging
ewega 11ef152
Fix compose file detection and docs consistency
Claude 0df339b
Fix status docs and error wrapping consistency
Claude c517bca
Remove unused composeFileHasDefaultPorts helper
Claude afdb558
Clarify compose file references and URL mappings
Claude 78c70a3
Add backup/atomic writes and improve port-conflict UX
Claude 30e443a
Clarify auto-start as default in deploy local docs
Claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,304 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
| // DeployErrorClass represents a known failure class during deployment. | ||
| type DeployErrorClass string | ||
|
|
||
| const ( | ||
| ErrorClassDockerPortConflict DeployErrorClass = "docker_port_conflict" | ||
| ErrorClassDockerBindFailed DeployErrorClass = "docker_bind_failed" | ||
| ErrorClassAzureAuth DeployErrorClass = "azure_auth" | ||
| ErrorClassAzureMySQLStopped DeployErrorClass = "azure_mysql_stopped" | ||
| ErrorClassAzureKeyVault DeployErrorClass = "azure_keyvault_softdelete" | ||
| ErrorClassUnknown DeployErrorClass = "unknown" | ||
| ) | ||
|
|
||
| // DeployError represents a classified deployment error with recovery context. | ||
| type DeployError struct { | ||
| Class DeployErrorClass | ||
| OriginalErr error | ||
| Port string // For port conflict errors | ||
| Container string // For port conflict errors | ||
| ComposeFile string // For port conflict errors | ||
| Message string // Human-readable classification | ||
| } | ||
|
|
||
| // classifyDockerComposeError inspects a docker compose error and returns | ||
| // a classified error with recovery context. This covers: | ||
| // - "port is already allocated" | ||
| // - "Bind for 0.0.0.0:PORT" | ||
| // - "ports are not available" / "Ports are not available" | ||
| // - "address already in use" | ||
| // - "failed programming external connectivity" | ||
| func classifyDockerComposeError(err error) *DeployError { | ||
| if err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| errStr := err.Error() | ||
| errStrLower := strings.ToLower(errStr) | ||
|
|
||
| // Port conflict patterns (case-insensitive) | ||
| portConflictPatterns := []string{ | ||
| "port is already allocated", | ||
| "bind for", | ||
| "ports are not available", | ||
| "address already in use", | ||
| "failed programming external connectivity", | ||
| } | ||
|
|
||
| isPortConflict := false | ||
| for _, pattern := range portConflictPatterns { | ||
| if strings.Contains(errStrLower, pattern) { | ||
| isPortConflict = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !isPortConflict { | ||
| return &DeployError{ | ||
| Class: ErrorClassUnknown, | ||
| OriginalErr: err, | ||
| Message: "Docker Compose failed", | ||
| } | ||
| } | ||
|
|
||
| // Extract port number from various error formats | ||
| port := extractPortFromError(errStr) | ||
|
|
||
| result := &DeployError{ | ||
| Class: ErrorClassDockerPortConflict, | ||
| OriginalErr: err, | ||
| Port: port, | ||
| Message: "Docker port conflict detected", | ||
| } | ||
|
|
||
| // Try to identify owning container | ||
| if port != "" { | ||
| container, composeFile := findPortOwner(port) | ||
| result.Container = container | ||
| result.ComposeFile = composeFile | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| // extractPortFromError extracts the port number from various Docker error formats: | ||
| // - "Bind for 0.0.0.0:8080: failed: port is already allocated" | ||
| // - "Error response from daemon: Ports are not available: exposing port TCP 0.0.0.0:8080" | ||
| // - "bind: address already in use (listening on [::]:8080)" | ||
| // - "failed programming external connectivity on endpoint devlake (8080/tcp)" | ||
| func extractPortFromError(errStr string) string { | ||
| // Pattern 1: "Bind for 0.0.0.0:PORT" (case-insensitive using regexp) | ||
| re := regexp.MustCompile(`(?i)bind for 0\.0\.0\.0:(\d+)`) | ||
| if matches := re.FindStringSubmatch(errStr); len(matches) > 1 { | ||
| port := matches[1] | ||
| if isValidPort(port) { | ||
| return port | ||
| } | ||
| } | ||
|
|
||
| // Pattern 2: "exposing port TCP 0.0.0.0:PORT" | ||
| if idx := strings.Index(errStr, "0.0.0.0:"); idx != -1 { | ||
| rest := errStr[idx+len("0.0.0.0:"):] | ||
| if end := strings.IndexAny(rest, " ->\n"); end > 0 { | ||
| port := rest[:end] | ||
| if isValidPort(port) { | ||
| return port | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Pattern 3: "listening on [::]:PORT" or "[::]PORT" | ||
| if idx := strings.Index(errStr, "[::]"); idx != -1 { | ||
| rest := errStr[idx+len("[::]"):] | ||
| // Skip potential colon separator | ||
| if strings.HasPrefix(rest, ":") { | ||
| rest = rest[1:] | ||
| } | ||
| if end := strings.IndexAny(rest, " )\n"); end > 0 { | ||
| port := rest[:end] | ||
| if isValidPort(port) { | ||
| return port | ||
| } | ||
| } | ||
| // If no delimiter found, but there are digits, use them | ||
| if len(rest) > 0 { | ||
| for i, ch := range rest { | ||
| if ch < '0' || ch > '9' { | ||
| if i > 0 { | ||
| port := rest[:i] | ||
| if isValidPort(port) { | ||
| return port | ||
| } | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Pattern 4: "(PORT/tcp)" or "(PORT/udp)" in endpoint errors | ||
| if idx := strings.Index(errStr, "("); idx != -1 { | ||
| rest := errStr[idx+1:] | ||
| if end := strings.Index(rest, "/tcp)"); end > 0 { | ||
| port := strings.TrimSpace(rest[:end]) | ||
| if isValidPort(port) { | ||
| return port | ||
| } | ||
| } | ||
| if end := strings.Index(rest, "/udp)"); end > 0 { | ||
| port := strings.TrimSpace(rest[:end]) | ||
| if isValidPort(port) { | ||
| return port | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Pattern 5: Generic port number extraction (last resort) | ||
| // Look for sequences like ":8080" or " 8080 " in the error | ||
| for _, candidate := range strings.Fields(errStr) { | ||
| // Try splitting by colons | ||
| if strings.Contains(candidate, ":") { | ||
| parts := strings.Split(candidate, ":") | ||
| for _, part := range parts { | ||
| part = strings.Trim(part, "(),[]") | ||
| if isValidPort(part) { | ||
| return part | ||
| } | ||
| } | ||
| } | ||
| // Try the field itself (for cases like "[::] 3002") | ||
| cleaned := strings.Trim(candidate, "(),[]") | ||
| if isValidPort(cleaned) { | ||
| return cleaned | ||
| } | ||
| } | ||
|
|
||
| return "" | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
| } | ||
| // Parse to int and validate range 1-65535 | ||
| port := 0 | ||
| for _, ch := range s { | ||
| port = port*10 + int(ch-'0') | ||
| } | ||
| return port >= 1 && port <= 65535 | ||
| } | ||
|
|
||
| // findPortOwner queries Docker to find which container is using the specified port. | ||
| // Returns (containerName, composeFilePath). | ||
| func findPortOwner(port string) (string, string) { | ||
| out, err := 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 err != nil || len(strings.TrimSpace(string(out))) == 0 { | ||
| return "", "" | ||
| } | ||
|
|
||
| lines := strings.Split(strings.TrimSpace(string(out)), "\n") | ||
| 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]) | ||
| } | ||
|
|
||
| // Prefer the exact compose file path Docker recorded | ||
| if configFiles != "" { | ||
| configFile := strings.Split(configFiles, ";")[0] | ||
| configFile = strings.TrimSpace(configFile) | ||
| if configFile != "" { | ||
| if _, statErr := os.Stat(configFile); statErr == nil { | ||
| return containerName, configFile | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: assume docker-compose.yml under working_dir | ||
| if workDir != "" { | ||
| composePath := filepath.Join(workDir, "docker-compose.yml") | ||
| if _, statErr := os.Stat(composePath); statErr == nil { | ||
| return containerName, composePath | ||
| } | ||
| } | ||
|
|
||
| return containerName, "" | ||
| } | ||
|
|
||
| // printDockerPortConflictError prints a user-friendly error message for port conflicts | ||
| // with actionable remediation steps. | ||
| // If customHeader is provided, it replaces the default "Port conflict detected" header. | ||
| // If nextSteps is provided, it replaces the default "Then re-run: gh devlake deploy local" text. | ||
| func printDockerPortConflictError(de *DeployError, customHeader string, nextSteps string) { | ||
| // Print header | ||
| if customHeader != "" { | ||
| // Normalize header to ensure consistent spacing (blank line before emoji-prefixed steps) | ||
| normalizedHeader := customHeader | ||
| if !strings.HasPrefix(normalizedHeader, "\n") { | ||
| normalizedHeader = "\n" + normalizedHeader | ||
| } | ||
| fmt.Println(normalizedHeader) | ||
| } else { | ||
| if de.Port != "" { | ||
| fmt.Printf("\n❌ Port conflict detected: port %s is already in use.\n", de.Port) | ||
| } else { | ||
| fmt.Println("\n❌ Port conflict detected: a required port is already in use.") | ||
| } | ||
| } | ||
|
|
||
| // Print container info and stop commands | ||
| if de.Container != "" { | ||
| fmt.Printf(" Container holding the port: %s\n", de.Container) | ||
|
|
||
| if de.ComposeFile != "" { | ||
| fmt.Println(" Stop it with:") | ||
| fmt.Printf(" docker compose -f \"%s\" down\n", de.ComposeFile) | ||
| } else { | ||
| fmt.Println(" Stop it with:") | ||
| fmt.Printf(" docker stop %s\n", de.Container) | ||
| } | ||
| } else if de.Port != "" { | ||
| fmt.Println(" Find what's using it:") | ||
| fmt.Println(" docker ps --format \"table {{.Names}}\\t{{.Ports}}\"") | ||
| } | ||
|
|
||
| // Print next steps | ||
| if nextSteps != "" { | ||
| fmt.Println(nextSteps) | ||
| } else { | ||
| fmt.Println(" Then re-run:") | ||
| fmt.Println(" gh devlake deploy local") | ||
| } | ||
|
|
||
| fmt.Println("\n💡 To clean up partial artifacts:") | ||
| fmt.Println(" gh devlake cleanup --local --force") | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port bundle ordering here (
8080/4000/3002and8085/4004/3004) is inconsistent with other docs/help text that use8080/3002/4000and8085/3004/4004. Consider using the same ordering everywhere to reduce user confusion.