-
-
Notifications
You must be signed in to change notification settings - Fork 258
feat(docker): global compose file #386
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
base: master
Are you sure you want to change the base?
Conversation
|
Nice, we definitely do want this. I think instead of In general though, I would prefer to see all the project-specific in terms of other things, the port specification is also a great feature, but let's keep that out of this PR and do it later. With so much backwards compatibility required in this PR, we want to keep every PR small and focused. |
|
Thanks for the feedback. I just let the local compose files in to keep the backwards compability. But I would love to remove them as well. I will split the PR in two and a small one (for f7723a7). |
33f032c to
509b32d
Compare
| fi | ||
|
|
||
| # execute the command with the compose file(s) and any additional arguments | ||
| $dockerComposeCommand $composeFilePath $@; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that happy with the solution, since it's not very readable. However, it's the best way I could think of to avoid some code duplication.
| function compose_exec(){ | ||
| # check whether there is a local compose file and append them to the global one. | ||
| composeFilePath="-f ../../docker-compose.yml -f ./docker-compose.yml" | ||
| export peliasRegion="projects/$(basename $(pwd))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the peliasRegion variable used for?
My understanding is as long as we require people to cd projects/someProject, we don't need to consider this relative path anywhere. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this variable to properly mount the directories into the docker container. E.g.
Line 17 in 30d952f
| - "./${peliasRegion}/pelias.json:/code/pelias.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna play with this a bit, I feel like there's got to be a way to remove it, right?
I find it really surprising that docker would base the mount directories from where the docker-compose.yml file is located rather than the current working directory where the docker compose command is run from.
Let me know if you've played around with it more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looks like they do use the path relative to docker-compose.yml, which is annoying, but presumably they have their reasons 😆
I've made a few changes so will likely open my own PR so we can iterate more quickly on this.
👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.
Here's the reason for this change 🚀
As mentioned in #253 it would be nice to remove at least the
docker-compose.ymlfrom each project directory since it is more or less the same in each for each region.I will use the terms "project", "region" and "local" synonymously. They all describe a thing in the project directory (e.g. portland-metro)
Here's what actually got changed 👏
In the pelia compose exec function (https://github.com/arnesetzer/pelias_docker/blob/33f032c1e5380930ef245e5be0171f0cacfeac74/cmd/docker.sh#L20), a check has been added to see if a region-specific 'docker-compose.yml' file exists. If so, this file will be used. Otherwise, the new 'global' compose file in the root directory will be used. So it is backwards compatible.Currently, there is some echoing to easily determine which file is being used. This should be removed before going to production.
The new 'global' compose file supports custom ports via the .env file. If these are not set, the default ports will be used.All .env files have been updated with the variables to change the ports.docker-compose.ymlare removed. The compose file in the root dir will be used. If a local file is present it will append/overwrite the global one.Here's how others can test the changes 👀
pelias download wof)