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

Add Docker setup #242

Merged
merged 2 commits into from Jul 29, 2018

Conversation

@teohhanhui
Copy link
Contributor

commented Jul 23, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets Sylius/Sylius#9414
License MIT

Replaces #232

@teohhanhui teohhanhui force-pushed the teohhanhui:add/docker-setup branch from 9f5253c to 5176bae Jul 23, 2018
@pamil pamil added RFC Feature labels Jul 23, 2018
@teohhanhui teohhanhui force-pushed the teohhanhui:add/docker-setup branch from 5176bae to bdac10c Jul 23, 2018
host: '%database_host%'
port: '%database_port%'
dbname: '%database_name%'
user: '%database_user%'
password: '%database_password%'
driver: pdo_mysql

This comment has been minimized.

Copy link
@pamil

pamil Jul 23, 2018

Member

Why not keep that parameter?

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 23, 2018

Author Contributor

Because it cannot be used in any meaningful way. 😄

This comment has been minimized.

Copy link
@pamil

pamil Jul 29, 2018

Member

Reverted this change for now.

@@ -10,13 +9,13 @@ parameters:

mailer_transport: '%env(SYLIUS_MAILER_TRANSPORT)%'
mailer_host: '%env(SYLIUS_MAILER_HOST)%'
mailer_port: '%env(SYLIUS_MAILER_PORT)%'

This comment has been minimized.

Copy link
@pamil

pamil Jul 23, 2018

Member

Could we extract this change to another PR?

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 23, 2018

Author Contributor

Sure. But we need it here...

@@ -52,6 +52,9 @@ watchShop.description = 'Watch shop asset sources and rebuild on changes.';
export const build = gulp.parallel(buildAdmin, buildShop);
build.description = 'Build assets.';

export const watch = gulp.parallel(watchAdmin, watchShop);
watch.description = 'Watch asset sources and rebuild on changes.';

This comment has been minimized.

Copy link
@pamil

pamil Jul 23, 2018

Member

Could we extract this change?

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 23, 2018

Author Contributor

Sure. But we need it here...

.travis.yml Outdated
- docker-compose pull --ignore-pull-failures
- docker-compose build --pull
- docker-compose up -d
- sleep 60

This comment has been minimized.

Copy link
@pamil

pamil Jul 23, 2018

Member

No idea how to solve that better, just leaving a comment so it's more visible

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 23, 2018

Author Contributor

I'm sure there are more fancy ways, but this is the simplest way that works. 😆

@@ -0,0 +1,108 @@
version: '3.4'

This comment has been minimized.

Copy link
@pamil

pamil Jul 23, 2018

Member

These two different files for dev and prod would be hard to keep in sync (eg. this version differs).

@jacquesbh mentioned this on Slack on Friday:

yes. And it's just a step away to create a docker-compose.traefik.yml and explain that docker-compose -f docker-compose.yml -f docker-compose.traefik.yml can be use

Can we use an architecture like this for those extra containers (eg. mailhog, nodejs)?

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 23, 2018

Author Contributor

It's not just about additional services...

@@ -0,0 +1,14 @@
apc.enable_cli = 1
date.timezone = ${PHP_DATE_TIMEZONE}
opcache.enable_cli = 1

This comment has been minimized.

Copy link
@pamil

pamil Jul 23, 2018

Member

Opcache in CLI might cause some issues during development

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 23, 2018

Author Contributor

Such as? :D

This comment has been minimized.

Copy link
@pamil

pamil Jul 29, 2018

Member

Once we had an issue when phpspec was running unexisting spec which had been removed before, the issue vanished after we disabled opcache.

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Jul 29, 2018

Author Contributor

To be honest, that sounds like a misconfiguration of OpCache, so we should be fine here.

@nietzscheson

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

