-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Upgrade Node10 #3199
Upgrade Node10 #3199
Conversation
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
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.
Switching to multi from build is something we wanted to do for quite some time, so thank you for taking a stab at this!
I have a few comments though.
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
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.
Thanks again for your effort here! A few more comments :)
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
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.
Why did you remove the bundle step from everywhere?
sorry, I misunderstood. You mean it is okay to delete |
@koooge yes, I meant to remove it in the Dockerfile only. |
This reverts commit 14b7835.
Signed-off-by: koooge <koooooge@gmail.com>
docker-compose.yml
Outdated
@@ -12,6 +12,7 @@ services: | |||
- "5000:5000" | |||
volumes: | |||
- ".:/app" | |||
- /app/client/dist |
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.
What's this for?
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.
it needs for /dist created by npm run build
. Please refer to The node_modules Volume Trick chapter on https://jdlm.info/articles/2016/03/06/lessons-building-node-app-docker.html
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.
The way this docker-compose.yml
file was designed to work is with the user running npm related stuff on the host, so client/dist
will be part of the mounted volume.
See: https://redash.io/help/open-source/dev-guide/docker for details.
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.
well.. we seem to need more hack for this point 🤔
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.
Wait, I just realized /dist created by npm run build
in Dockerfile will be overwritten by volume .:/app
. So, npm run build
is not necessary under development, right? (I understood this build is for redash/redash image!)
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.
So, npm run build is not necessary under development, right?
Correct.
This reverts commit bffc5c9.
redash/base is using node 6 now, but it will be EOL in 2019/4 (https://github.com/nodejs/Release#readme).