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

Docker compose #56

Merged
merged 28 commits into from
Oct 30, 2018
Merged

Docker compose #56

merged 28 commits into from
Oct 30, 2018

Conversation

sdecandelario
Copy link
Contributor

I added a docker-compose, taking as example the old one and making some improvements. Is opened to discussion and to improve the ecosystem.

# This file is auto-generated during the composer install
parameters:
locale: en
secret: f89f0fadda52bf9275459ef5ae135712e29e95a0
Copy link

Choose a reason for hiding this comment

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

It should be an env var.

server_name api.codelytv.dev;
root /app/applications/api/web;

error_log /var/log/nginx/error.log;
Copy link

Choose a reason for hiding this comment

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

Why not stdout and stderr?

build:
context: etc/infrastructure/php
ports:
- "2244:22"
Copy link

Choose a reason for hiding this comment

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

No ssh in Dockerfile

- PHP_IDE_CONFIG=serverName=CodelyCQRSCourse
depends_on:
- mysql
working_dir: "/app"
Copy link

Choose a reason for hiding this comment

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

Why not define this in the Dockerfile instead?

RUN apk --update upgrade \
&& apk add autoconf automake make gcc g++

RUN apk add rabbitmq-c rabbitmq-c-dev \
Copy link

Choose a reason for hiding this comment

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

A single run instruction is much recommended here

&& pecl install amqp apcu\
&& docker-php-ext-enable amqp apcu

CMD ["sh", "-c", "composer install && php-fpm"]
Copy link

Choose a reason for hiding this comment

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

Why composer install is container start? The provisioning should be in an early stage IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think for development is nice when you start the environment that install dependencies, thinking about someone new to the project. Or it's better to put in the Dockerfile instead?

Copy link
Member

@JavierCane JavierCane Oct 30, 2018

Choose a reason for hiding this comment

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

I would also go for the approach suggested by @jorge07. The reason why is because provisioning (running the composer install) in an early stage of our Dockerfile would allow us to perform some optimizations 😬

One of these optimizations would be applying the builder pattern or Multistage builds. You can read more information about them in the "Docker: De 0 a deploy" CodelyTV Pro course by @fiunchinho (here you have the course repository with some examples), and a blog post about this technic and its benefits.

However, we've tried to apply this strategy and we've miserably failed (🤣). The reason why we haven't been able to do so is because the PHP Dockerfile is in a subdirectory and it can't copy the composer.json file (it is inside the parent folder).

I've opened an isolated issue in order to deal with this (comments and suggestions welcomed!). This way we can go ahead merging all the improvements contained in this PR 🙂

@xserrat
Copy link

xserrat commented Oct 14, 2018

Hi @sdecandelario ,

I've found a way to import an initial database using the official mysql image you use in the docker-compose.yml file. You only need to create a Dockerfile that uses the same image and then, copy the content of the databases folder to /docker-entrypoint-initdb.d.

etc/infrastructure/mysql/Dockerfile:

FROM mysql:5.7.23

COPY etc/infrastructure/mysql/databases /docker-entrypoint-initdb.d

here I've moved the video.sql file under etc/infrastructure/mysql/databases

And docker-compose.yml:

 mysql:
        build:
            dockerfile: etc/infrastructure/mysql/Dockerfile
            context: .
        container_name: cqrs_codely_tv_mysql
        ports:
            - "3306:3306"
        env_file:
            - .env

I hope it helps :)

See documentation https://hub.docker.com/_/mysql/ section Initializing a fresh instance:

Initializing a fresh instance:
When a container is started for the first time, a new database with the specified name will be created and initialized with the provided configuration variables. Furthermore, it will execute files with extensions .sh, .sql and .sql.gz that are found in /docker-entrypoint-initdb.d. Files will be executed in alphabetical order. You can easily populate your mysql services by mounting a SQL dump into that directory and provide custom images with contributed data. SQL files will be imported by default to the database specified by the MYSQL_DATABASE variable.

@sdecandelario
Copy link
Contributor Author

Hello @xserrat, that's a nice point. But maybe it's more interesting mount a volume mapping a directory of the project with the mysql docker image? In this way you don't need to change the Dockerfile and everyone can use it without effort :)

@xserrat
Copy link

xserrat commented Oct 19, 2018

@sdecandelario, I totally agree with you! Mapping the /docker-entrypoint-initdb.d directory in the docker-compose.yml it's much more simple :)

