tdmalone / slackemon

Inspired by Pokémon Go, now you can catch and battle Pokémon with your teammates on Slack!
GNU General Public License v3.0
10 stars 3 forks source link

Adds Compose #35

Closed Naramsim closed 7 years ago

Naramsim commented 7 years ago

Add a docker-compose file which has three containers in it, all alpines.

Rewrites scripts. I commented your checks because with Compose they are no more necessary. But if you want you can still use them.

PG connection string should be

postgres://slackemon_user:slackemon_password@database:5432/slackemon_db

Moreover, slackemon_user, slackemon_password, and slackemon_db are polled from .env file when PostrgreSQL initializes the database. So in PHP, you could load them as you have done with other environment vars.

tdmalone commented 7 years ago

Ok building this now to check it out.

I assume I run this?

 docker build -t slackemon .
 docker-composer up

Currently I have SLACKEMON_DATABASE_URL as an environment variable which reads the full database URL format. This means interoperability with Heroku is quite simple because it uses the same database URL format, just on DATABASE_URL. Is it possible for us to use one of these rather than needing the three separate variables? Or is that too complicated because it would mean we'd have to modify postgres:alpine?

tdmalone commented 7 years ago

Because we're not creating a separate user in our Dockerfile anymore, it means we're running composer install as root:

Step 8/8 : RUN php composer.phar install
 ---> Running in 6d4bd4f26d0f
Do not run Composer as root/super user! See https://getcomposer.org/root for details

Is it ok to return to creating our own user for composer, cron etc.?

Naramsim commented 7 years ago

Hi, @tdmalone, to start the image the first time there is no need to build it, compose will take care of everything, just run: docker-compose up -d or ./scripts/start. (To debug I usually omit the -d, and run attached)

I think we can have an environment var SLACKEMON_DATABASE_URL that is built upon some other variables, I need three environment variable for creating the Postgres database. So we could end with an

SLACKEMON_DATABASE_URL=postgres://${POSTGRES_USER}:${POSTGRES_PASSWORD}@172.17.0.2:5432/${POSTGRES_DB}

(When on Docker the Postgres's IP address is always database:5432)

Or, more complicated, let everything as it is now (only a string slackemon_databaseurl) and extending [Postgres](https://hub.docker.com//postgres/) having it to use a regex in bash to split and create three different variables that are readable to Postgres.

I could set up a user as it was if you want. I will try this afternoon.

tdmalone commented 7 years ago

Yeah fair enough about the env vars, I don't think we want to fork/extend the base postgres image. I think we will just put it together around this point of config.php.

So for the most part this looks good!

I did run into an issue with the way background commands/actions are called: they use the SERVER_NAME made available to PHP, which is coming through as localhost. PHP is then curl'ing this, expecting to call the script, but this is not happening - I think because localhost as far as PHP is concerned is its own container, whereas it needs to be calling the nginx container. I think I should probably be using HTTP_HOST in the PHP instead, which will solve this issue.

Finally the other issue I have encountered is that cron didn't seem to be running. Did it work for you? It seemed to run after I manually executed crond in the PHP container, but I might need to try debugging that again later to be sure. Gotta head off now but will go over this more tomorrow morning!

tdmalone commented 7 years ago

Oh, one more thing - could we put the nginx conf in the /etc folder in the repo rather than creating the new /_docker folder? etc is for config (well, config that we can control the path of at least ;) ) to avoid making the root file listing too long.

Naramsim commented 7 years ago

PHP is then curl'ing this, expecting to call the script, but this is not happening - I think because localhost as far as PHP is concerned is its own container, whereas it needs to be calling the nginx container.

In Docker, one should be able to call http://webserver/somepage.php, but in another environment that surely changes

About crond I have no Idea why it is killed. I tried to put it in a CMD final command, but in this scenario Alpine kills PHP... I think the best thing is to create a new Alpine container which runs crond in foreground.

tdmalone commented 7 years ago

I've also removed the old commands from the shell scripts & documented the new ones.

Changes: https://github.com/tdmalone/slackemon/compare/92c207...9a0991

tdmalone commented 7 years ago

Also - I've exposed the Postgres port (https://github.com/tdmalone/slackemon/commit/ede4df64d88009a2d3f1bff8a6372d21e40e63b2) so that it can be accessed easily using eg. pgAdmin - very handy during dev.

tdmalone commented 7 years ago

@Naramsim So, this is all awesome, great work! Its running fairly smoothly for me now! Just one thing, nginx didn't seem to want to accept connections until I had stopped and started docker-compose again. I will try again tomorrow from scratch to see if it was a onetime thing, but I did find someone else experiencing a similar problem.

We probably also don't need php dotenv anymore, since we are always loading .env via docker-compose now. That does mean that a restart is required after changing environment variables, but that's probably not a massive issue.

From here I have/will:

Thanks again for your work on this Alessandro :)

Naramsim commented 7 years ago

Well, good, when in production better to hide the database to the public. Moreover, should the PHP files only be called by Slack? If so, we can set Nginx to only accept request coming from Slack.

tdmalone commented 7 years ago

True... is it possible to conditionally set ports in docker_compose based on an env var? Then we could set the ports option for DB only if APP_ENV = development?

Yes, PHP files should only be called by Slack. There's built in checking of the team ID and private token sent by Slack which takes care of that, though - checking on user agent wouldn't do much and checking on IPs, well Slack's ranges are published but still could change. So I think we've covered what we need to there.

On second thought about php dotenv, I won't remove that, because it's still possible to run Slackemon locally outside of Docker if you already have eg. XAMPP or another set up.

tdmalone commented 7 years ago

Oh did you have a chance to look at the non-root user for Composer install? If not I'm happy to still proceed for now, we can leave that as a future issue to solve (worst case the root user only has access to that container anyway!)

tdmalone commented 7 years ago

Looks like we can't do conditional port setting in docker-compose, except by potentially passing through an environment variable - but then we still have to set it to something.

It looks like we could use extends for local use. I will remove the PG port I added and go ahead without it, and then work out the best thing to do for a local extended file. Probably docker-compose.local.yml or something, and put that in the .gitignore.

And other than that, this is ready to merge!

tdmalone commented 7 years ago

This is now in develop. Will go to master with next release :)

Naramsim commented 7 years ago

Good, I will make another PR to set the correct user