The Docker configurations for the Sylius project should not be included in the standard version. The reasons that I consider main are:

  • There are many out there who do not even use Docker. They do not even use virtual machines.
  • The learning curve with Docker is quite complex. So its use should not be mandatory. Not all cloud payment systems allow the use of root to install certain packages on the virtual machine.
  • There are other containerization projects such as:
       - Docker Container for Windows (https://github.com/docker/for-win)
       - RKT (https://github.com/rkt/rkt)

If Docker is included in the Standard version, the configurations of the Vagrant configured for Sylius are also included.

In Sylius/Sylius#9414 @stefandoorn adds the following: "can it still be a separate repo? I prefer to have my infra stuff in separate repo, instead of combined with the application itself (SRP). Besides that, not everyone likes or is using Docker, so for these it's not needed to have it in the same repo."

Matter of which I totally agree.

So, all efforts to achieve a good configuration of Docker for Sylius (https://github.com/Sylius/docker) should focus on the repository that already exists and keep it separate.

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

@nietzscheson We've already had that discussion in Sylius/Sylius#9414 and on Slack. I don't see the need to bring it up again here...

@nietzscheson

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

So is. But this is where the code merges and I see that you want to reiterate what precisely was already discussed on the channel. I just want to make clear my position...

@teohhanhui teohhanhui force-pushed the teohhanhui:add/docker-setup branch from bdac10c to 9975227 Jul 23, 2018
@gabiudrescu

This comment has been minimized.

Copy link

commented Jul 24, 2018

Personally, I have no preference between having the Docker setup in Sylius/Sylius-Standard or as a separate repository.

Though I have a question: how can one re-use this setup when trying to contribute to Sylius/Sylius? through copy-paste of the docker and docker compose files?

Also, certain services are missing:

  • Selenium
  • Chrome headless
  • Redis or Memcache (for production environment, especially on multi-node setup, I find it mandatory)

Shouldn't we include these too?

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

how can one re-use this setup when trying to contribute to Sylius/Sylius? through copy-paste of the docker and docker compose files?

One way is to mount the host directory into, say, /src/Sylius/Sylius

Also, certain services are missing

Could be added in other PR(s)

@gabiudrescu

This comment has been minimized.

Copy link

commented Jul 24, 2018

Second thing I want to bring under your attention: running commands under the root user inside containers is going to mess up permissions on the files on the host.

Especially on development mode, I find it better to use another user inside the container that has UID and GID mapped with the one from the host. On the setup I generally use, I have the following in the docker file:

groupadd -g "$APPLICATION_GID" "$APPLICATION_GROUP"
useradd -u "$APPLICATION_UID" --home "/home/$APPLICATION_USER" --create-home --shell /bin/bash --no-user-group "$APPLICATION_GROUP" -g "$APPLICATION_GID"

This way, whenever entering the container, I make sure I use the $APPLICATION_USER and everything is OK.

Also, my understanding is that running commands inside docker containers as root may imply a security risk:

@gabiudrescu

This comment has been minimized.

Copy link

commented Jul 24, 2018

@teohhanhui

One way is to mount the host directory into, say, /src/Sylius/Sylius

can you offer me an example on how one can use this docker setup to test a modification it brought to the Sylius/Sylius vendor file?

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

@gabiudrescu

can you offer me an example on how one can use this docker setup to test a modification it brought to the Sylius/Sylius vendor file?

It's basically on its own, not as a vendor package. Just cd to that directory and do what you need to do.

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

As for running as root in the container, it's as we've discussed on Slack: there is no good solution that I'm aware of. But we could perhaps add support for running with a different uid/gid in the entrypoint.

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Switching user in the Dockerfile cannot work, because the built image has to be portable (and redistributable).

@jacquesbh

This comment has been minimized.

Copy link

commented Jul 24, 2018

User rights

For the user rights we have this:

Makefile:

export USER_UID=$(shell id -u)
example:
	docker-compose ps

In our Dockerfiles:

# Use www-data user
ARG USER_UID=1000
RUN usermod -u $USER_UID www-data

By default www-data has uid 1000.
On macOS the user id is 501 usually.

In our docker-compose.yml:

services:
  my-container:
    build:
      context: my-container/
      args:
        USER_UID: ${USER_UID}

This way, by using a simple variable, we avoid a lot of problems.

We don't have a makefile yet. Why not adding one? It's a good way to improve the developers flow.

Missing containers

The missing containers are for testing purpose.

This is why I propose to use a docker-compose.testing.yml. We don't need to run all the containers all the time, specially Selenium and headless browsers since they are consuming a lot.

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

@jacquesbh Anything that involves setting uid/gid in the Dockerfile is not a solution, because it'd result in an image that's not portable / redistributable.

@teohhanhui teohhanhui closed this Jul 24, 2018
@teohhanhui teohhanhui reopened this Jul 24, 2018
@jacquesbh

This comment has been minimized.

Copy link

commented Jul 24, 2018

This is portable, working on Windows, Linux and macOS. We use it everyday for many projects. So…

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Missing containers

Let's keep this first PR small?

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

No, I'm talking about the image being portable. It should be able to be used by any user regardless of their uid/gid on the host, without having to rebuild the image.

@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

We should support running with arbitrary uid/gid (via the --user flag of docker run and its equivalent in docker-compose.yml), but otherwise step down from root if necessary:

docker-library/mysql#397 (comment)
docker-library/php#70 (comment)

@teohhanhui teohhanhui force-pushed the teohhanhui:add/docker-setup branch from 9975227 to 0104725 Jul 24, 2018
@teohhanhui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

I hope we could have this PR merged soon, then others could help to make things better.

@teohhanhui teohhanhui force-pushed the teohhanhui:add/docker-setup branch from 0104725 to 5a70148 Jul 26, 2018
@pamil
pamil approved these changes Jul 29, 2018
@pamil pamil merged commit a56eecf into Sylius:master Jul 29, 2018
2 checks passed
2 checks passed
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pamil

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

Thank you, Teoh, let's make it the best Docker ever for 1.3 release 🎉

@teohhanhui teohhanhui deleted the teohhanhui:add/docker-setup branch Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.