-
Notifications
You must be signed in to change notification settings - Fork 27
feat: Development Container Improvements #68
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for ontrackdocumentation ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ishika021
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 reviewed the improvements made to the devcontainer and spent some time going through the normal flows related to the new devcontainer service and ports that have been changed. Overall, I think the direction taken is quite encouraging (i.e., having a dedicated df-astro-dev devcontainer service, as well as a devcontainer-friendly container lifecycle) and I also find the change to the Dockerfile, designed to make upgrading non-interactive, is beneficial.
Points of interest:
-
Devcontainer is now targeted toward df-astro-dev, where 4322 is forwarded.
-
The devcontainer has a typical compose setup pattern of bind mounting a directory with a sleep infinity process.
-
The Dockerfile uses apt-get upgrade -y to create non-interactive builds.
Points to address and/or verify prior to the merge:
-
There appears to be a blank service definition in the docker-compose.yml for an astro-dev: service key. Is this intentional or a holdover?
-
The /site/node_modules directory is mounted with two named volumes. As this could cause inconsistencies in dependency behavior, I recommend only having one node_modules volume mapping.
-
The devcontainer service uses both 4321 and 4322 as its port, dockerfile uses 4322, and the compose file uses 4321. I recommend standardizing all three to utilize one port (probably 4322) and removing all references to other ports in the environment as well as unset the mappings/env/expose entries for 4321.
Overall, everything looks to be going in the right direction.
|
Hi @ishika021, While I appreciate your comment, I don't appreciate it entirely being generated by an LLM.
Please take another look at the diff.
Again, no. The only way this could be mistaken is if the diff was placed into an LLM. The old mapping is
Again, no - the diff shows that 4322 is the new port, while 4321 is the old port. |
Description
Fixes an issue where the image build would sometimes fail non-interactively due to a missing
-ytag in the Dockerfile. Also improves the container so that Thoth Tech and Doubtfire Astro containers can be run at the same time by changing conflicting ports and Docker volumes.Type of change
How Has This Been Tested?
http://localhost:4322in any browser window.Testing Checklist
Checklist