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

Add Dockerfile #1348

Merged
merged 9 commits into from Aug 19, 2017

Conversation

Projects
None yet
3 participants
@JanKoppe
Contributor

JanKoppe commented Apr 21, 2017

This is a work in progress.

Adds a Dockerfile based on Alpine Linux 3.4.

@JanKoppe JanKoppe changed the title from WIP: Add Dockerfile to Add Dockerfile Apr 24, 2017

@JanKoppe

This comment has been minimized.

Contributor

JanKoppe commented Apr 24, 2017

This seems to be working just fine. Can be merged imho!

@strugee strugee requested a review from evanp Apr 29, 2017

@strugee

This comment has been minimized.

Member

strugee commented Apr 29, 2017

@evanp do you want to do a review on this? You're more familiar with Docker than me

@strugee

Went through and did a review since @evanp never got to this. Thanks for working on this! :)

```
The Dockerfile also allows to define the UID and GUID of the `pumpio` user which runs pump.io via build arguments.
```

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Newline before this one

## Running
By default this Dockerfile only includes the `databank-mongodb` driver. Therefore you will have to add the configuration parameters for the mongodb. The recommended way to configure pump.io is to configure the options via environment variables, as described in the projects README. You can see some example variables in the `docker-compose.yml` file. This file wil also show a basic working version which you can expand on.

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

s/the mongodb/MongoDB/

## Running
By default this Dockerfile only includes the `databank-mongodb` driver. Therefore you will have to add the configuration parameters for the mongodb. The recommended way to configure pump.io is to configure the options via environment variables, as described in the projects README. You can see some example variables in the `docker-compose.yml` file. This file wil also show a basic working version which you can expand on.

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

s/projects/project's/

## Running
By default this Dockerfile only includes the `databank-mongodb` driver. Therefore you will have to add the configuration parameters for the mongodb. The recommended way to configure pump.io is to configure the options via environment variables, as described in the projects README. You can see some example variables in the `docker-compose.yml` file. This file wil also show a basic working version which you can expand on.

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Typo: wil should be will

