-
Notifications
You must be signed in to change notification settings - Fork 336
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] Fix fixture images not being available #885 #887
Conversation
1. mount media volume to migrations container so fixture images are available in php and nginx container 2. change nginx user and group to match php's, so it can read image cache files
@@ -0,0 +1,31 @@ | |||
user www-data; |
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.
Do you think we can merge both configurations?
https://github.com/Sylius/Sylius-Standard/blob/1.12/docker/nginx/conf.d/default.conf
if they can be separate, then I would rather have them moved to conf.d directory and let nginx handle loading it
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.
The files from conf.d
are included in the http
section, they can't be merged and this file can't go to conf.d
.
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 how docs recommend to customize it: https://github.com/docker-library/docs/tree/master/nginx#complex-configuration
|
||
#gzip on; | ||
|
||
include /etc/nginx/conf.d/*.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.
@Ferror this is where the files from conf.d
are included. This config file is a copy of the one that was shipped with this image, I just changed the user.
I hate root containers... the problem still persist becuase the migrations container runs as root and the image files are created with root user, but now the nginx and fpm process run with a non-root user... |
… with user www-data
@@ -22,4 +22,7 @@ php bin/console doctrine:migrations:migrate --no-interaction | |||
|
|||
if [ "$LOAD_FIXTURES" = "1" ]; then | |||
php bin/console sylius:fixtures:load --no-interaction | |||
|
|||
# make the image files created by fixtures accessible by fpm which runs with user www-data | |||
chown -R www-data:www-data public/media/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.
@Ferror this will fix the issue, but it's just like applying FlexTape... ideally we should adopt non-root containers and make GIDs and UIDs configurable via build args and env vars, like this: https://github.com/laradock/laradock/blob/96e0f2e92f6e5b128a71cc77a05b297ac4a0eadb/workspace/Dockerfile#L43-L47
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 realized that if I launched the fixtures manually from the php container, I had to reset the rights each time afterwards.
php bin/console sylius:fixtures:load -n && chown -R www-data:www-data public/media/image
PS. The pipeline probably fails because MySQL is taking a little more time to spin up. Let's try to increment the attempt amount to 25. It's still gonna be only 25 seconds in the worst-case scenario, and in real life, the database would be always available. https://github.com/Sylius/Sylius-Standard/blob/1.12/docker/migrations/docker-entrypoint.sh#L4 |
I have the same issue. Can we merge this? |
@vvasiloi who do we need to bribe to get this merged in master? |
This should not be merged. It's not a good solution. We should use non-root containers instead. |
Fix #885