-
Notifications
You must be signed in to change notification settings - Fork 78
feat(dev): add development container configuration (#54) #62
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
feat(dev): add development container configuration (#54) #62
Conversation
86a1f24 to
f53db3b
Compare
…rm#54) - Add Dockerfile with Go and Node.js development environment - Configure docker-compose.yml with volume mounts and caching - Create setup-dev.sh for post-container initialization - Set up Git, Go, and Node.js development tools - Add environment variables and user configurations - Configure npm global package handling - Implement tool verification and error handling This change introduces containerized development environment for consistent development across different machines.
…m#54) - Add quick start instructions for VS Code and other IDEs - Include container verification steps
f53db3b to
6e3cae9
Compare
kyasbal
left a comment
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.
Thank you for contributing our code base!
I haven't tried around .devcontainer for a while, but I learnt how this is well configured and make new contributors easy to start contributes. I think @RyuSA will also comment on this because he should be familiar with around this.
I added some comments. We are open to improve, thus please feel free to reply these suggestions if you have better solution rather than my suggestion.
Thanks! 👍
|
And we need to add a license headers in the following scripts. You can add this just by running Lines 21 to 22 in 737a2f8
|
…oogleCloudPlatform#54) - Add hash verification for tool installations - Support AMD64/ARM64 architectures for Node.js - Fix npm global permissions and caching - Improve build configuration and cleanup
90b5e6b to
e9911cf
Compare
kyasbal
left a comment
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.
I tested it on my newly setup Linux VM. It worked after configuring user id or group id environment variables as I pointed out in a review comment.
This devcontainer is very easy setup and so useful!
.devcontainer/docker-compose.yml
Outdated
| USER_UID: ${USER_UID:-1000} | ||
| USER_GID: ${USER_GID:-1000} |
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.
I wonder how these are common to have in environment variables.
I tested this with a new Debian environment just with a docker command. $USER_UID and $USER_GID don't exist but $USERNAME exists.
This seems to cause file permission error on setup-dev.sh L25 because these folder is hold by UID 1000, but VSCode seems to pass the right UID to connect the container.
It works if I set these outside of the container and we can document it, but do you have any better solution?
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.
Now, I've introduced a different approach 7df77f0 for the user handling to work better across different environments:
- Made the GID allocation dynamic:
- Uses next available GID incase of system group conflicts (like GID 20 for 'dialout')
- Preserves specified GID when no conflicts exist
- User setup is now more robust:
- Used POSIX-standard
idcommand for detection and UID/GID fallbacks in Dockerfile (UID/GID 1000) - Should ideally work with VSCode and other environments
- Used POSIX-standard
I have tested and it's working. Let me know your thoughts on this!
NOTE: This must be run as a non-root user. Running as root (UID 0) is not supported and will fail with an error.
…dPlatform#54) - Add support for large UIDs in container user creation - Fix user group handling when GID conflicts with system groups - Update environment variable management in setup script
c1a6a47 to
7df77f0
Compare
kyasbal
left a comment
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.
Looks everything good except a thing.
The .env generated in .devcontainer after the build seems not to be ignored in .gitignore.
If I understand this change correctly, that should be ignored, shouldn't it? I'm gonna approve this PR once this concern is solved.
Yep, 👍 I'll add it |
kyasbal
left a comment
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.
Looks awesome! Thank you a lot for your contribution 🎉 🎉 🎉
Add Development Container Configuration (#54)
Overview
This PR introduces devcontainer setup configurations. The configuration includes necessary tools, dependencies, and post-setup scripts for Go and Node.js development.
Changes
Added Development Container Config Files
Dockerfile : Sets up Ubuntu-based development environment with:
docker-compose.yml : Configures:
setup-dev.sh : Post-creation setup script that:
Testing
Verified the following tools inside the devcontainer
Notes