Implement docker #931

mpiot · 2019-01-13T12:33:25Z

This PR provides a Docker implementation for the Symfony Demo project. (based on https://github.com/mpiot/docker4symfony)

javiereguiluz

Thanks for contributing this. We've wanted to add this for a long time.

However, some of the proposed changes are a no-go for us:

We must use SQLite by default, not MySQL.
We must include all the compiled assets by default, not generate them with Yarn + Webpack Encore.

The Symfony Demo app must be runnable after downloading it without installing anything, configuring anything, changing any file or running any command. Thanks!

.env

config/packages/doctrine.yaml

mpiot · 2019-01-14T14:51:26Z

@javiereguiluz okay, I see many possibilities:

implement MySQL and Yarn in docker (add SQLite to docker), keep build assets and the SQLite db
replace MySQL by SQLite
replace MySQL by SQLite, and remove yarn

I think the first proposal is better, then I also can add a comment in docker near the PDO extension ?

javiereguiluz · 2019-01-14T14:59:36Z

@mpiot I'm afraid I can't help you because I don't know much about Docker. But others will read this and will help you.

In any case, the basic premise is: we can do anything inside Docker ... but we can't change anything in the existing app.

mpiot · 2019-01-14T19:53:31Z

@javiereguiluz I've fix it :-)
I've just keep a Doctrine Migration file, then if a user want to use MySQL he can execute the migration.

javiereguiluz · 2019-01-16T16:04:46Z

Thanks a lot for reworking this. To me it's OK because it doesn't change the existing app ... but I 'll let Docker experts review it before merging. Thanks.

stof · 2019-01-16T18:28:36Z

docker/NginxDockerfile

+        echo '        return 404;'; \
+        echo '    }'; \
+        echo '}'; \
+    } > /etc/nginx/conf.d/default.conf


I suggest to have a default.conf file in this docker and to add it instead. The nginx config file split into echo commands for each line is not really readable. Having a config file would be easier to maintain IMO.

@stof I agrre with you, before I did it like that. I don't remember why I've change it to use this echo...

voronkovich

IMHO It's better to drop all the MySql-related stuff, because it looks very confusing.

voronkovich · 2019-01-24T16:24:12Z

Dockerfile

+RUN { \
+        echo 'date.timezone = Europe/Paris'; \
+        echo 'short_open_tag = off'; \
+        echo 'memory_limit = 8192M'; \


Maybe it's better to just use: memory_limit = -1?

I agree, I've just copied some files used on a personnal project, and I've forget to removed some parts...

By the way, date.timezone can be set to UTC so everyone will have same dates on the app, WDYT?

@Pierstoval Because I'm in France I've configure it like that, but you're totaly right.

Thanks a lot :-)

voronkovich · 2019-01-24T16:29:36Z

docker/nginx-default.conf

