-
Notifications
You must be signed in to change notification settings - Fork 174
docker: use node lts and run the app as flood user #837
Changes from all commits
3ca47b9
25409cc
0434bf2
4c754b6
d004b52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,24 @@ | ||
ARG NODE_IMAGE=node:12.2-alpine | ||
ARG NODE_IMAGE=node:12-alpine | ||
ARG WORKDIR=/usr/src/app/ | ||
|
||
FROM ${NODE_IMAGE} as nodebuild | ||
ARG WORKDIR | ||
|
||
ARG FLOOD_BASE_URI=/ | ||
ENV FLOOD_BASE_URI $FLOOD_BASE_URI | ||
|
||
WORKDIR $WORKDIR | ||
|
||
# Generate node_modules | ||
COPY package.json \ | ||
package-lock.json \ | ||
.babelrc \ | ||
.eslintrc.js \ | ||
.eslintignore \ | ||
.prettierrc \ | ||
ABOUT.md \ | ||
$WORKDIR | ||
package-lock.json \ | ||
.babelrc \ | ||
.eslintrc.js \ | ||
.eslintignore \ | ||
.prettierrc \ | ||
ABOUT.md \ | ||
$WORKDIR | ||
|
||
RUN apk add --no-cache --virtual=build-dependencies \ | ||
python build-base && \ | ||
npm install && \ | ||
|
@@ -39,10 +43,18 @@ WORKDIR $WORKDIR | |
RUN apk --no-cache add \ | ||
mediainfo | ||
|
||
COPY --from=nodebuild $WORKDIR $WORKDIR | ||
# Add user to run the application | ||
RUN addgroup -S flood | ||
RUN adduser -S flood -G flood | ||
RUN mkdir /data | ||
RUN chown flood:flood /data | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concerned if adding a user now will break existing setups, looking at existing docker volume for I will have a quick test on my local setup to see what effects there are on my setup when a build new image with these changes, may require users to manually run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you been able to test how it affects current configurations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MacTwister maybe the file are own by root because the docker image was built and the default user was root but if you build it and add another user the file will be own by another user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, so now if you build the docker image on your side using this Dockerfile (with chown changes) then the app will be owned and run as the For new setups using docker:New container will run as For existing setups using docker:The scenario I am describing is for people already running with Docker and have setup the # ... rest of you docker compose file
floodui:
build: .
ports:
- 3000:3000
volumes:
- flood_data:/data So when re-creating the container with new image, the app will crash with the following error:
This is because my volume julian$ docker-compose run --rm floodui ls -l /data
total 4
drwxr-xr-x 3 root root 4096 Oct 3 08:41 server What I am saying is that, if we go through with this change, we'd need to some how inform users what they would need to do to continue working after this upgrade. i.e. update the docs for docker setup to inform the to update the /data ownership manually. What I needed to do using my docker compose setup to continue working:julian$ docker-compose run --user=root --rm floodui chown -R flood:flood /data |
||
|
||
USER flood | ||
|
||
COPY --from=nodebuild --chown=flood:flood $WORKDIR $WORKDIR | ||
|
||
# Hints for consumers of the container. | ||
EXPOSE 3000 | ||
EXPOSE 3000 | ||
VOLUME ["/data"] | ||
|
||
# Start application. | ||
|
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.
nit: Can we restore the previous, unless there's a good reason to change it?
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 previous indentation has 5 spaces. The rest of file has 4 spaces, i think it should use the same. What do you think? If you want to mantain 5 spaces in these lines, i can revert it.