Skip to content
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

fix: ensure Dockerfile includes etc directory and correct CMD based on config #4343

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JackGod001
Copy link

fix: ensure Dockerfile includes etc directory and correct CMD based on 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

Bug Fix: Include etc directory in Dockerfile and set correct CMD based on config

Description

This PR fixes two related issues:

  1. The etc directory was not being included in the generated Dockerfile when running goctl docker from a directory different from where the Go file is located.
  2. Due to the missing etc directory, the Dockerfile's CMD was not being set correctly based on the configuration file.

Problems

  1. When users ran 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.
  2. As a consequence of the missing etc directory, the Dockerfile was not able to locate and use the configuration file to set the correct CMD instruction. This led to incorrect startup commands in the resulting Docker containers.

Solution

The fix involves modifying the dockerCommand function to:

  1. Correctly check for the existence of the etc directory relative to the Go file's location, rather than the current working directory.
  2. Ensure that when the etc directory exists, it's properly included in the Dockerfile, and the CMD is set based on the configuration file.

Changes made:

  1. Updated the logic to check for the etc directory using the Go file's path as a reference.
  2. Used filepath.Join for better cross-platform compatibility.
  3. Ensured that the etc directory is included in the Dockerfile if it exists in the same directory as the Go file.
  4. Fixed the logic to properly set the CMD in the Dockerfile based on the configuration file when the etc directory is found.

Testing

Tested the changes by:

  1. Running 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.
  2. Verifying that the generated Dockerfile contains the correct CMD instruction based on the configuration file when the etc directory is present.
  3. Building and running containers using the generated Dockerfiles to ensure they start up correctly with the proper configuration.

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.

白少杰macpro added 2 commits August 30, 2024 18:19
…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
@JackGod001
Copy link
Author

This PR addresses a potential issue with path duplication when generating Dockerfiles.
The change uses filepath.Base() to extract just the filename from goFile,
preventing situations where the project path might be inadvertently duplicated.

Changes made:

  • Modified the GoMainFrom field assignment in the Docker struct to use filepath.Base(goFile).

This change ensures that the path used in the Dockerfile is always relative to the project root,
improving consistency and preventing potential build issues due to incorrect file paths.

Please merge the two submissions together, otherwise it will be incomplete

Copy link

codecov bot commented Sep 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.01%. Comparing base (8690859) to head (2153f14).
Report is 137 commits behind head on master.

Additional details and impacted files

see 255 files with indirect coverage changes

@kevwan kevwan self-requested a review September 1, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant