-
Notifications
You must be signed in to change notification settings - Fork 576
Updated docker-compose to 3.0, use of .env file, and use of a custom network #195
Conversation
- Enable the use of a .env file, and remove the enviroment sections on the docker-compose.yml file. - Enable a custom network, for the complete stack.
# Conflicts: # .env.sample
- Enable the use of a .env file, and remove the enviroment sections on the docker-compose.yml file. - Enable a custom network, for the complete stack.
Tested in: Linux Mint 18.2, and last versions of Docker and docker-compose. Debian 9 and last versions of Docker and docker-compose. |
Thanks @joserprieto for the pull request! Per the CONTRIBUTING.md file displayed when you created this pull request, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Your form should be processed within 24 hours and reviewers for your pull request will be able to proceed. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
Thanks @joserprieto ! |
docker-compose.yml
Outdated
restart: unless-stopped | ||
links: |
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 removed links
in PR #185 because it is deprecated. Should remove it I guess.
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.
The links is because you can assign one of more aliases for the different hosts; i think that, for the moment, docker-compose will mantain retrocompatibility
So we're back on the same question again @jasonblais and @xcompass : should we move to Docker Compose 3 ? Maybe it is a good idea. Actually, the |
@pichouk I forget the previous discussion, but what were the potential side effects of moving to Docker Compose v3? (Link to a previous post is okay, I wasn't able to find it) And conversely, what is the benefit of moving to v3? At first it was to support swarm cluster I think, but that is resolved with #176 |
docker-compose.yml
Outdated
# args: | ||
# - edition=team | ||
args: | ||
- edition=team |
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 should not change the default behavior.
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.
Ok.
For, then, the docker-compose.yml don't be versioned.
docker-compose.yml
Outdated
# - AWS_REGION=us-east-1 | ||
env_file: | ||
- .env | ||
networks: |
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 rationale to use custom network?
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.
For many reasons, you can want to have under control the IPs and the ports exposed outside, and so on.
@@ -0,0 +1,29 @@ | |||
#!/usr/bin/env bash |
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.
Could you also add some instruction to copy the sample file to .env?
docker-compose.yml
Outdated
@@ -1,53 +1,68 @@ | |||
version: "2" | |||
version: '3' |
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.
Which docker-compose directive requires v3 format? If we upgrade the compose files to v3, it will break backward compatibility. Not sure if it is good time to upgrade to v3 and if we decide to go with v3, we need to list it as a breaking change.
@jasonblais We add some discussions about this subject on #151 and this forum post (and a little on #104 and #147). Yes the main benefit to moving to v3 is the swarm support, but it was solved. So, the "only" benefit now is to be up to date on our default configuration to deploy with Docker. I have no particular interest to moving on v3. It just not the first time someone submit the idea ;) |
Thanks! I'm okay switching to v3, don't see major issues and seems to be the preference of the community @pichouk |
As long as we update the documentation accordingly, which you've taken care of here :) Can you also help take a look at
to see if anything needs to be updated? |
@jasonblais Great :) |
I read all the discussions, and i think that here are a debate of use version 2 or version 3 of docker-compose files. The solution can be easy; put docker-compose.v2.yml.sample and docker-compose.v3.yml.sample, and the correspondent instructions on the README. I make a PR right now with this approach in mind. |
…ml, and updated readme, and .gitignore
…ml, and updated readme, and .gitignore
@pichouk about the main documentation, that remarks @jasonblais, i think that: https://docs.mattermost.com/install/prod-docker.html
Can be:
The other link: https://docs.mattermost.com/install/docker-local-machine.html Is for use on a dev environment, not for make all the stack and so on. Regards! |
@joserprieto Thanks. I would prefer to leave a docker-compose yaml file as default one (maybe v3 one). And let people to copy v2 one or use |
I agree with @joserprieto now. We will never achieve to have a "perfect It will not be a breaking change. Current users will still have their |
@pichouk when you remove the docker-compose.yml from this repo, wouldn't it also remove from the local repo when one pulls the change (assume no change in the file)? |
@xcompass Hum you're right. I just tested it and if we want to remove the |
I'm agree with @pichouk. I think that a votation may resolve this question; logically, i can't vote, but the mattermost team's votes can resolve all of this. Regards. |
Good for me. If @jasonblais and @xcompass are ok, then you can update your PR to match this new organization. |
About of to not broke current users installations; i can see two scenarios:
A correct approach (imho), it's to isolate the vars outside the docker-compose.yml file (on a .env file gitignored too), and isolate the production ready docker-compose.yml, that defines the stack (so, it's a configuration file, in a certain way..) I think that is this approach is reflected on the documentation, the user will not have trouble with the change. Regards. |
Any news on a merge for this PR ? Once its merged i'll start to work on #203 :) |
@joserprieto can submit changes describe in this comment and, as soon as @xcompass and/or @jasonblais approve this changes, we can merge :) |
The use of docker volumes, or Traefik, i think that is a personal election. For example; on the current stack definition, we have: volumes:
- ./volumes/app/mattermost/config:/mattermost/config:rw
- ./volumes/app/mattermost/data:/mattermost/data:rw
- ./volumes/app/mattermost/logs:/mattermost/logs:rw
- /etc/localtime:/etc/localtime:ro
volumes:
# This directory must have cert files if you want to enable SSL
- ./volumes/web/cert:/cert:ro
- /etc/localtime:/etc/localtime:ro And yes, we can use something like that: version: "3"
services:
(...)
volumes:
- mttm-config:/mattermost/config:rw
- mttm-data:/mattermost/data:rw
- mttm-logs:/mattermost/logs:rw
- etc-localtime:/etc/localtime:ro
(...)
volumes:
mttm-config:
mttm-data:
mttm-logs:
web-cert:
etc-localtime: But it's the same of the discussion on other threads; is better to use volumes? is better to use custom networks? is better to use compose files v2 or v3? The answer is easy; depends of the use case. With traefick, is the same. I think that put sample docker-compose files, is the best choice; and, with this, any user can take is own decissions, taking his/her use case. |
@pichouk , if i'm not wrong, i made the changes. I resume changes made:
I think that is all done... But, if any change is needed, please, tell me. |
Another changes that can will add, is the explanation of how isolated the .env file and docker-compose.yml file. One option (as valid as another), is take advantage of git hooks. And do something like:
Another form, is make a simple bash script outside the repo dir, that do something like "setup.sh --enable" and "setup.sh --disable", for put symlinks / delete symlinks, before/after pull/push. Maybe useful to explain this on the README. Or, it can be useful on the docs. As you wish 😉 |
Well, for "simple" compose files (debate v2/v3), i think that going with the flow is important here, v2 is getting really old and should be present only for backward compatible reasons. Regarding the "Volumes" debate, a quick look at docker doc is enough for me:
I read a lot of people being reluctant to move to docker volumes because they think its easier to backup, why would it be ? Docker volumes are stored in a predictible directory on the host ... (/var/lib/docker/volumes) and are named like you named them in the compose file. Same goes for custom networks, they offer better isolation for services, while still allowing them to talk to each other if the user wants to. As for the env file, it is a best practice that i'm going to apply on docker-stack.yml file, but we also should promote the use of docker secrets for passwords in this file. And for Traefik, yes it is a personal choice, but it offers this choice to the users of this repo, and doesn't prevent from adding more choices for them in the future by adding more compose/stack file with support for more load-balancers ... For all of these reasons, i completely back the solution with multiple yaml "sample" files in the root of the repo, with something like:
I'm willing to do this after the merge of this PR by extending the work i've done on docker-stack.yml file, depending of what you decide, just let me know ;) |
.env.sample
Outdated
|
||
# See https://docs.docker.com/compose/environment-variables/#the-env-file | ||
|
||
# PostgreSQL Enviroment Variables: |
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.
Typo Enviroment
=> environment
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.
done
.env.sample
Outdated
POSTGRES_DB=mattermost | ||
#PGDATA="/path/to/your/postgresql/data/files | default: /var/lib/postgresql/data/pgdata optional" | ||
#POSTGRES_DB="name_of_your_postgresql_db | default: postgres_user value | optional | ||
# uncomment the following to enable backupe |
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.
backup*
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.
done
docker-compose.v2.yml.sample
Outdated
@@ -50,4 +41,4 @@ services: | |||
- /etc/localtime:/etc/localtime:ro | |||
# Uncomment for SSL | |||
# environment: | |||
# - MATTERMOST_ENABLE_SSL=true | |||
# - MATTERMOST_ENABLE_SSL=true |
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.
Please keep the newline :)
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.
done
|
||
# Specific files: | ||
|
||
docker-compose.yml |
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.
Please keep a newline :)
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.
done
README.md
Outdated
* https://forum.mattermost.org/t/mattermost-with-docker-discussion-about-requirements/3518 | ||
* https://github.com/mattermost/mattermost-docker/issues/151 | ||
|
||
You only have to do is: |
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.
is
-> this
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.
done
README.md
Outdated
@@ -83,6 +132,14 @@ Put your SSL certificate as `./volumes/web/cert/cert.pem` and the private key th | |||
no password as `./volumes/web/cert/key-no-password.pem`. If you don't have | |||
them you may generate a self-signed SSL certificate. | |||
|
|||
### Building images | |||
|
|||
Before start the containers, we must to build the images: |
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 must to build
-> you must build
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.
done
@joserprieto Great :) To add new features (as @pdemagny suggest) I prefer to wait for another PR, but it is a good idea :) Also, thanks for your proposal, it is a great improvement :) |
@pichouk i think that i made all fixes. About the documentation, i can propose here the new doc page, and if you like it, i wll make the PR. Do you agree in proceed on this way? Regards. |
LGTM, thanks a lot :) |
But, this PR is still open? |
@joserprieto You proposed to submit a PR to Mattermost documentation before merging this one. |
Ah, ok. |
This issue has been automatically labelled "stale" because it hasn't had recent activity. /cc @jasonblais @hanzei |
Hey @pichouk, What is the status of this PR? Can it be merged after it has been synced with Would you help with the changes required in the docs? |
We are dropping the support of the |
Hi;
This may be useful for your;
If you want, i can edit the readme and document the use and changes, and why is better this approach.
Regards.