search

ikatson / docker-reviewboard

Dockerized reviewboard
MIT License
112 stars 84 forks source link

minor tweaks to consider #14

Closed alzix closed 7 years ago

alzix commented 7 years ago

Hi, It is my first pull request - so i do not sure about the body format you would expect :) I can summarize my changes as following:

  1. minor tweaks regarding Docker command invocation
  2. using Docker TAG version rather than latest would help your follower to track changes. In case committed DockerHub will preserve no history.

Cool work by the way. Thumbs up! Best regards, Alex

ikatson commented 7 years ago

Hi, @Alex--- , thanks, it looks great!

A few comments:

  1. Let's make a separate volume for postgres. Using dockerized postgres is optional, whereas using dockerized reviewboard is not, cause it's the purpose of this project. Also, it's kind of insecure cause reviewboard will have access to all postgres files, where I think the only think it should have access to is the postgres port.
  2. Not sure, why the memory limit for memcached is needed. The way you describe it sounds like a memory requirement, but that's not what the --memory flag is doing. It's just limiting memcached from using more than 2 gigs, which is probably an overkill for a generic setup.
  3. The docker tag in the makefile is probably not needed (in master). I added tags in dockerhub instead, so that they will be built from corresponding branches. I might tweak dockerhub configuration later, but anyway I think master is for "latest".

Thanks for your contribution!

alzix commented 7 years ago

Hi Igor, regarding item#1 - i totally agree - i'll fix that. regarding item#2 memcached - you are absolutely right. I was not clear about what --memory flag is doing, as it sets an upper bound for memory avalable to memcached. But this tweak is follows ReviewBoard optimization recommendations: https://www.reviewboard.org/docs/manual/dev/admin/optimization/memcached/ regarding item#3 it is to you :) it was my gentle way of suggesting you to use versioned tags on DockerHub rather than latest. You have 2K+ downloads. This means that you have delivered great Docker image. Versioning I was missing in my setup :)

ikatson commented 7 years ago

Thanks, @Alex--- , here are the updated tags: https://hub.docker.com/r/ikatson/reviewboard/tags/

ikatson commented 7 years ago

Regarding memcached thing, I actually did not look up carefully, I thought you are passing the "-m" flag to docker, not to memcached.

In this case, I'm not sure if it's a good default, as I thought running the whole thing from the makefile is more for dev/demo purposes than real-world usage. You probably would not want your docker-machine or docker-for-mac VMs to take 2 gigs for memcached. At least I would not :) Maybe extract it into an optional MEMCACHED_MEMORY variable which is empty by default, and describe that in "Tips" section in README? Any other ideas?

In real world, e.g. using kubernetes or docker-compose would be better, where you can set your own values for everything.