Refactor nginx path resolution with improved regex and fallback #1414
Conversation
Updated regex patterns for extracting nginx configuration paths and added fallback mechanisms for determining paths on different operating systems. Improved error handling and logging for better debugging.
|
Thank you for your contribution. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors nginx path resolution functions to improve robustness and provide better fallback behavior. Instead of returning empty strings when parsing fails, the code now falls back to reasonable defaults.
Key changes include:
- Improved regex patterns for parsing nginx configuration paths from
nginx -Voutput - Added fallback mechanisms for
GetConfPath,GetConfEntryPath, andGetPIDPathwhen parsing fails - Enhanced error handling with debug logging for fallback scenarios
- For PID path, added probing of common locations (
/var/run/nginx.pid,/run/nginx.pid) when the configured path isn't found
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| out := getNginxV() | ||
| r, _ := regexp.Compile(`--conf-path=([^\s]+)`) | ||
| match := r.FindStringSubmatch(out) | ||
|
|
There was a problem hiding this comment.
Mixed indentation detected: tabs and spaces are inconsistent. Line 69 uses tabs while the surrounding lines (65-68, 70-81) use spaces for indentation. This should be standardized to match the project's indentation style (appears to be tabs based on other parts of the file).
| } else { | ||
| confPath = "/etc/nginx" | ||
| } | ||
|
|
There was a problem hiding this comment.
Mixed indentation detected: line 79 uses tabs while the surrounding lines use spaces. This should be standardized to match the project's indentation style.
| path = match[1] | ||
| } else { | ||
| baseDir := GetConfPath() | ||
|
|
There was a problem hiding this comment.
Mixed indentation detected: line 108 uses tabs while the surrounding lines use spaces. This should be standardized to match the project's indentation style.
| baseDir := GetConfPath() | ||
|
|
||
| if baseDir != "" { |
There was a problem hiding this comment.
Potential infinite recursion: GetConfEntryPath calls GetConfPath() as a fallback (line 107), which could lead to circular dependency issues if GetConfPath ever needs to call GetConfEntryPath. While this may not happen in the current code, consider passing a flag or using a different approach to avoid this pattern.
| candidates := []string{ | ||
| "/var/run/nginx.pid", | ||
| "/run/nginx.pid", |
There was a problem hiding this comment.
The hardcoded candidate paths for PID files don't include Windows-specific locations. Consider adding Windows-specific candidates (e.g., relative to GetPrefix() or GetNginxExeDir()) or conditionally defining candidates based on runtime.GOOS to ensure cross-platform compatibility.
| candidates := []string{ | |
| "/var/run/nginx.pid", | |
| "/run/nginx.pid", | |
| var candidates []string | |
| if runtime.GOOS == "windows" { | |
| exeDir := GetNginxExeDir() | |
| prefix := GetPrefix() | |
| candidates = []string{ | |
| filepath.Join(exeDir, "nginx.pid"), | |
| filepath.Join(exeDir, "logs", "nginx.pid"), | |
| filepath.Join(prefix, "nginx.pid"), | |
| filepath.Join(prefix, "logs", "nginx.pid"), | |
| } | |
| } else { | |
| candidates = []string{ | |
| "/var/run/nginx.pid", | |
| "/run/nginx.pid", | |
| } |
You should create a hotfix out of it. Did some additional tests today and its unable with the current version to setup a running NGINX UI inside Docker... |
2 participants
Updated regex patterns for extracting nginx configuration paths and added fallback mechanisms for determining paths on different operating systems. Improved error handling and logging for better debugging.
If we have helped you, please leave our company a nice review: https://g.page/r/CaNMAPSpl68TEBM/review