+    add_header X-XSS-Protection "1; mode=block";
+    add_header X-Frame-Options SAMEORIGIN;
+
+    if ($http_user_agent ~* "WordPress") {


Why did you add this block?

Mmm, it's about security, but useless in our case too, I've really forget a lot of parts... :-(

voronkovich · 2019-01-24T16:29:54Z

docker/nginx-default.conf

+        try_files $uri /index.php$is_args$args;
+    }
+
+    location /protected-files/ {


Why did you add this block?

It's to store protected files and use X-Accel-Redirect, idem, forget...

voronkovich · 2019-01-25T15:20:07Z

Dockerfile

+
+# Used for the ICU compilation
+ENV PHP_CPPFLAGS="${PHP_CPPFLAGS} -std=c++11"
+ENV APP_VERSION=0.0.0


If we don't need this envs at runtime, we can set their values in the RUN directive:

RUN export PHP_CPPFLAGS="${PHP_CPPFLAGS} -std=c++11" ENV APP_VERSION=0.0.0 \ set -ex; \ # Install required system packages apt-get update; \ apt-get install -qy --no-install-recommends \ ...

See https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#env

I agree, it's interesting :-) I do it ;-)

voronkovich · 2019-01-25T15:26:24Z

Makefile

+	$(EXEC) $(CONSOLE) cache:clear --no-warmup
+	$(EXEC) $(CONSOLE) cache:warmup
+
+tty:                                                                                                   ## Run app container in interactive mode


I would suggest to use sh instead o tty here.

This part open a terminal in the container, then I've named it tty:
https://en.wikipedia.org/wiki/Computer_terminal#Text_terminals

I think sh is not really correct because there is a difference between /bin/sh and /bin/bash

So, maybe a bash would be more appropriate name? BTW, I think you forgot to add a -it flags to allocate tty.

voronkovich · 2019-01-25T15:31:01Z

Makefile

+
+clear: perm                                                                                            ## Remove all the cache, the logs, the sessions and the built assets
+	$(EXEC) rm -rf var/cache/*
+	$(EXEC) $(CONSOLE) redis:flushall -n


voronkovich · 2019-01-25T16:04:12Z

Makefile

+wait-for-db:
+	$(EXEC) php -r "set_time_limit(60);for(;;){if(@fsockopen('db',3306)){break;}echo \"Waiting for MySQL\n\";sleep(1);}"
+
+db-diff: vendor wait-for-db                                                                            ## Generate a migration by comparing your current database to your mapping information


So, If we don't use the MySql all these command will wait 60 seconds?

Yes, in fact... I can remove all the db part, or just remove the waiting part and the db-reset part.

This is actually quite nice oneliner. Wait until services are ready is very useful, so I would like to see something like this in the makefile, even if commented out as we use sqlite here.

Yes, but it's difficult to add it in the Makefile whithout MySQL... Basically I've add all the Makefile db part in the case the user would like to use MySQL, but then we've removed all MySQL trace...

mpiot · 2019-01-25T16:51:13Z

@voronkovich I've fixed some parts, as mentioned above, I've forget some parts (I use files from a personnal project).

I've removed MySQL part, indeed, it's simpler whithout all theses commented parts. I've keep the Makefile db part too, but maybe we can remove it ?

voronkovich · 2019-01-25T16:58:31Z

src/Migrations/Version20190113114527.php

@@ -0,0 +1,46 @@
+<?php declare(strict_types=1);


@mpiot, You forgot to remove this migration file.

voronkovich · 2019-01-25T17:00:10Z

docker-compose.yml

+
+
+networks:
+    frontend:


This network is useless, we already have a default one. https://docs.docker.com/compose/networking/#configure-the-default-network

voronkovich · 2019-01-25T17:04:45Z

Makefile

+	$(EXEC) $(CONSOLE) cache:clear --no-warmup
+	$(EXEC) $(CONSOLE) cache:warmup
+
+tty:                                                                                                   ## Run app container in interactive mode


So, maybe a bash would be more appropriate name? BTW, I think you forgot to add a -it flags to allocate tty.

mpiot · 2019-01-25T17:22:29Z

@voronkovich In the makefile the make tty use docker-compose exec then we don't need to add -it options.

I wait some other coments for this part, make tty, make bash or make terminal ?

jkufner · 2019-01-25T17:47:24Z

I wait some other coments for this part, make tty, make bash or make terminal ?

make shell

mpiot · 2019-01-25T18:11:36Z

@jkufner why not, now: 3 persons => 3 choices :-)

voronkovich · 2019-01-25T21:46:29Z

make shell sounds nice to me too.

mpiot · 2019-01-30T07:49:30Z

This is ok, I've rename the tty part in shell :-)

mpiot · 2019-02-01T08:32:26Z

What do you think about the naming for the php-cs-fixer part ? Because we name lint-yaml, lint twig, lint xliff, why not lint php ?

I'm pretty divided about it.

mpiot · 2019-03-29T10:28:21Z

Test fails because there is a vulnerability (Twig 2.7 required, but it will be fixed when #955 is merged)

At the moment, I've no updates used versions. Then maybe before merge, say it to me to define last versions. (PHP, node, composer, etc...)

athlan

One note about layers.

How about creating .dockerignore with .env files? Now they're uploaded, especially env.local and may conflict with env variables passed.

athlan · 2019-08-02T09:25:50Z

Dockerfile

+    make clean; \
+    make; \
+    make install; \
+    #Install the PHP extensions


I'd split it as separate RUN step, any mistake in app-specific set of docker-php-ext requires full rebuild of this later which takes ages.

I've add all of it in a single layout to follow Dockerfile best practices that recommend to do the less number of layer as possible. Then, when you want to add something in your image, you can split it to test before, then regroup it after ?

I think, it's not totaly usefull to split this part, because we just need the split sometimes when you want to change something, but otherwise we never change anything.

That's my opinion. But I understand what you say, because when I edit this part I split it for try, because this part (with ICU) is very long.

Wait coments, but if more people prefer, we can split after the ICU install.

mpiot · 2019-08-02T14:13:40Z

@athlan I've added a .dockerignore file.

athlan · 2019-08-02T14:17:22Z

Dockerfile

+#####################################
+##       PROD VENDOR BUILDER       ##
+#####################################
+FROM composer:${COMPOSER_VERSION} as vendor-builder


Here I'd inherit from app step, because if application relies on some extension installed before (in app step) (example bcmath) here this dependency will be missing. Proposal:

FROM app as vendor-builder ARG COMPOSER_VERSION # Install Composer RUN set -ex; \ EXPECTED_SIGNATURE="$(curl -L https://getcomposer.org/download/${COMPOSER_VERSION}/composer.phar.sha256sum)"; \ curl -L -o composer.phar https://getcomposer.org/download/${COMPOSER_VERSION}/composer.phar; \ ACTUAL_SIGNATURE="$(sha256sum composer.phar)"; \ if [ "$EXPECTED_SIGNATURE" != "$ACTUAL_SIGNATURE" ]; then >&2 echo 'ERROR: Invalid installer signature' && rm /usr/local/bin/composer && exit 1 ; fi; \ chmod +x composer.phar && mv composer.phar /usr/local/bin/composer; \ RESULT=$?; \ exit $RESULT;

Previously, I was doing like it, but I've change to avoid have composer in the final image.

At the moment on all my projects, I've never had this problem (but, sure, I've not test all dependencies). The goal of this part is to only install vendors and avoid to have composer in the final image.

That's good point 👍 . Build mechanisms should't be on final image.

However, final image won't have this dependency, because app-prod inherits from app, not vendor-builder. You wisely used only copy of /app files from vendor-builder while composer is located under /usr/local/bin/composer.

(hope it will be finally merged - good contribution I think)

Yes but if we install composer on app then, you have composer on app-prod and finaly on the image.

Do you have a case were it fails ? At the moment on all projects it work fine like it (use the composer image to install vendors).

For the last point, I'm not sure, this PR is open since a long time... :(

Yes yes, that's why my proposal is to have it only on vendor-builder (not app):

FROM app as vendor-builder

I do have a case for amqp not-native library, which requires bcmath. Otherwise I'd need to install amqp extension, so, both cases would require that chage.

athlan · 2019-08-02T14:57:57Z

.dockerignore

+Dockerfile
+Makefile
+phpunit.xml.dist
+README.md


I suggest add also:

/docker

/node_modules (might appear while development, where drive is mounted in docker-compose)

npm-debug.log

yarn-error.log

and others from .gitignore?

Poissible to miss some files yes, I've just copy a file from my repository, but I run the build in Travis, then some folder/files are never present.

For the docker folder, I can't because the Dockerfile need to access it.

Edit: no sorry, that's okay for docker to... Now I use a single app image (nginx + php-fpm) to deploy on azure, aws as exemple) but here, no problem.

Ah, yes, in case of CI/CD those folders probably wont appear. Tests outputs are also isolated in your makefiles. Just missed it checking build locally.

mpiot · 2019-08-02T15:07:04Z

You want to install composer in vendor-builder that extend app, that's it ? Then we install composer in the app-dev and vendor-builder ? That's possible, but, I'm not totally convinced. Because at the moment on some projects, I've never had a problem on vendor install because a php extension missed. Le ven. 2 août 2019 à 17:01, Piotr Pelczar <[email protected]> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In Dockerfile <#931 (comment)>: > + + +##################################### +## PROD ASSETS BUILDER ## +##################################### +FROM node:${NODE_VERSION} as assets-builder + +COPY . /app +WORKDIR /app + +RUN yarn install && yarn build && rm -R node_modules + +##################################### +## PROD VENDOR BUILDER ## +##################################### +FROM composer:${COMPOSER_VERSION} as vendor-builder Yes yes, that's why my proposal is to have it only on vendor-builder: FROM app as vendor-builder — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#931?email_source=notifications&email_token=ACPYEBAJTASX2GSEESAG3LTQCRD2XA5CNFSM4GPUSSRKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCANWQQQ#discussion_r310171951>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPYEBEDIQATD37X7FRGEA3QCRD2XANCNFSM4GPUSSRA> .

mstrom · 2019-09-06T18:00:14Z

Makefile

+
+deps: vendor assets                                                                                    ## Install the project dependencies
+
+


What do you think do we need to add command to update dependencies?

Suggested change

deps-update:

## Update the project dependencies

$(EXEC) composer update -n

$(EXEC) yarn upgrade

mstrom · 2019-09-06T18:09:52Z

Makefile

+assets: node_modules                                                                                   ## Build the development version of the assets
+	$(EXEC) yarn dev
+
+assets-build: node_modules                                                                             ## Build the production version of the assets


assets and assets-build both building assets
maybe use assets-prod instead of assets-build?

Suggested change

assets-build: node_modules ## Build the production version of the assets

assets-prod: node_modules ## Build the production version of the assets

ker0x · 2020-02-23T14:36:42Z

The project already encourages to use the Symfony binary, either to create the project or to launch the local server. Why not use the advantages it offers with Docker integration (https://symfony.com/doc/current/setup/symfony_server.html#docker-integration)?

landsman · 2024-04-22T14:55:08Z

It would be nice to finalize this and merge it. Can I help?

setineos · 2024-10-03T14:58:44Z

I think we should use the basic Docker configuration if we want to implement it in this repository.

landsman · 2024-10-14T08:01:23Z

It would be nice to merge at least something. It's here from 2019. 😢

mpiot force-pushed the implement-docker branch from 29b99cb to a8114e0 Compare January 13, 2019 12:46

javiereguiluz reviewed Jan 14, 2019

View reviewed changes

.env Outdated Show resolved Hide resolved

config/packages/doctrine.yaml Outdated Show resolved Hide resolved

mpiot force-pushed the implement-docker branch 2 times, most recently from 9bcfdd9 to d6e6c58 Compare January 14, 2019 19:50

Implement Docker

70f435b

mpiot force-pushed the implement-docker branch from d6e6c58 to 70f435b Compare January 14, 2019 19:53

stof reviewed Jan 16, 2019

View reviewed changes

Use a .conf file for Nginx

82ded75

voronkovich reviewed Jan 25, 2019

View reviewed changes

mpiot force-pushed the implement-docker branch from 517c4ff to c3240c0 Compare January 25, 2019 16:49

voronkovich reviewed Jan 25, 2019

View reviewed changes

mpiot force-pushed the implement-docker branch from b94025a to 0c45694 Compare January 30, 2019 07:48

mpiot added 7 commits March 29, 2019 11:00

Merge remote-tracking branch 'upstream/master'

fd6f473

Remove some useless parts

871f2c4

Remove MySQL parts

e1ca54a

Remove Migration and network from docker-compose

23c594e

Rename make tty to make shell

c380ea1

Fix Dockerfile

cf9c6c9

Remove/Update some forgetten parts

4f184d2

[Dockerfile] Set PHP timezone on UTC

561eb0a

mpiot force-pushed the implement-docker branch from e8b578e to 1ebe6e7 Compare March 29, 2019 10:00

Add some fixes

d618831

mpiot force-pushed the implement-docker branch from 1ebe6e7 to d618831 Compare March 29, 2019 10:46

athlan reviewed Aug 2, 2019

View reviewed changes

Add a Dockerignore file

c49b24a

athlan reviewed Aug 2, 2019

View reviewed changes

mstrom reviewed Sep 6, 2019

View reviewed changes

nicolas-grekas changed the base branch from master to main November 19, 2020 12:37

COil mentioned this pull request Dec 24, 2021

WIP: Feature/dockerize #1275

Closed

+deps-update:
+## Update the project dependencies
+	$(EXEC) composer update -n
+	$(EXEC) yarn upgrade

	assets-build: node_modules ## Build the production version of the assets
	assets-prod: node_modules ## Build the production version of the assets



		networks:
		frontend:

Implement docker #931

Are you sure you want to change the base?

Implement docker #931

Conversation

mpiot commented Jan 13, 2019 • edited Loading

javiereguiluz left a comment

Choose a reason for hiding this comment

mpiot commented Jan 14, 2019

javiereguiluz commented Jan 14, 2019

mpiot commented Jan 14, 2019

javiereguiluz commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voronkovich 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

mpiot Mar 1, 2019 • edited Loading

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

mpiot commented Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpiot commented Jan 25, 2019 • edited Loading

jkufner commented Jan 25, 2019

mpiot commented Jan 25, 2019

voronkovich commented Jan 25, 2019

mpiot commented Jan 30, 2019 • edited Loading

mpiot commented Feb 1, 2019

mpiot commented Mar 29, 2019 • edited Loading

athlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpiot commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athlan Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

mpiot Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

athlan Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

athlan Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

mpiot Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpiot commented Aug 2, 2019 via email

mstrom Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ker0x commented Feb 23, 2020

landsman commented Apr 22, 2024

setineos commented Oct 3, 2024

landsman commented Oct 14, 2024

mpiot commented Jan 13, 2019 •

edited

Loading

mpiot Mar 1, 2019 •

edited

Loading

mpiot commented Jan 25, 2019 •

edited

Loading

mpiot commented Jan 25, 2019 •

edited

Loading

mpiot commented Jan 30, 2019 •

edited

Loading

mpiot commented Mar 29, 2019 •

edited

Loading

athlan Aug 2, 2019 •

edited

Loading

mpiot Aug 2, 2019 •

edited

Loading

athlan Aug 2, 2019 •

edited

Loading

athlan Aug 2, 2019 •

edited

Loading

mpiot Aug 2, 2019 •

edited

Loading

mstrom Sep 6, 2019 •

edited

Loading