mozillascience / PaperBadger

Issuing badges to credit authors for their work on academic papers
https://badges.mozillascience.org/
Mozilla Public License 2.0
95 stars 45 forks source link

Add redis to Docker setup #155

Closed abbycabs closed 8 years ago

abbycabs commented 8 years ago

Redis was added after we set the project up with Docker. Add redis and updates the docs!

sudheesh001 commented 8 years ago

@acabunoc It looks like this issue was resolved in https://github.com/mozillascience/PaperBadger/commit/a086d2a5c148db76fb20e84382d5dc9653afb780

Should the fix for this issue be from

You can use Docker to bring up a quick instance of the app to develop against. This way you dont need to have node or mongo installed on your host.

to

You can use Docker to bring up a quick instance of the app to develop against. This way you don't need to have node, mongo or redis installed on your host.

should I make changes to the dockerfile as follows ?

FROM node:0.12
FROM redis

RUN mkdir /src
WORKDIR /src

COPY . /src

# build the project only if it needs to
RUN if [ ! -d "node_modules" ]; then npm install; fi

CMD redis-server
CMD npm start

# Expose ports locally
EXPOSE 6379
josmas commented 8 years ago

I am a bit confused about the use of this Dockerfile and the docker-compose file.

If the aim is to quickly setup a development environment, then copying the sources into the container (as it is done in the file) is far from ideal, as they can only be edited from within the container. Sharing the sources from the host would be the easiest way to use docker for development.

If the aim is to quickly put together a few containers for deployment, then it is fine as it is, but it should be properly documented in the readme (stating the intent of the setup).

Unrelated to that, and with regards to the last comment; the redis server runs on its own container (follow the docker-compose file), so there's no need to make those changes to the file.

The issue, as it is framed, is already taken care of in the docker-compose file.

abbycabs commented 8 years ago

Wow, yes this already taken care of. By me. thanks @sudheesh001 & @josmas!

@josmas I'm not very familiar with Docker. @jonocodes helped us set this up and maybe he has more insight into the setup.

jonocodes commented 8 years ago

Hello all.

Yes @sudheesh001, you should not need to edit the Dockerfile since compose it taking care of redis.

@josmas , I think the goal was to use docker to bring up an environment you could use quickly so you can try to use paperbadger with another app. Using Docker to develop paperbadger is a good idea, and is not really covered by this setup. But it would be easy to share in a directory like you said if that was use case. I'm happy to help with any of that.

As a side note, something to consider is distrubuting paperbadger via docker images. You would basically build the docker image that is already specified here and push it to docker hub. Then you could direct people who want to try (not develop) paperbadger to pull it from there. It would take a little maintaince but may be a way of doing some simple packaging/distribution. I'm seeing a lot of projects do that these days.

josmas commented 8 years ago

hey @jonocodes thanks for the info! It makes sense.

I have done something similar to what you mention in your last point with the CoderDojo sources, and it's been really useful at hackathons (instead of spending half the morning installing stuff, you can just spin a container in no time).

In any case, I don't think the current setup is being used a lot, so I guess there's room for changes. What do you all think about the following changes?:

  1. Build from node 4 (instead of 0.12; this may need some other changes)
  2. Share the sources with the host <-- this can be done by adding a run script so it can be configurable
  3. Remove npm start from the file and provide two scripts, one to simply run bash, and one to run npm start
jonocodes commented 8 years ago

Build from node 4 (instead of 0.12; this may need some other changes)

I dont know much about node versions, but perhaps this should be updated then? https://github.com/mozillascience/PaperBadger/blob/master/package.json#L52

Share the sources with the host <-- this can be done by adding a run script so it can be configurable

Sure. This makes sense as long as there is no plan to create distributable images.

Remove npm start from the file and provide two scripts, one to simply run bash, and one to run npm start

What would the bash one be for?

josmas commented 8 years ago

Hi @jonocodes

yep, package.json can be changed. There was a short convo on gitter about how 0.12 will go out of LTS by the end of the year. There's also the use of npm 2 and changes in npm 3 that would improve versions being pulled in.

With regards to the scripts, the first one would be to provide the existing functionality (just run the app), and the bash one would give more flexibility while developing. At times, I find that I want to restart the server (or run tests, or stuff like that), and the only way to restart it now is to bring down the whole container and start it up again.

jonocodes commented 8 years ago

With regards to the scripts, the first one would be to provide the existing functionality (just run the app), and the bash one would give more flexibility while developing. At times, I find that I want to restart the server (or run tests, or stuff like that), and the only way to restart it now is to bring down the whole container and start it up again.

Perhaps the things you are trying to do would work by using 'docker exec' to shell into the container instead of creating a run variant.

josmas commented 8 years ago

It would work for some things with exec ... bash but not in the case of restarting the server.

jonocodes commented 8 years ago

I would probably restart the container completely instead, but sure.

jonocodes commented 8 years ago

Per your suggestion, I have redone the docker setup to mount local sources instead of building an image. There is no Dockerfile anymore - but requires a newer version of Docker. Since the project requirements are so small, I was able to put all the setup into the docker-compose file. It will also be easier for @josmas to extend to create a bash variation for his needs. See pr #179.

abbycabs commented 8 years ago

Amazing, thanks @jonocodes! I'll take a closer look today

josmas commented 8 years ago

This was merged as #179 so closing now.