Skip to content
This repository has been archived by the owner on May 20, 2022. It is now read-only.

add docker swarm mode support #176

Merged
merged 2 commits into from Sep 25, 2017
Merged

add docker swarm mode support #176

merged 2 commits into from Sep 25, 2017

Conversation

pdemagny
Copy link
Contributor

Signed-off-by: Pierre DEMAGNY pdemagny@printoclock.com

Signed-off-by: Pierre DEMAGNY <pdemagny@printoclock.com>
@pichouk pichouk self-requested a review September 24, 2017 19:33
@pichouk pichouk added the 2: Dev Review Requires review by a core committer label Sep 24, 2017
@xcompass
Copy link
Contributor

Looks good to me. Though I'm never run swarm mode. So I may not be in a good position to review.

@pichouk
Copy link
Contributor

pichouk commented Sep 25, 2017

@pdemagny Thanks for your help !

Copy link
Contributor

@pichouk pichouk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but with few suggestions.
One question : did you test this on a Docker Swarm cluster ? I have no Swarm cluster so I cannot test.

Another thing is that it could be better to add your docker-swarm.yml file under a new /contrib/swarm/ directory on the repository. Usually contribution like this are on a specific folder. Don't forget to modify path on README and volumes also.

docker-swarm.yml Outdated
restart_policy:
condition: on-failure
web:
image: nginx:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using the mattermost/mattermost-prod-web image ?

@@ -5,5 +5,6 @@ if [ -f "/cert/cert.pem" -a -f "/cert/key-no-password.pem" ]; then
else
echo "linking plain config"
fi
ln -s /etc/nginx/sites-available/mattermost$ssl /etc/nginx/conf.d/mattermost.conf
if [ -e "/etc/nginx/conf.d/default.conf" ]; then rm /etc/nginx/conf.d/default.conf; fi
ln -s /etc/nginx/sites-available/mattermost${ssl} /etc/nginx/conf.d/mattermost.conf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point here, but is that necessary to add this to this PR ?

To keep a correct Git history I will prefer to remove this changes from this Pull Request and to open a new one just for this (or if you don't want, I'll do the commit by myself). I will accept directly ;)

@pichouk pichouk added Awaiting Submitter Action Blocked on the author Enhancement Work In Progress Not yet ready for review and removed 2: Dev Review Requires review by a core committer labels Sep 25, 2017
@pichouk
Copy link
Contributor

pichouk commented Sep 25, 2017

Also, it seems that mattermod bot is not working (@jasonblais ?) and didn't post the comment about Mattermost CLA.
So @pdemagny can you please sign the Mattermost CLA ? More information about this is available on another PR comment #171 (comment)

@pichouk pichouk added this to In progress/debating in Tasks Sep 25, 2017
@jasonblais
Copy link
Contributor

Thanks @pichouk, posted a note about mattermod in the Developers channel: https://pre-release.mattermost.com/core/pl/8k1wnzk6ppb6pmppuocueqt6kc

@pdemagny
Copy link
Contributor Author

@pichouk Yes ofc i did test it, i wrote it & ran it within a single node swarm with ssl enabled using letsencrypt, tested a few integrations (fabric & jenkins).
I didn't realize there was a mattermost-prod-web image. I'll use it instead of nginx image, which will make the change on docker-entry.sh irrelevant, so i'll remove it. I had other improvements to make anyway, so i'll make the changes later this week or next week.
I'm not a mattermost user/installer normally (which is why i did this, to try it :p), so i'm not familiar with mattermod, but i'll try to look into it too.

@hmhealey
Copy link
Member

hmhealey commented Sep 25, 2017

I'm just going to do my best Mattermod impression here since it's not going to pick up this PR now that it's already open.


Thanks 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.

@pichouk
Copy link
Contributor

pichouk commented Sep 25, 2017

@pdemagny Great ! Ping me when you will need another review :)

For the mattermod stuff, it's just a bot that ask you to sign CLA (like @hmhealey does :) ). I can see that you sign it, so it's good now ;)

…out & update readme accordingly.

Signed-off-by: Pierre DEMAGNY <pdemagny@printoclock.com>
@pdemagny
Copy link
Contributor Author

pdemagny commented Sep 25, 2017

@pichouk actually, ping ;)

@pdemagny
Copy link
Contributor Author

Using official image made my modifications to docker-entry.sh irrelevant as expected.
More importantly, using it also made use of docker configs useless too, which means that this stack file is not tied to docker 17.06+ anymore.

@pichouk pichouk removed Awaiting Submitter Action Blocked on the author Work In Progress Not yet ready for review labels Sep 25, 2017
@pichouk
Copy link
Contributor

pichouk commented Sep 25, 2017

Good for me !

Thanks a lot for this contribution ! 👍 😄

@pichouk pichouk merged commit bcbe60a into mattermost:master Sep 25, 2017
@pichouk pichouk moved this from In progress/debating to Done/Merged in Tasks Sep 25, 2017
@pdemagny pdemagny deleted the swarm-support branch September 25, 2017 21:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Tasks
Done/Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants