-
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
Dockerfile and docker-compose update for php version 7.4 #536
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
49976be
Update dockerfile and docker-compose file to work with php 7.4
arti0090 45b276f
restore composer.lock for better performance built
arti0090 fe56227
Revert "restore composer.lock for better performance built"
arti0090 71d3ce0
nodejs to node
arti0090 084405a
Add fix for composer files
arti0090 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,12 @@ services: | |
build: | ||
context: . | ||
target: sylius_php | ||
cache_from: | ||
- quay.io/sylius/php:latest | ||
- quay.io/sylius/nodejs:latest | ||
- quay.io/sylius/nginx:latest | ||
image: quay.io/sylius/php:latest | ||
# Quay does not work, should be replaced in future with f.e. ghcr.io | ||
# cache_from: | ||
# - quay.io/sylius/php:latest | ||
# - quay.io/sylius/nodejs:latest | ||
# - quay.io/sylius/nginx:latest | ||
image: php:latest | ||
depends_on: | ||
- mysql | ||
environment: | ||
|
@@ -42,15 +43,16 @@ services: | |
ports: | ||
- "3306:3306" | ||
|
||
nodejs: | ||
node: | ||
build: | ||
context: . | ||
target: sylius_nodejs | ||
cache_from: | ||
- quay.io/sylius/php:latest | ||
- quay.io/sylius/nodejs:latest | ||
- quay.io/sylius/nginx:latest | ||
image: quay.io/sylius/nodejs:latest | ||
target: sylius_node | ||
# Quay does not work, should be replaced in future with f.e. ghcr.io | ||
# cache_from: | ||
# - quay.io/sylius/php:latest | ||
# - quay.io/sylius/nodejs:latest | ||
# - quay.io/sylius/nginx:latest | ||
Comment on lines
+50
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commented config (unless left for reference for users) should be removed - as it is just a dead "code" |
||
image: node:latest | ||
depends_on: | ||
- php | ||
environment: | ||
|
@@ -67,14 +69,15 @@ services: | |
build: | ||
context: . | ||
target: sylius_nginx | ||
cache_from: | ||
- quay.io/sylius/php:latest | ||
- quay.io/sylius/nodejs:latest | ||
- quay.io/sylius/nginx:latest | ||
image: quay.io/sylius/nginx:latest | ||
image: nginx:latest | ||
# Quay does not work, should be replaced in future with f.e. ghcr.io | ||
# cache_from: | ||
# - quay.io/sylius/php:latest | ||
# - quay.io/sylius/nodejs:latest | ||
# - quay.io/sylius/nginx:latest | ||
depends_on: | ||
- php | ||
- nodejs # to ensure correct build order | ||
- node # to ensure correct build order | ||
volumes: | ||
- ./public:/srv/sylius/public:ro | ||
# if you develop on Linux, you may use a bind-mounted host directory instead | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,17 @@ | ||
server { | ||
root /srv/sylius/public; | ||
listen *:80; | ||
|
||
location / { | ||
# try to serve file directly, fallback to index.php | ||
try_files $uri /index.php$is_args$args; | ||
} | ||
|
||
location ~ ^/index\.php(/|$) { | ||
# Comment the next line and uncomment the next to enable dynamic resolution (incompatible with Kubernetes) | ||
fastcgi_pass php:9000; | ||
resolver 127.0.0.11 valid=10s ipv6=off; | ||
set $backendfpm "php:9000"; | ||
fastcgi_pass $backendfpm; | ||
# Comment the next line and uncomment the next to enable dynamic resolution (incompatible with Kubernetes); | ||
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't they inversed ? |
||
#resolver 127.0.0.11; | ||
#set $upstream_host php; | ||
#fastcgi_pass $upstream_host:9000; | ||
|
File renamed without changes.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
are you sure this should be removed?
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 composer.lock file is not needed anymore (since composer v2 as i remember?). The composer is creating it when building containers and modify directory on your machine (the composer.lock is created). I might be wrong with my thinking and the composer.lock file might be needed to prevent reinstalation.
When I checked manualy I made some changes in code of application, and run
docker-compose build
and the composer was running from cache ( no reinstallation of vendors happened).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 somebody with docker experience is looking here please give an opinion, I am still learning docker 😄
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.
@arti0090 Thanks for your PR!
According to the composer docs here: https://getcomposer.org/doc/01-basic-usage.md#installing-with-composer-lock it is a good practice to commit your project's composer.lock file to version control. Also it makes sense to copy this file to Docker's context during image build, because composer then don't need to resolve the latest versions of a dependency but simply grabs the locked version of a dependency in the composer.lock file. This also ensures that you have a controlled Docker image build. Else it may happen that some dependencies changes during your Docker image build process which results in another behaviour.
So with the composer.lock file you can have controlled way of updating your dependencies, without it each
composer install
execution may install some other versions of a dependency which may produce some error in your application.As far as I see this Sylius-Standard repo also does not include such a composer.lock file, so you need to make sure that you have run composer install (or composer create-project...) before you build a Docker image, else the Docker image build will fail with the message that "the composer.lock file does not exist...". See also #503 which proposes to remove composer.lock file from the .gitignore file.
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.
Just a small question here, how would you run
composer install
if you rely on the PHP & Composer provided by the Docker image (which, as you said, will fail to built on first run b/c there is nocomposer.lock
)?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 @ilimination for your review comment, unfortunately I don't think that composer.lock file can be left. As in Sylius there are is a matrix of version supported (like php 7.4 or 8.0 and different mysql's) and leaving composer lock might interfere with it. Speaking about splitting prod and dev in two - well te bo honest I struggle with my experience at the moment to make it right 😄 .
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.
A fix for being able to build the Docker image before
composer install
(without ancomposer.lock
file) was proposed here: #559There 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.
Including
composer.lock
in the Sylius-Standard repo might indeed interfere with the various PHP versions, but only providing instructions to Docker to COPY your owncomposer.lock
file once it's initialized should not interfere.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 have added the line from #559 into the PR. Credits to you @tarnawski , hope this won't be a problem to use your code 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.
A more proper way will be merge #559 and then rebase your changes to be aline with the master, but as is a small improvement - take it as yours 😉