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

Update Dockerfile to support PHP 8 #1272

Open
wants to merge 2 commits into
base: skosmos-2
Choose a base branch
from

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Feb 15, 2022

Reasons for creating this PR

Support PHP 8 when using the test Docker image.

Link to relevant issue(s), if any

Description of the changes in this PR

Used the same Dockerfile changes I used to test the linked pull request.

Known problems or uncertainties in this PR

There are existing images that support PHP 8 and produce smaller containers, but I think it would be better to move that work for another issue.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@sonarcloud
Copy link

sonarcloud bot commented Mar 1, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
4.0% 4.0% Duplication

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #1272 (83b9cfd) into master (5d193c2) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1272   +/-   ##
=========================================
  Coverage     70.68%   70.68%           
  Complexity     1646     1646           
=========================================
  Files            32       32           
  Lines          3786     3786           
=========================================
  Hits           2676     2676           
  Misses         1110     1110           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d193c2...83b9cfd. Read the comment docs.

@osma
Copy link
Member

osma commented Mar 31, 2022

Thanks for the PR!

Now that I see the changes, I'm not sure this is the best way to support PHP 8. Basically this is taking an Ubuntu 20.04 base image (which by default has PHP 7.4 packages), then activating the ondrej/php repository and installing PHP 8.0 packages from that repo. I think it would be better to use a base image that directly provides PHP 8.0 packages. You mentioned this above:

There are existing images that support PHP 8 and produce smaller containers, but I think it would be better to move that work for another issue.

Do you have any specifics on this - what base image alternatives are there?

We could also wait for Ubuntu 22.04, which is due in a month from now. But it will have PHP 8.1, which is not yet supported in Skosmos although we have a related open issues #1274 and #1291.

@kinow
Copy link
Collaborator Author

kinow commented Mar 31, 2022

There are existing images that support PHP 8 and produce smaller containers, but I think it would be better to move that work for another issue.

Do you have any specifics on this - what base image alternatives are there?

We could probably use some small image like php:8.1.4-fpm-alpine3.15. Although I had a look at the compressed size, and both this PHP 8 and Ubuntu 20.04 have similar compressed size. Interesting.

Nonetheless, we could use the PHP official images, with the extra things required for Skosmos, instead of using Ubuntu as base image. The Docker file would be smaller, and I think it would be easier to upgrade to newer versions of PHP, as long as there was a tag in the official PHP image.

From what I recall, I did not use the PHP images because I used our installation instructions as reference, and wanted to stay as close as possible to that documentation (although my memory often fails me, but I think that's the reason for using Ubuntu.)

We could also wait for Ubuntu 22.04, which is due in a month from now. But it will have PHP 8.1, which is not yet supported in Skosmos although we have a related open issues #1274 and #1291.

This sounds like an easy fix, unless we have the need to upgrade the Docker files immediately. I think I only created this PR because I had it from some tests for Skosmos + PHP 8 after you fixed the build for this newer version of PHP.

Thanks @osma!

Bruno

@kinow kinow marked this pull request as draft May 24, 2022 11:10
@sonarcloud
Copy link

sonarcloud bot commented May 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@kinow kinow marked this pull request as ready for review May 25, 2022 07:45
@kinow
Copy link
Collaborator Author

kinow commented May 25, 2022

@osma, now using the official php Docker image that ships with Apache httpd. The composer is being fetched from the official Composer image too.

If you start docker-compose the UI should come up, but there is a warning in the cache image. That is fixed in #1294 👍

-Bruno

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

Successfully merging this pull request may close these issues.

2 participants