Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Add a Dockerfile #477

Merged
merged 3 commits into from
Nov 4, 2015
Merged

Add a Dockerfile #477

merged 3 commits into from
Nov 4, 2015

Conversation

bencevans
Copy link
Contributor

Building:

docker build -t shout .

Running:

docker run --name shout -d --publish 9000:9000 shout

Building:

    docker build -t shout .

Running:

    docker run --name shout -d --publish 9000:9000 shout
@astorije
Copy link
Collaborator

Thanks a lot for your contribution! :-)

However, I'm not entirely sure this belongs to the shout repository itself.

I feel like although it doesn't hurt to have community-driven documentations on ecosystems that integrate with Shout (deployment/configuration tools like Ansible, process managers like systemd or supervisor, ...), we should avoid to merge these with the main repo.

Does that make sense to sysadmins and Docker experts around here?

@Xe
Copy link
Contributor

Xe commented Sep 26, 2015

To be honest, from a deployment standpoint it is less pain to just put it in the main repo.

@williamboman
Copy link
Contributor

Or create a new shout-docker repo and add a simple;

wget -O -- https://github.com/erming/shout/archive/${RELEASE}.tar.gz | tar -xvz -

Seems a bit overkill though. Docker is very ubiquitous and from most standpoints having the Dockerfile in this repo would be easier to manage.

@astorije
Copy link
Collaborator

Heck, this is still better here than nowhere, so why not. We can still move it to some place else later on anyway.
However, I have never used Docker myself, can someone confirm @bencevans's file is enough? Or do we need to add something else? I'm making this a fully-community-reviewed PR apparently ^^

Also, if you want to update the doc accordingly to this PR, the file to modify is here.

@Xe
Copy link
Contributor

Xe commented Sep 27, 2015

Not to mention my own dockerfile.

@astorije
Copy link
Collaborator

Oh my, now that's a list, thanks @floogulinc!
Some of these are really well written even, we might consider contacting the author(s) of the best one(s) to make them official repo instead of patching this current repo with a Dockerfile, what do you think?

I'm considering closing that PR now that I see there are so many out there...

@williamboman
Copy link
Contributor

I think it's good to have this stuff in official repos. Publishing it under the same username and repo name, as is standard with npm modules, would be good too.

As for the Dockerfile in this PR , I'd prefer if it was a bit more expressive. ENTRYPOINT should also be ["node", "index.js"].

@astorije
Copy link
Collaborator

Thanks for your input @williamboman. Fair enough, let's add a Dockerfile to this repo, but @bencevans, could you update your file according to @williamboman's comment and files from @floogulinc's list and @Xe's file please?

@astorije
Copy link
Collaborator

There is also #166 ... and #491, and #53, and #54 . Either we merge this or we teach the world how to use a search bar :D

@astorije astorije mentioned this pull request Sep 28, 2015
@bencevans
Copy link
Contributor Author

@astorije I've updated the PR.

I've already got a docker-shout, I opened this as it would make things simpler for deployments.

@@ -0,0 +1,3 @@
FROM node:0.12-onbuild
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, doc about this: https://github.com/docker-library/docs/tree/master/node#image-variants

Reading that doc, it sounds like we should be using node:<version> instead. On that note, it's what your own Dockerfile says, any reason why you don't want to do that here too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we run without the -onbuild tag - node:0.12, or even node:4 (but wait until Monday for critical HTTP DoS fix).

@astorije What the -onbuild tag indicates is that it will automatically do this;

WORKDIR /usr/src/app
ONBUILD COPY package.json /usr/src/app
ONBUILD RUN npm install
ONBUILD COPY . /usr/src/app

CMD ["npm", "start"]

What this essentially means is that it will take care of moving source files into the image, and making sure dependencies is installed when running the image. It also specifies a default command ($ npm start) to be executed when running the image (this will be overridden by our ENTRYPOINT).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc page says that it's good to go from 0 to Dockerized quickly but that we should not rely on that on the long run. I'm OK with onbuild knowing that we might have to change in future if we want to have more control over the setup. But again, by that time this might have been moved to its own official repo.

@astorije
Copy link
Collaborator

astorije commented Oct 2, 2015

Hey @bencevans, apart from my comment, it look good to me. Considering I'm no Docker expert, I compared with other links given by @floogulinc.

Also, do you want to update http://shout-irc.com/docs/deployment/docker.html (through https://github.com/erming/shout-website/blob/gh-pages/_docs/deployment/docker.md) according to your PR?

@astorije
Copy link
Collaborator

astorije commented Oct 2, 2015

👍