sdecandelario and others added 4 commits October 22, 2018 19:38
* Docker Compose:
    * Do not specify patch versions of the used images. Example: from `nginx:1.15.5-alpine` to `nginx:1.15-alpine`. Why: allow to upgrade automatically to a nugfix versions (we're using this repo as a example and we don't need to maintain a strict relation with a production environment)
    * Change the ports mapping to a non standard ones in the host machine. Example: Nginx from port `80` to port `8030`. Why: Avoid collisions with other containers or services exposed in these ports in the host machine. We have set the `XX30` as the ending port for this project (don't ask why 🤷‍♂️)
    * Renamed the container names. Example: from `cqrs_codely_tv_nginx` to `codelytv-cqrs_ddd_php_example-nginx`. Why: Follow a standard naming in order to avoid collisions and make it easier to find the needed container. Convention: `%vendor%-%repository_or_project%-%docker_service%`
    * Changed the indentation in the `docker-compose.yml` from 4 to 2 spaces. Why: Follow the same convention as in the official Docker Compose website and community examples
    * Modified the volume mapping `$PWD` by the shorter version of the current directory: `.`

* Nginx:
    * Renamed `/etc/infrastructure/nginx/site.conf` to `default.conf`. Why: Maintain the same standard name used by Nginx

* PHP:
    * Updated the `PHP_IDE_CONFIG=serverName` value to `CodelyTvCqrsDddPhpExample` in order to maintain the same naming conventions suggested avobe
    * Dockerfile:
        * Removed the `maintainer` label since we're not publishing this image in any Docker Registry and it could be confusing for other collaborators to expect updates from an ocassional contributor to the repository
        * Added pdo and pdo_mysql extensions installation because we need them to run production code
        * Added xdebug installation because we need them to debug while developing
        * Unified all the commands to execute in order to save one temporary Docker image
        * Added `php.ini` and `xdebug.ini` configurations
        * Updated the `composer install` command in order to run it without expecting user interaction and optimizing the autoloader class map

* MySQL:
    * Modified the mapped directory to the `/docker-entrypoint-initdb.d` one from `$PWD/etc/infrastructure/mysql/data` to the currently available `./databases`. Why: This directory already contains all the databases and tables definitions. We just need to add the `CREATE DATABASE` and `USE` sentences in order to be fully compatible with the new standard way to initialize DB structures proposed by MySQL official image 🤟
    * Added the `restaty: always` clause to the MySQL container in order to restard it when it fails

* RabbitMQ:
    * Added the `rabbitmq` container in order to serve as a message broker server and be able to perform integration tests taking it into account
    * We've decided to go for the `rabbitmq:3.7-management` version in order to be able to have the RabbitMQ administration frontend available

* Others:
    * Modified the `.env.dist` default values in order to maintain the same naming conventions suggested avobe while creating the DB, and use different values for each one of the variables in order to differentiate them
Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

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

Thanks to all the people involved in this PR:

Now, let's talk about the changes done:

We've done some changes here and there. I hope you don't mind jumping directly into your branch in order to perform these changes, but this way I can just continue the work where you left it without asking you to do further work 🙂

However, in order to let you know why we've done the committed changes, we've tried to isolate them into several commits with descriptive commit messages. Here you have the summary of the commit messages.

Please, let me know what do you think about the changes. We can discuss further modifications in other issues or PRs.

Thanks again for collaborating with this repo!

Modifications to the Docker Compose:

  • Docker Compose:

    • Do not specify patch versions of the used images. Example: from nginx:1.15.5-alpine to nginx:1.15-alpine. Why: allow to upgrade automatically to a nugfix versions (we're using this repo as a example and we don't need to maintain a strict relation with a production environment)
    • Change the ports mapping to a non standard ones in the host machine. Example: Nginx from port 80 to port 8030. Why: Avoid collisions with other containers or services exposed in these ports in the host machine. We have set the XX30 as the ending port for this project (don't ask why 🤷‍♂️)
    • Renamed the container names. Example: from cqrs_codely_tv_nginx to codelytv-cqrs_ddd_php_example-nginx. Why: Follow a standard naming in order to avoid collisions and make it easier to find the needed container. Convention: %vendor%-%repository_or_project%-%docker_service%
    • Changed the indentation in the docker-compose.yml from 4 to 2 spaces. Why: Follow the same convention as in the official Docker Compose website and community examples
    • Modified the volume mapping $PWD by the shorter version of the current directory: .
  • Nginx:

    • Renamed /etc/infrastructure/nginx/site.conf to default.conf. Why: Maintain the same standard name used by Nginx
  • PHP:

    • Updated the PHP_IDE_CONFIG=serverName value to CodelyTvCqrsDddPhpExample in order to maintain the same naming conventions suggested avobe
    • Dockerfile:
      • Removed the maintainer label since we're not publishing this image in any Docker Registry and it could be confusing for other collaborators to expect updates from an ocassional contributor to the repository
      • Added pdo and pdo_mysql extensions installation because we need them to run production code
      • Added xdebug installation because we need them to debug while developing
      • Unified all the commands to execute in order to save one temporary Docker image
      • Added php.ini and xdebug.ini configurations
      • Updated the composer install command in order to run it without expecting user interaction and optimizing the autoloader class map
  • MySQL:

    • Modified the mapped directory to the /docker-entrypoint-initdb.d one from $PWD/etc/infrastructure/mysql/data to the currently available ./databases. Why: This directory already contains all the databases and tables definitions. We just need to add the CREATE DATABASE and USE sentences in order to be fully compatible with the new standard way to initialize DB structures proposed by MySQL official image 🤟
    • Added the restaty: always clause to the MySQL container in order to restard it when it fails
  • RabbitMQ:

    • Added the rabbitmq container in order to serve as a message broker server and be able to perform integration tests taking it into account
    • We've decided to go for the rabbitmq:3.7-management version in order to be able to have the RabbitMQ administration frontend available
  • Others:

    • Modified the .env.dist default values in order to maintain the same naming conventions suggested avobe while creating the DB, and use different values for each one of the variables in order to differentiate them

Minor changes

  • Remove unused SYMFONY_SECRET env var
  • Remove unused global/config/parameters.yml
  • Fix for "Required parameter $trustedHeaderSet missing" error. More info: Required parameter $trustedHeaderSet missing #57
  • Update README.md instructions on how to edit the local hosts file and check if the API is properly working

@JavierCane JavierCane merged commit 022db00 into CodelyTV:master Oct 30, 2018
@sdecandelario
Copy link
Contributor Author

Okey, perfect @JavierCane for merging and making some changes to the code, I will take a look and learn for further improvement for other cases, proud to can contribute to Codely :)

@xserrat
Copy link

xserrat commented Nov 5, 2018

Congrats @sdecandelario 👏 and awesome repo @JavierCane @rgomezcasas :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants