Images App Alpine, optimization #208

andruwa13 · 2017-11-21T16:21:05Z

No description provided.

pichouk · 2017-11-21T21:31:25Z

app/Dockerfile

@@ -1,4 +1,4 @@
-FROM ubuntu:16.04
+FROM alpine


We need to stick on a specific Alpine version. Let's go for alpine:3.6, which seems to be the latest.

pichouk · 2017-11-21T21:32:21Z

app/Dockerfile


+RUN apk add --no-cache \
+	netcat-openbsd \


Some maniac style comment here : can you order package list by alphabetical order and use the same indent please ? :)

pichouk · 2017-11-21T21:33:56Z

app/Dockerfile

@@ -25,6 +30,7 @@ RUN mkdir -p /mattermost/data \

 # Configure entrypoint and command
 COPY entrypoint.sh /
+RUN chmod +x /entrypoint.sh


Useless entry in my opinion. We just need to set the correct permission for the file on the repository, there is no need to do it on Dockerfile I guess.

pichouk · 2017-11-21T21:35:16Z

app/entrypoint.sh

@@ -78,4 +78,4 @@ if [ "$1" = 'platform' ]; then
  echo "Starting platform"
 fi

-exec "$@"
+exec "$@"


Please keep newline at the end of the file :)

pichouk · 2017-11-21T21:43:59Z

I really really want to use Alpine for Mattermost Docker image, but this is an important thing and we cannot decided alone.
There is an official list of supported operating systems, but Alpine is not in that list. I think we should ask Mattermost staff to discuss if they could consider to support Alpine officially and/or if it sounds ok for them to use it on our Docker image.

@jasonblais I would appreciate your opinion on this. If there is also some developers at Mattermost with an experience on Alpine, I'll like to hear from them too :)

andruwa13 · 2017-11-21T23:21:55Z

@pichouk all fix

pichouk · 2017-11-21T23:27:44Z

Great, good job 👍 Thanks !

pichouk · 2017-11-21T23:30:06Z

Tests failed but it seems that's because you change entrypoint.sh mode from 755 to 644.
Just chmod +x entrypoint.sh and commit the change please :)

pichouk · 2017-11-21T23:34:07Z

app/Dockerfile

@@ -29,6 +29,7 @@ RUN mkdir -p /mattermost/data \

 # Configure entrypoint and command
 COPY entrypoint.sh /
+RUN chmod 750 /entrypoint.sh


No, I think we should not use this instruction on the Dockerfile. I explained myself badly I guess.
Just chmod +x entrypoint.sh inside your Git repository and commit this change :)

pichouk · 2017-11-21T23:49:56Z

app/Dockerfile

@@ -25,6 +29,7 @@ RUN mkdir -p /mattermost/data \

 # Configure entrypoint and command
 COPY entrypoint.sh /
+RUN chmod +x entrypoint.sh


Same here.
Should remove this line, run chmod 755 entrypoint.sh inside your repository and commit the change :)

pichouk · 2017-11-25T16:19:39Z

app/Dockerfile

@@ -25,6 +29,7 @@ RUN mkdir -p /mattermost/data \

 # Configure entrypoint and command
 COPY entrypoint.sh /
+RUN chmod 755 entrypoint.sh


Still not what we should do. Access mode should be directly on the repository, not an extra-step inside the Dockerfile.
Just remove this line, apply this command on your repository chmod 755 entrypoint.sh, commit and push changes :)

I did not understand what to do, remove chmod 755 entrypoint.sh?

Yes remove this line. But it will then only work if correct mode is apply on entrypoint.sh file of the repository.
It worked like that before, but it seems you changed it in your PR. When I check at your changes I see this :

It means that you change mode for the entrypoint.sh file from 755 to 644. You just have to run the command chmod 755 entrypoint.sh inside your shell and push changes to fix this.

pichouk · 2017-11-25T16:36:04Z

TODO/reminder before merge:

~~Fix access mode on entrypoint.sh~~
~~Test in a running server (@pichouk)~~
Update official Mattermost documentation to add Alpine on supported OS (https://docs.mattermost.com/install/requirements.html#software-requirements)

pichouk · 2017-11-25T17:15:04Z

Just test it on my personal server. Seems to work :)

I'll wait for @xcompass review and the Mattermost doc update. Then it is ok to merge for me.

pichouk · 2017-12-06T21:29:39Z

~~I just tested it on my existing server, it seems to works perfectly ;)~~
EDIT: Ok so it seems that I did it 11 days ago and I didn't remember... So double check ;)

LGTM, I'll wait for @xcompass review and if this is ok, we will merge :)

jasonblais · 2017-12-06T21:42:02Z

@pichouk Before we merge the Alpine changes, there was a proposal for one of the Mattermost members do a quick sanity test as well. We have a ticket for it here https://mattermost.atlassian.net/browse/PLT-8291

pichouk · 2017-12-06T22:14:46Z

@jasonblais Good idea ! :)

ccbrown · 2017-12-07T00:07:40Z

app/Dockerfile

+	libffi-dev \
+	linux-headers \
+	netcat-openbsd \
+	&& rm -rf /var/cache/apk/* \


The --no-cache flag shouldn't leave anything behind here, so I'm pretty sure this line is unnecessary.

@ccbrown is this?
rm -rf / var / cache / apk / * \

I'm suggesting removing that line:

RUN apk add --no-cache \ bash \ ca-certificates \ curl \ jq \ libc6-compat \ libffi-dev \ linux-headers \ netcat-openbsd \ && rm -rf /tmp/*

As of Alpine Linux 3.3 there exists a new --no-cache option for apk. This avoids the need to use --update and remove /var/cache/apk/* when done installing packages.

– https://github.com/gliderlabs/docker-alpine/blob/master/docs/usage.md#disabling-cache

xcompass

This looks good to me.

pichouk · 2017-12-09T19:59:46Z

app/Dockerfile


+RUN apk add --no-cache \
+	bash \


If you use /bin/sh for the app/entrypoint.sh script then there is no need to install bash package I guess.

pichouk · 2017-12-12T20:00:16Z

LGTM now.
I'm waiting for Mattermost devs to test this image.
@jasonblais I cannot comment or follow the Jira task. Can you:

add a link to this PR on the task. I think it will not have changes anymore, you can test.
ping me when the task is completed on your side

Thanks :)

jasonblais · 2017-12-12T22:15:00Z

@pichouk Sure thing. I've added a link to this PR. I've also referenced the Docker Compose v3 PR #195

pichouk · 2018-01-18T18:02:43Z

Please also add xmlsec1 package as requested on #225

andruwa13 · 2018-01-24T06:24:16Z

@pichouk done

pichouk · 2018-02-02T18:17:17Z

Good. I'll wait for the Mattermost staff to test and validate this change.
In my side I will test again the image on my server, to validate that everything is ok. I invite everyone who can to do the same :)

Also, I'll squash all commits to get a clean git history :)

pichouk · 2018-02-05T22:12:45Z

I squash the commits and test the image. Looks good to me :)
If anyone else want to test, go ahead.
@jasonblais I'll wait for validation from Mattermost team ?

jasonblais · 2018-02-05T22:30:43Z

I think @coreyhulen will be testing it soon :)

Huge thanks @pichouk and everyone else involved here!

coreyhulen · 2018-02-06T01:31:34Z

Looks good, I've been testing it for a bit. I want to test for a little longer, but we should be gtg by tomorrow.

Unrelated to this PR, but looks like I hit the issue described at docker/for-mac#2396 since we map - /etc/localtime:/etc/localtime:ro

pichouk · 2018-02-06T21:46:54Z

Thanks to @andruwa13 who did the job :)

@coreyhulen It seems that there is a workaround by using an environment variable (TZ ?) instead of mounting /etc/localtime inside the container. Maybe we can use that method.

pichouk · 2018-02-08T23:55:17Z

Everything looks great, I just deployed it to my production server and everything is fine. Let's merge 🎉 😄
Thanks again @andruwa13 for your job !

This reverts commit efec608.

IonCharge · 2018-02-13T23:13:32Z

Are you including Bash in the alpine package? Or just Sh? It seems Mattermost needs/prefers bash?
Maybe worth seeing if it fixes this issue: #234

Just add the following to the dockerfile for the app:
RUN apk add --no-cache bash and change the entrypoint to be bin\bash

@pichouk

This reverts commit efec608.

Images App Alpine, optimization

8a3b0db

pichouk requested review from xcompass and pichouk November 21, 2017 18:40

pichouk self-assigned this Nov 21, 2017

pichouk added Enhancement need tests in production null 2: Dev Review Requires review by a core committer labels Nov 21, 2017

pichouk mentioned this pull request Nov 21, 2017

Convert db and web containers to use alpine images #135

Closed

pichouk suggested changes Nov 21, 2017

View reviewed changes

pichouk reviewed Nov 21, 2017

View reviewed changes

pichouk suggested changes Nov 25, 2017

View reviewed changes

pichouk mentioned this pull request Nov 25, 2017

Mention Docker as a supported server platform mattermost/docs#1656

Merged

pichouk approved these changes Nov 25, 2017

View reviewed changes

pichouk added Do Not Merge Should not be merged until this label is removed and removed need tests in production null labels Nov 25, 2017

pichouk removed the Do Not Merge Should not be merged until this label is removed label Dec 6, 2017

ccbrown reviewed Dec 7, 2017

View reviewed changes

xcompass approved these changes Dec 7, 2017

View reviewed changes

pichouk reviewed Dec 9, 2017

View reviewed changes

lasley mentioned this pull request Jan 4, 2018

[FIX] URLEncode PSQL Password #219

Merged

pichouk mentioned this pull request Jan 30, 2018

mime types issue that prevent video preview #229

Closed

Images App Alpine, optimization

e57fb04

pichouk force-pushed the app branch from 99d12c0 to 6f30791 Compare February 5, 2018 21:55

Merge branch 'master' into app

8d88db8

pichouk force-pushed the app branch from 6f30791 to 8d88db8 Compare February 5, 2018 21:56

coreyhulen self-requested a review February 6, 2018 01:32

coreyhulen approved these changes Feb 8, 2018

View reviewed changes

pichouk merged commit efec608 into mattermost:master Feb 8, 2018

pichouk mentioned this pull request Feb 8, 2018

Alpine variant? #109

Closed

andruwa13 deleted the app branch February 9, 2018 13:15

TechnicLab pushed a commit to TechnicLab/mattermost-docker that referenced this pull request Feb 10, 2018

Revert "Use Alpine based image for app (mattermost#208)"

7c18917

This reverts commit efec608.

pichouk mentioned this pull request Feb 13, 2018

Images Postgres Alpine, optimization #207

Merged

TechnicLab pushed a commit to TechnicLab/mattermost-docker that referenced this pull request Feb 18, 2018

Revert "Use Alpine based image for app (mattermost#208)"

de720f2

This reverts commit efec608.

Images App Alpine, optimization #208

Images App Alpine, optimization #208

Conversation

andruwa13 commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichouk commented Nov 21, 2017

andruwa13 commented Nov 21, 2017

pichouk commented Nov 21, 2017

pichouk commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichouk commented Nov 25, 2017 • edited Loading

pichouk commented Nov 25, 2017

pichouk commented Dec 6, 2017 • edited Loading

jasonblais commented Dec 6, 2017

pichouk commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcompass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichouk commented Dec 12, 2017

jasonblais commented Dec 12, 2017

pichouk commented Jan 18, 2018

andruwa13 commented Jan 24, 2018

pichouk commented Feb 2, 2018

pichouk commented Feb 5, 2018

jasonblais commented Feb 5, 2018

coreyhulen commented Feb 6, 2018

pichouk commented Feb 6, 2018

pichouk commented Feb 8, 2018

IonCharge commented Feb 13, 2018

pichouk commented Nov 25, 2017 •

edited

Loading

pichouk commented Dec 6, 2017 •

edited

Loading