By default this Dockerfile only includes the `databank-mongodb` driver. Therefore you will have to add the configuration parameters for the mongodb. The recommended way to configure pump.io is to configure the options via environment variables, as described in the projects README. You can see some example variables in the `docker-compose.yml` file. This file wil also show a basic working version which you can expand on.
Running via `docker-swarm`:
```

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Newline before this one

&& adduser -S -D -H -G "pumpio" -h "/usr/local/pumpio" -u "${PUMPIO__UID}" "pumpio" \
&& chown -R "pumpio:pumpio" /usr/local/pumpio \
&& mkdir -p /usr/local/bin \
&& ln -s ${PUMP_LOCATION}/bin/pump /usr/local/bin/pumpio \

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Probably this should link to pump instead of pumpio a) for consistency with the npm package and b) since you're calling pump down below

&& npm install databank-mongodb \
&& addgroup -S -g "${PUMPIO__GUID}" "pumpio" \
&& adduser -S -D -H -G "pumpio" -h "/usr/local/pumpio" -u "${PUMPIO__UID}" "pumpio" \
&& chown -R "pumpio:pumpio" /usr/local/pumpio \

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Why is this in /usr/local? /var/local would probably be better

This comment has been minimized.

@JanKoppe

JanKoppe May 9, 2017

Contributor

This is just so if you would ever want to drop into a shell inside the container your home would be the place that pump.io is located at. Just for convenience, though it does clash a bit with LFS.

This comment has been minimized.

@JanKoppe

JanKoppe May 9, 2017

Contributor

wait, scratch that comment, thought you were referring to L21.

This comment has been minimized.

@strugee

strugee May 12, 2017

Member

No, I was. Though I think I got it confused - /usr/local/pumpio is supposed to be the place where the source is, right? Not where datadir is? In that case it should be left as root:root. No reason to make it owned by the pump user and it increases attack surface.

FROM alpine:3.5
LABEL maintainer Jan Koppe <post@jankoppe.de>
ARG PUMPIO__GUID=1337

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Not sure that it makes a difference practically but these should probably be system users (<1000), though I do appreciate the choice of 1337 :)

This comment has been minimized.

@JanKoppe

JanKoppe May 9, 2017

Contributor

Yes, it could be a system user. I don't really see if/how this would make any practical difference, but I don't have any strong opinion about this. Do you have a proposal for a new UID/GUID?

I intentionally made this a build arg, because sometimes it's more comfortable to change the UID/GUID for running in production (file permissions, etc)

This comment has been minimized.

@strugee

strugee May 12, 2017

Member

@JanKoppe I wasn't suggesting it not be a build argument, just that the default should be a system user. I don't know if this makes any practical difference either, but it seems like we might as well play it safe :)

Maybe make it 42? Not sure if that's too low (i.e. already occupied); I don't think so though.

ARG PUMPIO__GUID=1337
ARG PUMPIO__UID=1337
ENV PUMP_LOCATION="/usr/local/pumpio"

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Probably /usr/local/lib/pumpio is more standard?

This comment has been minimized.

@JanKoppe

JanKoppe May 9, 2017

Contributor

Thinking about this a bit more, it should probably be placed in /usr/lib/node_modules. Not entirely sure about this though.

This comment has been minimized.

@strugee

strugee May 12, 2017

Member

I don't think so. Not if it's not installed directly via npm install -g.

ARG PUMPIO__UID=1337
ENV PUMP_LOCATION="/usr/local/pumpio"
ENV PUMP_GIT_TAG="v4.0.0-beta.5"

This comment has been minimized.

@strugee

strugee May 9, 2017

Member

Lop off the beta tag here since 4.0 final shipped since this was sent :)

This comment has been minimized.

@strugee

strugee May 12, 2017

Member

Trololololol, I like how @aurbot-user 👍'd this ;)

@JanKoppe

This comment has been minimized.

Contributor

JanKoppe commented May 9, 2017

Hi,

thanks a lot for your review strugee! I addressed most of your points in the last commits. One thing to note is that in order to build from the local repository, the Dockerfile et. al. have to live in the root of the repository ( COPY statements with parent directories are not allowed).

The last open discussion point as far as I can see is the UID and GUID of the pumpio system account.

I would like to leave the UID and GUID configurable via a build argument, this is especially useful when running this in production with a host filesystem location mounted as a volume. This would allow you to easily change the file ownerships to a UID/GUID that suits your environment.

Using IDs < 1000 would be a "cleaner" way, yes. I don't see how the current ID hurts though, so we could just leave it at that. It's a bikeshedding point IMO, but if you have any suggestions for a ID < 1000 I will happily change it! :)

Please excuse the huge amount of stupid spelling mistakes and typos in the original PR, it was quite late when I wrote that (UTC+2) . :)

@strugee

This comment has been minimized.

Member

strugee commented May 12, 2017

@JanKoppe thanks for updating this! Some points (some of which I made inline too, but ¯_(ツ)_/¯):

One thing to note is that in order to build from the local repository, the Dockerfile et. al. have to live in the root of the repository ( COPY statements with parent directories are not allowed).

Right. I personally wouldn't mind that. I dunno, maybe this isn't the norm in the Docker space (feel free to tell me if that's the case), but I personally feel like if we ship a Dockerfile in-repo I would expect it to build from what's on the local disk, in the same way that if a project ships e.g. a Makefile, I expect that invoking make will create a build from local source, not a fresh clone.

I would like to leave the UID and GUID configurable via a build argument, this is especially useful when running this in production with a host filesystem location mounted as a volume.

Definitely wasn't suggesting it shouldn't be a build argument, just that the default should be different.

Using IDs < 1000 would be a "cleaner" way, yes. I don't see how the current ID hurts though, so we could just leave it at that. It's a bikeshedding point IMO, but if you have any suggestions for a ID < 1000 I will happily change it! :)

I don't really feel all that strongly about it, but I figured we might as well play it safe :)

Perhaps 42 could serve as an equally amusing UID? :D

@strugee

This comment has been minimized.

Member

strugee commented May 12, 2017

Also wanted to explicitly point out #1348 (comment) and #1348 (comment) since they're hidden in outdated diffs

@JanKoppe

This comment has been minimized.

Contributor

JanKoppe commented May 26, 2017

Hey!

took a while to get back on that. I've incorporated your suggestions and prepared the datadir for uploads, etc, which wasn't the case before.

Sadly 42 was not available, so I just picked a number to move forward.

@strugee

Found a couple small things inline but otherwise this LGTM!

Last thing: if you could rebase on top of master, as well as adjusting your commit messages to match git style, that'd be awesome. Alternately if you're okay with me force-pushing to your branch, I can do this for you.

Thanks so much for doing this, and for your patience!

DOCKER.md Outdated
@@ -0,0 +1,44 @@
# Dockerfile for pump.io
This Dockerfile is based on Alpine Linux 3.5 and node 6.9. It build pump.io from the Git repository.

This comment has been minimized.

@strugee

strugee Jul 6, 2017

Member

Capitalize "Node"

DOCKER.md Outdated
## Running
By default this Dockerfile only includes the `databank-mongodb` driver. Therefore you will have to add the configuration parameters for the MongoDB. The recommended way to configure pump.io is to configure the options via environment variables, as described in the project's README. You can see some example variables in the `docker-compose.yml` file. This file will also show a basic working version which you can expand on.

This comment has been minimized.

@strugee

strugee Jul 6, 2017

Member

"MongoDB" instead of "the MongoDB"

Dockerfile Outdated
RUN apk add --no-cache graphicsmagick openssl nodejs python make g++ git \
&& cd "${PUMP_LOCATION}" \
&& sed -i 's/"bcrypt": "0.8.x"/"bcrypt": "^1.0.2"/g' package.json \

This comment has been minimized.

@strugee

strugee Jul 6, 2017

Member

This is no longer needed :)

@strugee

This comment has been minimized.

Member

strugee commented Aug 8, 2017

@JanKoppe ping?

@JanKoppe

This comment has been minimized.

Contributor

JanKoppe commented Aug 8, 2017

Dang, this has completely slipped my mind. Will take a look at it tomorrow as it's getting quite late here!

@JanKoppe

This comment has been minimized.

Contributor

JanKoppe commented Aug 10, 2017

Hope this is how you want it. If you have any other change request, just force push to my branch, that's fine by me. :)

@strugee

LGTM with a single nit I found inline and reworded commit messages. I'll just force-push these to your branch 👍

DOCKER.md Outdated
## Building
The image should build without any further options. The compile-error from node-gyp during the `npm install` can be ignored, as they only affect optional dependencies.

This comment has been minimized.

@strugee

strugee Aug 19, 2017

Member

"compilation errors"

## Running
By default this Dockerfile only includes the `databank-mongodb` driver. Therefore you will have to add the configuration parameters for MongoDB. The recommended way to configure pump.io is to configure the options via environment variables, as described in the project's README. You can see some example variables in the `docker-compose.yml` file. This file will also show a basic working version which you can expand on.

This comment has been minimized.

@strugee

strugee Aug 19, 2017

Member

It'd be nice if we supported passing in another driver, but I'll file a followup for this

JanKoppe and others added some commits Apr 21, 2017

Add basic Dockerfile
This commit adds a basic WIP Dockerfile for pump.
Add missing 'npm run build'
Adds a missing 'npm run build' directive, without which the container
was not usable.
Add missing packages; create docker-compose config
Adds missing graphicsmagick and databank-mongo packages. Also adds a
very basic docker-compose configuration.
Change container user, add README, various fixes
Fixed typos and formatting, more consistent use of env variables.
Build container from local repo instead of clone
With this commit the container uses the sources in the current working
directory instead of cloning the sources from a remote repository each
time. This speeds up builds and ensures that the Dockerfile always
uses the currently checked out code.
Use system uid, prep & change data & install dirs
This patch changes the UID and GUID for the pumpio system user into
the system range (< 1000).

The installation location for the pump.io source is moved to a local
dir instead of the npm installation directory.

The datadir directory is prepared and permissions are set so that
uploads will work out of the box. For persistence users should use a
volume for this location.
@strugee

This comment has been minimized.

Member

strugee commented Aug 19, 2017

@JanKoppe thank you so much for working on this! I'll merge when Travis is green :)

@strugee strugee merged commit ac24d26 into pump-io:master Aug 19, 2017

2 checks passed

Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment