-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Extend Container build to support multiple PHP version #113
Conversation
'We will need this in the future to build docker containers with multiple versions of PHP.
@@ -69,7 +73,7 @@ jobs: | |||
# list of Docker images to use as base name for tags | |||
images: ${{secrets.IMAGE_NAME}} | |||
flavor: | | |||
suffix=${{matrix.flavor}} | |||
suffix=${{matrix.php-version}}-${{matrix.flavor}} |
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.
You should also add this to the test workflow as well so we can see what the output is when it builds this PR. I'm not 100% sure, but I believe this change would prevent latest
from being updated.
@@ -45,7 +47,7 @@ RUN cd caddy/frankenphp && \ | |||
|
|||
ENTRYPOINT ["/bin/bash","-c"] | |||
|
|||
FROM php:8.2.0RC6-zts-bullseye AS final | |||
FROM php:${php_base_tag}-bullseye AS final |
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.
Instead of using build-args, we should perhaps move the Dockerfiles into a dedicated directory /dockerfiles/8.2/Dockerfile
, etc.
If we need to go back to manually building PHP again for some reason, it would be nice if that didn't affect every single PHP version and it may be further complicated if the build steps are different between PHP versions. FWIW, it's likely to be somewhat different for PHP 8.3 due to some changes in Fibers (new build switches), at least for x86.
I think this would also simplify the matrix somewhat.
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.
Maybe should we use the same templating system as official images?
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 found this after trying to migrate our PHP 8.1 dev environment to use FrankenPHP instead of nginx-- looks like a great project. Is FrankenPHP version locked to a minimum 8.2 or would it be possible to retain a 8.1 version? |
@jamogriff, PHP 8.1 is lacking the necessary bits (mostly related to OS signaling if I understand things correctly) to allow frankenphp to work, so 8.2 will probably always be the minimum version. |
@withinboredom Awesome to knowz thanks! I'll also set the dev stack I'm building to use minimum 8.2 also. Thanks for the kind comments 🤙 |
Fixed by #163. Thanks for having initialized that work @mariusbuescher! |
This extends the Container build to support multiple versions of PHP in the FrankenPHP container images. This will make the build future proof on the one hand, so adding new versions of PHP will become a one-line change.
On the other hand, this will also make it possible to add a specifig PHP version tag to the container image.
Closes #112