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

make docker work #168

Merged
merged 3 commits into from
Jan 13, 2025
Merged

make docker work #168

merged 3 commits into from
Jan 13, 2025

Conversation

sni
Copy link
Contributor

@sni sni commented Jan 12, 2025

previously the dockerfile installed the dependencies and you then would mount the source folder into the container, shadowing the just created node_modules folder. So this setup only works if you had a node_modules folder on the host machine already which was kind of the reason to use docker in first place.

To make this work, install the app into a subfolder and create the node_modules on folder up.

Adopted some example files from here:

Also added a Makefile :-)

So you can just do a 'make docker-server' or 'make docker-build'.

previously the dockerfile installed the dependencies and you then would mount
the source folder into the container, shadowing the just created node_modules
folder. So this setup only works if you had a node_modules folder on the host
machine already which was kind of the reason to use docker in first place.

To make this work, install the app into a subfolder and create the node_modules
on folder up.

Adopted some example files from here:

- https://github.com/BretFisher/node-docker-good-defaults

Also added a Makefile :-)

So you can just do a 'make docker-server' or 'make docker-build'.
@sni sni force-pushed the rework_dockerfile branch from 673b30f to 434acea Compare January 12, 2025 19:38
@nook24
Copy link
Member

nook24 commented Jan 13, 2025

Awesome, much better than my current hacky setup!

Just a few minor details I would address:

  • LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value"
  • Could you also add the docker run command into the Readme so folks without make can start the container (Windows)
    • In the Readme, you should replace `pwd` with "$PWD" as it works on Linux and Windows (PowerShell)

If you prefer, I can also merge this and adjust this.

@sni
Copy link
Contributor Author

sni commented Jan 13, 2025

there you go...

@nook24 nook24 merged commit 7e5b135 into naemon:vitepress Jan 13, 2025
1 check failed
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.

2 participants