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

Use upstream PHP FPM Alpine Docker image #144

Merged
merged 4 commits into from
Feb 12, 2018

Conversation

J0WI
Copy link
Contributor

@J0WI J0WI commented Jan 1, 2018

This switches the base to the upstream PHP FPM Alpine Docker image, so we can benefit from the maintenance there.
I included and configured the same php extensions as before.

This might be a dependency task of #56.

@nijel
Copy link
Contributor

nijel commented Jan 2, 2018

It fails to test as the test setup relies on using more recent apline (the PHP images are based on 3.4, while we were on 3.6).

That also pops up question whether it's better to stick with maintained alpine image having current all libs or maintained PHP image which has current PHP, but everything else is older. Do you know if Alpine 3.4 still receives security fixes?

@J0WI
Copy link
Contributor Author

J0WI commented Jan 2, 2018

I don't know why the tests are failing. Both versions build and run.
Can you point me to the relevant lines?

@nijel
Copy link
Contributor

nijel commented Jan 3, 2018

I don't see anything obvious, but apparently the container behaves differently than the original one.

The problem can be seen here - the test script is not found. Maybe the working directory is different than it used to be and that confuses the script lookup:

if [ -f ./phpmyadmin_test.py ] ; then
FILENAME=./phpmyadmin_test.py
else
FILENAME=./testing/phpmyadmin_test.py
fi

@J0WI
Copy link
Contributor Author

J0WI commented Jan 3, 2018

Thanks, I've updated the path in the tests.

@J0WI
Copy link
Contributor Author

J0WI commented Jan 31, 2018

@nijel would be great to get some feedback here.

Do you know if Alpine 3.4 still receives security fixes?

docker-library/php#504 (comment)

if [ -f ./phpmyadmin_test.py ] ; then
FILENAME=./phpmyadmin_test.py
if [ -f /phpmyadmin_test.py ] ; then
FILENAME=/phpmyadmin_test.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably both variants should stay to allow local testing.

@J0WI
Copy link
Contributor Author

J0WI commented Feb 10, 2018

I've re-added the old condition.

@nijel nijel self-assigned this Feb 12, 2018
@nijel nijel merged commit 3a01bac into phpmyadmin:master Feb 12, 2018
nijel added a commit that referenced this pull request Feb 12, 2018
Issue #144

Signed-off-by: Michal Čihař <michal@cihar.com>
nijel added a commit that referenced this pull request Feb 12, 2018
Issue #144

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel
Copy link
Contributor

nijel commented Feb 12, 2018

Merged, thanks for your contribution!

I've applied following fixups on top of your changes:

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