@JocelynDelalande or @erming, may I have your second review on this please? I know none of you guys are Docker experts, but I think we should leave the community contribute to this accordingly.

@Xe
Copy link
Contributor

Xe commented Oct 2, 2015

It will work.

@Xe
Copy link
Contributor

Xe commented Oct 6, 2015

I have made a more correct dockerfile in Shuo-irc/Shuo#87. It includes things like "not running the daemon as root".

@astorije
Copy link
Collaborator

@bencevans, what do you think of the suggestions @Xe made in Shuo-IRC/Shuo#87 for this current PR?

@bencevans
Copy link
Contributor Author

What @Xe has looks pretty ace. AFK for a few days though so can't update
this pull request for a bit.

@astorije
Copy link
Collaborator

No worries, update when you have time and I'll take another look so that we can merge.

@bencevans
Copy link
Contributor Author

@astorije ready for another look.

@astorije
Copy link
Collaborator

👍

@JocelynDelalande
Copy link
Collaborator

all: Thanks for the crawling work among all those dockerfiles :)

Just for the needs of the review, i started playing with docker, and I think this PR has a bug.
If I do docker build -t shout , I'm getting:

988b087479ac: Error pulling image (4.0-onbuild) from node, endpoint: https://registry-1.docker.io/v1/, Driver btrfs failed to create image rootfs 8b49fe88b40b6c09bbe751e9b235d1919e704ae1765a304226047bd0b203b3fe: stat /var/lib/docker/btrfs/subvolumes/8c00acfb017549e44d28098762c3e6988b087479ac: Error pulling image (4.0-onbuild) from node, Driver btrfs failed to create image rootfs 8b49fe88b40b6c09bbe751e9b235d1919e704ae1765a304226047bd0b203b3fe: stat /var/lib/docker/btrfs/subvolumes/8c00acfb017549e44d28098762c3e6296872a1ca9b90385855f1019d84bb0dac: no such file or directory 

And in fact, the image node:4.0-onbuild seems not to be listed.

Also @bencevans could you pull-request the documentation accordingly before we merge ? That's just so that we don't desynchronize documentation from actual code.

@Xe
Copy link
Contributor

Xe commented Oct 27, 2015

That is an unrelated BTRFS issue. Can you recreate this from a fresh VM running CoreOS?

@JocelynDelalande
Copy link
Collaborator

That is an unrelated BTRFS issue.

@Xe Maybe, but I'm not 100% sure (although I'm very docker begginer) that it is a btrfs issue, @Xe do you agree me that node:4.0-onbuild is no longer available or is it just me misunderstanding docker-hub ?

Can you recreate this from a fresh VM running CoreOS?

I will if really required, but my time is limited, and that's again a new big thing to install/discover to test this PR...

(for the record, I'm running Debian Jessie)

@Xe
Copy link
Contributor

Xe commented Oct 27, 2015

$ git clone https://github.com/coreos/coreos-vagrant
$ cd coreos-vagrant
$ vagrant up && vagrant ssh
vm$ git clone https://path/to/forked/repo
vm$ docker build -t shout shout

@astorije
Copy link
Collaborator

I gave a quick shot at @Xe's commands and I could complete them all, modulo a few extras (switch to right branch, git config to be able to pull, ...).

I was able to start Shout from there (only once though, odd, but that coincides with my lack of Docker knowledge), but couldn't ping it. I didn't spent time on this, I just wanted to test if the Dockerfile was loudly failing or not.

In regards to @JocelynDelalande's comment about Node version, looking at https://hub.docker.com/_/node/, I wonder if we should not specify 4.2-onbuild or, better, 4-onbuild. What do you think @bencevans?
@JocelynDelalande, I'm guessing they keep older versions around (otherwise re-building images over and over would be a nightmare), but these are the officially supported versions now.

@Xe
Copy link
Contributor

Xe commented Oct 29, 2015

@astorije that's just a matter of port forwarding to the VM, which is trivial.

@JocelynDelalande
Copy link
Collaborator

ah finally tested, thanks @Xe for the tip for testing using coreos, that was not simple, but I was finally able to test it :)

So, well, that's all I can say as docker-ignorant : « it works », 👍 thanks @bencevans :)

In regards to @JocelynDelalande's comment about Node version, looking at https://hub.docker.com/_/node/, I wonder if we should not specify 4.2-onbuild or, better, 4-onbuild. What do you think @bencevans?

For myself, I think that we should update… later, let's merge first :)

@JocelynDelalande, I'm guessing they keep older versions around (otherwise re-building images over and over would be a nightmare), but these are the officially supported versions now.

Yes, you are probably right.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants