added new version of Dockerfile and convenience script #2

meridius · 2020-04-11T17:07:05Z

I took a liberty of rewriting the Dockerfile to be more lightweight and added a convenience script.

The image now also includes Docker for serverless invoke local --docker -f myFunction to work. This however needs mounted Docker socket and group of user able to access it.
To make the work with the image easier I've created script that does all the heavy lifting and updated README accordingly.

I'm open to suggestions.
Thank you

meridius · 2020-05-10T12:01:58Z

Hi @mnapoli, I'm sure you have a lot on your plate already and I'm grateful for the work you do, but could you please consider looking into this PR? It's not a blocker for me since I already use it from my branch, but I think others might find it useful if it was merged.
Especially if they're new to Bref, installing the necessary tools and their dependencies might be too much of a curve for them (as it was for me). Luckily bref/dev-env image exists to make introduction to Bref easier.
Shame is that it's not mentioned anywhere in the https://bref.sh/docs. I'd like to change the documentation as well once this is merged because it slightly changes how one can work with Bref.

Thank you again

…ainer The `HOST_AWS_CONF_DIR` ENV variable is set in brefphp/dev-environment#2

mnapoli · 2020-05-12T12:17:16Z

Hi, thank you for the PR!

I did view that at the time and thought "oh that's a complete rewrite, I need to take time to review this properly" and never got that time. Merging a rewrite in a pull request is the hardest thing to be honest. I can't just hit merge and commit to maintain your code just like that.

I need to understand why each line was changed. I'm sure there are good reasons for each of them, but I can't "guess" why you changed it.

Here are ways to get out of this situation:

you could comment every change in the PR (yes every single one) so that we can review them one by one
you could open one PR per change (easy to merge, but easy to get conflicts)
same strategy as about (one PR per change) but base each PR on top of each other, that would minimize conflicts and ease your work (unless some code change in some review)
you could revert most changes and focus on the most important ones for now (and explain these changes)

Another problem we need to solve here is the installation of the dev-env script. On master, users don't have to install anything. With your changes, they have to install that script.

Don't get me wrong, that script is awesome. I'm just looking at "how" we can simplify the installation.

One solution for that could be to move it in Bref (e.g. vendor/bin/bref-dev), that way nothing to install and it's available to everyone?

meridius · 2020-05-12T19:38:55Z

Hi, thank you for your time and proposals!

I get why you're so reluctant to merge that (I'd be too). So unless I change my mind for some reason, I think I'll go with option 1 (commenting each line) because it seems the most logical for me and actually the least of work. I'll get to that in a couple day's time.

Regarding the script.
We sure can put it to the Bref itself, but that would kind of defeat its purpose since the script is meant to ease the work with the container so user wont' have to install PHP (in any way) and other deps, but Bref itself requires PHP (and Composer) to be installed.
I think there is a third way - to have it elsewhere completely and/or just link it as a downloadable from Bref Doc.
Anyway, as already said, the script is a nice-to-have, user can use the image without it.

Thank you again and have a nice time.

mnapoli · 2020-05-12T21:01:42Z

We sure can put it to the Bref itself, but that would kind of defeat its purpose since the script is meant to ease the work with the container so user wont' have to install PHP (in any way) and other deps, but Bref itself requires PHP (and Composer) to be installed.

Oh yeah that's a good point!

I think there is a third way - to have it elsewhere completely and/or just link it as a downloadable from Bref Doc.

Yep, good idea. E.g. curl https://bref.sh/bref-dev.sh or something like that.

I think I'll go with option 1 (commenting each line) because it seems the most logical for me and actually the least of work. I'll get to that in a couple day's time.

👍 awesome thank you

meridius · 2020-05-17T20:40:56Z

Dockerfile

-ENV PATH="/root/.serverless/bin:/root/.local/bin:${PATH}"
-
-WORKDIR '/var/task'
+FROM composer:1.10.4


From the start I chose to use Alpine-based image for the following reasons:

it is smaller

compare 136 MB of debian-buster:slim based PHP image

vs 27 MB of alpine:3.11 based PHP image

it already is official Composer image, so I don't need to worry about downloading Composer (and its dependencies) manually later

Suggested change

FROM composer:1.10.4

# This image is based on Linux Alpine, a lighter image than debian/ubuntu

FROM composer:1.10.4

By the way, why not use the latest version?

Is the comment really necessary? I mean yes, it's smaller than something else, but we could pick many other facts. Should we state them too? And how would we know which ones?

And for the latest. You really shouldn't use it when you need to do something that depends on the system version. For example we know that Alpine 3.11 has docker=19.03.5-r0 in repository, but what will the latest a month from now have?

Is the comment really necessary? I mean yes, it's smaller than something else, but we could pick many other facts. Should we state them too? And how would we know which ones?

This comment is to make my life, as maintainer, easier. In 1 year, I will stumble on that code and not remember why we chose that image over a classic linux distribution.

And for the latest. You really shouldn't use it when you need to do something that depends on the system version. For example we know that Alpine 3.11 has docker=19.03.5-r0 in repository, but what will the latest a month from now have?

Good point.

So does that mean that I will have to regularly update the version here, and then update the Docker version below too? How can I find the Docker version for the Alpine version that is referenced here?

I thought the latest through again and I think we can use it in this situation eventually. The only things we might require specific versions of are only serverless and awscli, and maybe even those not. So if we leave out specifying package versions we can have the base system as latest.
I'll push the changes.

BTW, you can find the available packages per Alpine release at https://pkgs.alpinelinux.org/packages?name=docker&branch=v3.11&arch=x86_64

Dockerfile

dev-env

meridius · 2020-05-17T22:09:02Z

dev-env

+fi
+
+docker run --rm --interactive --tty \
+    --user "$(id -u)":"$(id -g)" \


necessary so all files created in container are actually owned by the user running this container

Suggested change

--user "$(id -u)":"$(id -g)" \

# So that all files created in container are actually owned by the user running this container

--user "$(id -u)":"$(id -g)" \

The comment breaks the multiline command, so I'd put the explanation under Explanations in README.md instead.

I'll add it above the docker run command instead.

dev-env

meridius · 2020-05-17T22:12:17Z

dev-env

+    --volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \
+    --volume "${AWS_CONF_DIR}:/home/app/.aws" \
+    --volume "${PWD}:/app" \
+    "bref/dev-env" "$@"


and final one - to pass all commands to the container

Suggested change

"bref/dev-env" "$@"

# "$@" forwards all the arguments to the container

"bref/dev-env" "$@"

The $@ is pretty basic Bash knowledge, but if you really want to have it described it'd have to be put somewhere else (above, below, in README.md) than in the middle of multiline command.

mnapoli

That looks great!

I think some of these comments deserve to be persisted so I added suggestions to add them inline in the code.

I think we are moving forward!

mnapoli · 2020-05-18T17:30:46Z

Dockerfile

-ENV PATH="/root/.serverless/bin:/root/.local/bin:${PATH}"
-
-WORKDIR '/var/task'
+FROM composer:1.10.4


Suggested change

FROM composer:1.10.4

# This image is based on Linux Alpine, a lighter image than debian/ubuntu

FROM composer:1.10.4

By the way, why not use the latest version?

mnapoli · 2020-05-18T17:33:10Z

Dockerfile

+    && composer global require hirak/prestissimo \
+    && apk add --no-cache \
+        docker=19.03.5-r0 \
+        yarn=1.19.2-r0 \


All Bref docs mention npm, could we install it? (we can also have yarn, why not)

Dockerfile

mnapoli · 2020-05-18T17:33:54Z

Dockerfile

+    && pip3 install \
+        --progress-bar off \
+        --no-cache-dir \
+        --disable-pip-version-check \
+        awscli==1.18.39


👍 thanks

Could you add this information into a comment?

mnapoli · 2020-05-18T17:34:27Z

Dockerfile

+        awscli==1.18.39
+
+USER app
+ENV PATH=$PATH:/tmp/vendor/bin \


I'm not sure which packages you have in mind?

Do you have an example of a use case?

mnapoli · 2020-05-18T17:37:41Z

Dockerfile

+    COMPOSER_HOME=/app/.composer
+VOLUME /app
+VOLUME /home/app/.aws
+VOLUME /var/run/docker.sock


Suggested change

VOLUME /var/run/docker.sock

# Change the docker socket file location for successfully running Docker in Docker.

VOLUME /var/run/docker.sock

mnapoli · 2020-05-18T17:40:35Z

dev-env

+fi
+
+docker run --rm --interactive --tty \
+    --user "$(id -u)":"$(id -g)" \


Suggested change

--user "$(id -u)":"$(id -g)" \

# So that all files created in container are actually owned by the user running this container

--user "$(id -u)":"$(id -g)" \

dev-env

mnapoli · 2020-05-18T17:42:50Z

dev-env

+    --user "$(id -u)":"$(id -g)" \
+    --group-add "${DOCKER_GROUP_ID}" \
+    --env HOST_AWS_CONF_DIR="${AWS_CONF_DIR}" \
+    --volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \


Suggested change

--volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \

# Necessary for Docker in Docker

--volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \

The comment breaks the multiline command. In addition the explanation is already mentioned under Explanations in README.md.

mnapoli · 2020-05-18T17:43:22Z

dev-env

+    --volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \
+    --volume "${AWS_CONF_DIR}:/home/app/.aws" \
+    --volume "${PWD}:/app" \
+    "bref/dev-env" "$@"


Suggested change

"bref/dev-env" "$@"

# "$@" forwards all the arguments to the container

"bref/dev-env" "$@"

Co-authored-by: Matthieu Napoli <[email protected]>

meridius · 2020-05-21T20:57:51Z

Dockerfile

-ENV PATH="/root/.serverless/bin:/root/.local/bin:${PATH}"
-
-WORKDIR '/var/task'
+FROM composer:1.10.4


Is the comment really necessary? I mean yes, it's smaller than something else, but we could pick many other facts. Should we state them too? And how would we know which ones?

And for the latest. You really shouldn't use it when you need to do something that depends on the system version. For example we know that Alpine 3.11 has docker=19.03.5-r0 in repository, but what will the latest a month from now have?

meridius · 2020-05-21T21:29:50Z

Dockerfile

+    && composer global require hirak/prestissimo \
+    && apk add --no-cache \
+        docker=19.03.5-r0 \
+        yarn=1.19.2-r0 \


Well, the docs mention it exactly 3 times:

https://bref.sh/docs/installation.html

npm install -g serverless - already handled in this image

https://bref.sh/docs/environment/serverless-yml.html

there is a link to https://serverless.com/framework/docs/providers/aws/guide/plugins/ where they state npm install --save custom-serverless-plugin which can be written as yarn add custom-serverless-plugin

https://bref.sh/docs/frameworks/laravel.html

npm run prod the same as yarn run prod

But I guess we can have npm too, if you insist. I don't really mind.

meridius · 2020-05-21T21:41:19Z

Dockerfile

+    && pip3 install \
+        --progress-bar off \
+        --no-cache-dir \
+        --disable-pip-version-check \
+        awscli==1.18.39


Suggested change

&& pip3 install \

--progress-bar off \

--no-cache-dir \

--disable-pip-version-check \

awscli==1.18.39

&& pip3 install \

--progress-bar off \

--no-cache-dir \

--disable-pip-version-check \

awscli==1.18.39

##

# pip3 install options explained:

# --no-cache-dir makes resulting image is smaller

# --disable-pip-version-check makes build quicker

# --progress-bar off makes output cleaner

# --upgrade is not necessary since we don't want to deal with its upgrade anyway

# --user is not necessary since we're still root

##

However I'm against explaining options since user should be able to read it easily from their names (that's why i don't use short variants, if at all possible) and if not they should read docs of given program where more information and related info could be.

meridius · 2020-05-21T21:48:34Z

Dockerfile

+        --disable-pip-version-check \
+        awscli==1.18.39
+
+USER app


TODO: try to not create the app user

Dockerfile

README.md

meridius · 2020-05-21T22:25:01Z

dev-env

+fi
+
+docker run --rm --interactive --tty \
+    --user "$(id -u)":"$(id -g)" \


The comment breaks the multiline command, so I'd put the explanation under Explanations in README.md instead.

meridius · 2020-05-21T22:26:24Z

dev-env

+    --user "$(id -u)":"$(id -g)" \
+    --group-add "${DOCKER_GROUP_ID}" \
+    --env HOST_AWS_CONF_DIR="${AWS_CONF_DIR}" \
+    --volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \


The comment breaks the multiline command. In addition the explanation is already mentioned under Explanations in README.md.

meridius · 2020-05-21T22:29:07Z

dev-env

+    --volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \
+    --volume "${AWS_CONF_DIR}:/home/app/.aws" \
+    --volume "${PWD}:/app" \
+    "bref/dev-env" "$@"


The $@ is pretty basic Bash knowledge, but if you really want to have it described it'd have to be put somewhere else (above, below, in README.md) than in the middle of multiline command.

- removed unnecessary code - changed project dir from `/app` to `/var/task`

…ed them

added new version of Dockerfile and convenience script

06c5581

added support for bref/dashboard via HOST_AWS_CONF_DIR

a6a8531

meridius added a commit to meridius/bref that referenced this pull request May 10, 2020

added support for running bref/dashboard within bref/dev-env cont…

bde68f7

…ainer The `HOST_AWS_CONF_DIR` ENV variable is set in brefphp/dev-environment#2

meridius mentioned this pull request May 10, 2020

added support for running bref/dashboard within bref/dev-env brefphp/bref#649

Closed

mnapoli added the enhancement New feature or request label May 12, 2020

meridius commented May 17, 2020

View reviewed changes

mnapoli reviewed May 18, 2020

View reviewed changes

Apply suggestions from code review

65a1254

Co-authored-by: Matthieu Napoli <[email protected]>

meridius commented May 21, 2020

View reviewed changes

meridius added 3 commits May 22, 2020 00:39

Apply suggestions from code review

668f3d3

- removed unnecessary code - changed project dir from `/app` to `/var/task`

removed specific package and system versions since we don't really ne…

8051f6b

…ed them

updated documentation

e1813c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added new version of Dockerfile and convenience script #2

added new version of Dockerfile and convenience script #2

meridius commented Apr 11, 2020

meridius commented May 10, 2020

mnapoli commented May 12, 2020

meridius commented May 12, 2020

mnapoli commented May 12, 2020

meridius May 17, 2020

mnapoli May 18, 2020

meridius May 21, 2020

mnapoli May 22, 2020

meridius May 24, 2020

meridius May 17, 2020

mnapoli May 18, 2020

meridius May 21, 2020

meridius May 24, 2020

meridius May 17, 2020

mnapoli May 18, 2020

meridius May 21, 2020

mnapoli left a comment

mnapoli May 18, 2020

mnapoli May 18, 2020

mnapoli May 18, 2020

mnapoli May 18, 2020

mnapoli May 18, 2020

mnapoli May 18, 2020

mnapoli May 18, 2020

meridius May 21, 2020

mnapoli May 18, 2020

meridius May 21, 2020

meridius May 21, 2020

meridius May 21, 2020

meridius May 21, 2020

meridius May 21, 2020

meridius May 21, 2020

meridius May 21, 2020

	FROM composer:1.10.4
	# This image is based on Linux Alpine, a lighter image than debian/ubuntu
	FROM composer:1.10.4

	--user "$(id -u)":"$(id -g)" \
	# So that all files created in container are actually owned by the user running this container
	--user "$(id -u)":"$(id -g)" \

	"bref/dev-env" "$@"
	# "$@" forwards all the arguments to the container
	"bref/dev-env" "$@"

	VOLUME /var/run/docker.sock
	# Change the docker socket file location for successfully running Docker in Docker.
	VOLUME /var/run/docker.sock

	--volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \
	# Necessary for Docker in Docker
	--volume "${DOCKER_SOCK_PATH}:/var/run/docker.sock" \

added new version of Dockerfile and convenience script #2

Are you sure you want to change the base?

added new version of Dockerfile and convenience script #2

Conversation

meridius commented Apr 11, 2020

meridius commented May 10, 2020

mnapoli commented May 12, 2020

meridius commented May 12, 2020

mnapoli commented May 12, 2020

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

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

mnapoli left a 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

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

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