-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement docker #931
base: main
Are you sure you want to change the base?
Implement docker #931
Conversation
29b99cb
to
a8114e0
Compare
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.
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!
@javiereguiluz okay, I see many possibilities:
I think the first proposal is better, then I also can add a comment in docker near the PDO extension ? |
@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. |
9bcfdd9
to
d6e6c58
Compare
d6e6c58
to
70f435b
Compare
@javiereguiluz I've fix it :-) |
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. |
docker/NginxDockerfile
Outdated
echo ' return 404;'; \ | ||
echo ' }'; \ | ||
echo '}'; \ | ||
} > /etc/nginx/conf.d/default.conf |
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.
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.
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.
@stof I agrre with you, before I did it like that. I don't remember why I've change it to use this echo...
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.
IMHO It's better to drop all the MySql-related stuff, because it looks very confusing.
Dockerfile
Outdated
RUN { \ | ||
echo 'date.timezone = Europe/Paris'; \ | ||
echo 'short_open_tag = off'; \ | ||
echo 'memory_limit = 8192M'; \ |
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.
Maybe it's better to just use: memory_limit = -1
?
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.
I agree, I've just copied some files used on a personnal project, and I've forget to removed some parts...
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.
By the way, date.timezone
can be set to UTC so everyone will have same dates on the app, WDYT?
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.
@Pierstoval Because I'm in France I've configure it like that, but you're totaly right.
Thanks a lot :-)
docker/nginx-default.conf
Outdated
add_header X-XSS-Protection "1; mode=block"; | ||
add_header X-Frame-Options SAMEORIGIN; | ||
|
||
if ($http_user_agent ~* "WordPress") { |
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.
Why did you add this block?
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.
Mmm, it's about security, but useless in our case too, I've really forget a lot of parts... :-(
docker/nginx-default.conf
Outdated
try_files $uri /index.php$is_args$args; | ||
} | ||
|
||
location /protected-files/ { |
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.
Why did you add this block?
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.
It's to store protected files and use X-Accel-Redirect, idem, forget...
Dockerfile
Outdated
|
||
# Used for the ICU compilation | ||
ENV PHP_CPPFLAGS="${PHP_CPPFLAGS} -std=c++11" | ||
ENV APP_VERSION=0.0.0 |
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.
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
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.
I agree, it's interesting :-) I do it ;-)
Makefile
Outdated
$(EXEC) $(CONSOLE) cache:clear --no-warmup | ||
$(EXEC) $(CONSOLE) cache:warmup | ||
|
||
tty: ## Run app container in interactive mode |
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.
I would suggest to use sh
instead o tty
here.
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.
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
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.
So, maybe a bash
would be more appropriate name? BTW, I think you forgot to add a -it
flags to allocate tty.
Makefile
Outdated
|
||
clear: perm ## Remove all the cache, the logs, the sessions and the built assets | ||
$(EXEC) rm -rf var/cache/* | ||
$(EXEC) $(CONSOLE) redis:flushall -n |
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.
Redis?
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.
Idem...
Makefile
Outdated
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 |
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.
So, If we don't use the MySql all these command will wait 60 seconds?
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.
Yes, in fact... I can remove all the db part, or just remove the waiting part and the db-reset part.
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.
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.
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.
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...
517c4ff
to
c3240c0
Compare
@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 ? |
@@ -0,0 +1,46 @@ | |||
<?php declare(strict_types=1); |
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.
@mpiot, You forgot to remove this migration file.
docker-compose.yml
Outdated
|
||
|
||
networks: | ||
frontend: |
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.
This network is useless, we already have a default
one. https://docs.docker.com/compose/networking/#configure-the-default-network
Makefile
Outdated
$(EXEC) $(CONSOLE) cache:clear --no-warmup | ||
$(EXEC) $(CONSOLE) cache:warmup | ||
|
||
tty: ## Run app container in interactive mode |
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.
So, maybe a bash
would be more appropriate name? BTW, I think you forgot to add a -it
flags to allocate tty.
@voronkovich In the makefile the I wait some other coments for this part, |
make shell |
@jkufner why not, now: 3 persons => 3 choices :-) |
|
b94025a
to
0c45694
Compare
This is ok, I've rename the |
What do you think about the naming for the I'm pretty divided about it. |
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...) |
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.
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.
make clean; \ | ||
make; \ | ||
make install; \ | ||
#Install the PHP extensions |
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.
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.
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.
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.
@athlan I've added a .dockerignore file. |
##################################### | ||
## PROD VENDOR BUILDER ## | ||
##################################### | ||
FROM composer:${COMPOSER_VERSION} as vendor-builder |
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.
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;
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.
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.
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.
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)
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.
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... :(
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.
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.
Dockerfile | ||
Makefile | ||
phpunit.xml.dist | ||
README.md |
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.
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?
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.
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.
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.
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.
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>
.
|
|
||
deps: vendor assets ## Install the project dependencies | ||
|
||
|
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 do you think do we need to add command to update dependencies?
deps-update: | |
## Update the project dependencies | |
$(EXEC) composer update -n | |
$(EXEC) yarn upgrade |
assets: node_modules ## Build the development version of the assets | ||
$(EXEC) yarn dev | ||
|
||
assets-build: node_modules ## Build the production version of the assets |
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.
assets
and assets-build
both building assets
maybe use assets-prod
instead of assets-build
?
assets-build: node_modules ## Build the production version of the assets | |
assets-prod: node_modules ## Build the production version of the assets |
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)? |
It would be nice to finalize this and merge it. Can I help? |
This PR provides a Docker implementation for the Symfony Demo project. (based on https://github.com/mpiot/docker4symfony)