-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: ensure Dockerfile includes etc directory and correct CMD based on config #4343
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
Conversation
This PR addresses a potential issue with path duplication when generating Dockerfiles. Changes made:
This change ensures that the path used in the Dockerfile is always relative to the project root, Please merge the two submissions together, otherwise it will be incomplete |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…n config - Modify dockerCommand to correctly check for etc directory relative to Go file - Use filepath.Join for better cross-platform compatibility - Ensure etc directory is included in Dockerfile regardless of current working directory - Fix issue where CMD was not set correctly due to missing etc directory
Unit Tests Added for PR #4343I've reviewed this PR and can confirm it fixes two real bugs:
Test VerificationI created comprehensive unit tests to verify both fixes. Here's a summary: Test Coverage:
Results:
Reproduction of BugCreated a test scenario:
Running RUN go build -ldflags="-s -w" -o /app/api service/api/service/api/api.go # ❌ Path duplicated
CMD ["./api"] # ❌ No config file, etc directory not detected Recommendation✅ APPROVE and MERGE - The PR correctly fixes both bugs with minimal, focused changes. Suggested addition: Add the unit test file The complete test file is available if you'd like me to share it or submit it as an additional commit to this PR. |
Complete Unit Test FileI've created a comprehensive test file ( You can view or download the test file from this gist: Or I can commit it directly to the zeromicro/go-zero repository on a separate branch for review. Quick Test SummaryThe test file includes: // Key test functions:
func TestDockerCommand_EtcDirResolution(t *testing.T)
func TestGenerateDockerfile_GoMainFromPath(t *testing.T)
func TestGenerateDockerfile_PathJoinBehavior(t *testing.T)
func TestFindConfig(t *testing.T)
func TestGetFilePath(t *testing.T)
func TestDockerCommandIntegration(t *testing.T)
func TestPR4343_BugFixes(t *testing.T) // ⭐ Specific to this PR All tests pass with Let me know if you'd like me to:
|
Add comprehensive unit tests for goctl docker command to verify fixes for: - Bug 1: etc directory check using wrong base path - Bug 2: GoMainFrom path duplication Test coverage: - TestDockerCommand_EtcDirResolution: Tests etc dir resolution logic - TestGenerateDockerfile_GoMainFromPath: Tests path generation - TestGenerateDockerfile_PathJoinBehavior: Demonstrates bug and fix - TestFindConfig: Tests config file finding - TestGetFilePath: Tests file path resolution - TestDockerCommandIntegration: Integration tests - TestPR4343_BugFixes: Specific tests for PR #4343 bugs All 25 test cases pass with 27.1% coverage added. Related to PR #4343
✅ Unit Tests SubmittedI've created a separate PR with comprehensive unit tests for this fix: The test PR includes:
The tests can be reviewed and merged independently or together with this PR to ensure the fixes are properly validated. |
Add comprehensive unit tests for goctl docker command to verify fixes for: - Bug 1: etc directory check using wrong base path - Bug 2: GoMainFrom path duplication Test coverage: - TestDockerCommand_EtcDirResolution: Tests etc dir resolution logic - TestGenerateDockerfile_GoMainFromPath: Tests path generation - TestGenerateDockerfile_PathJoinBehavior: Demonstrates bug and fix - TestFindConfig: Tests config file finding - TestGetFilePath: Tests file path resolution - TestDockerCommandIntegration: Integration tests - TestPR4343_BugFixes: Specific tests for PR #4343 bugs All 25 test cases pass with 27.1% coverage added. Related to PR #4343
fix: ensure Dockerfile includes etc directory and correct CMD based on config
Bug Fix: Include etc directory in Dockerfile and set correct CMD based on config
Description
This PR fixes two related issues:
goctl docker
from a directory different from where the Go file is located.Problems
goctl docker
from a directory that was not the same as the Go file's location, the generated Dockerfile did not include the etc directory. This caused issues for projects that rely on configuration files stored in the etc directory.Solution
The fix involves modifying the
dockerCommand
function to:Changes made:
filepath.Join
for better cross-platform compatibility.Testing
Tested the changes by:
goctl docker
from various directory locations relative to the Go file, confirming that the etc directory is correctly included in all cases where it exists.Additional Notes
This fix improves the reliability and consistency of the
goctl docker
command, especially for users working with more complex project structures or running the command from different locations within their project. It ensures that not only is the etc directory properly included, but also that the resulting Docker containers are configured correctly based on the project's configuration files.