Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Implement docker #931

wants to merge 12 commits into from

Conversation

mpiot
Copy link

@mpiot mpiot commented Jan 13, 2019

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

Copy link
Member

@javiereguiluz javiereguiluz left a 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!

.env Outdated Show resolved Hide resolved
config/packages/doctrine.yaml Outdated Show resolved Hide resolved
@mpiot
Copy link
Author

mpiot commented Jan 14, 2019

@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
Copy link
Member

@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 mpiot force-pushed the implement-docker branch 2 times, most recently from 9bcfdd9 to d6e6c58 Compare January 14, 2019 19:50
@mpiot
Copy link
Author

mpiot commented Jan 14, 2019

@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
Copy link
Member

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.

echo ' return 404;'; \
echo ' }'; \
echo '}'; \
} > /etc/nginx/conf.d/default.conf
Copy link
Member

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.

Copy link
Author

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

Copy link
Contributor

@voronkovich voronkovich left a 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'; \
Copy link
Contributor

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?

Copy link
Author

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

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?

Copy link
Author

@mpiot mpiot Mar 1, 2019

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 :-)

add_header X-XSS-Protection "1; mode=block";
add_header X-Frame-Options SAMEORIGIN;

if ($http_user_agent ~* "WordPress") {
Copy link
Contributor

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?

Copy link
Author

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... :-(

try_files $uri /index.php$is_args$args;
}

location /protected-files/ {
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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

Copy link
Author

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
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Redis?

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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

@mpiot
Copy link
Author

mpiot commented Jan 25, 2019

@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);
Copy link
Contributor

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.



networks:
frontend:
Copy link
Contributor

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
Copy link
Contributor

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.

@mpiot
Copy link
Author

mpiot commented Jan 25, 2019

@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
Copy link
Contributor

jkufner commented Jan 25, 2019

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

make shell

@mpiot
Copy link
Author

mpiot commented Jan 25, 2019

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

@voronkovich
Copy link
Contributor

make shell sounds nice to me too.

@mpiot
Copy link
Author

mpiot commented Jan 30, 2019

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

@mpiot
Copy link
Author

mpiot commented Feb 1, 2019

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
Copy link
Author

mpiot commented Mar 29, 2019

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

Copy link

@athlan athlan left a 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
Copy link

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.

Copy link
Author

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.

@mpiot
Copy link
Author

mpiot commented Aug 2, 2019

@athlan I've added a .dockerignore file.

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

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;

Copy link
Author

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.

Copy link

@athlan athlan Aug 2, 2019

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)

Copy link
Author

@mpiot mpiot Aug 2, 2019

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... :(

Copy link

@athlan athlan Aug 2, 2019

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
Copy link

@athlan athlan Aug 2, 2019

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?

Copy link
Author

@mpiot mpiot Aug 2, 2019

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.

Copy link

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.

@mpiot
Copy link
Author

mpiot commented Aug 2, 2019 via email


deps: vendor assets ## Install the project dependencies


Copy link
Contributor

@mstrom mstrom Sep 6, 2019

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?

Suggested change
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
Copy link
Contributor

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?

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
Copy link
Contributor

ker0x commented Feb 23, 2020

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)?

@nicolas-grekas nicolas-grekas changed the base branch from master to main November 19, 2020 12:37
@COil COil mentioned this pull request Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants