-
Notifications
You must be signed in to change notification settings - Fork 450
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 docker-compose test environment based on CI's tests #110
Conversation
- Add MariaDB 10.1 version - Add MySQL 5.7 version - Link databases to PHPMyAdmin container
d159581
to
1cd0aef
Compare
- Add pip and curl Alpine packages - Add env variable to run or not the tests - Create MariaDB and MySQL test process - Create supervisor test config file - Run tests inside or not container - Run tests automatically on docker provision
1cd0aef
to
4d71a9c
Compare
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 for your contribution, but overall I'm not sure this is good approach.
It includes part of the test setup in the production Docker container which will get published on Docker hub. This will make the image bigger and contain not used software while the testing will still not work as it needs additional tweaking done in docker-compose.testing.yml.
What might be better approach to build the testing container on top of the production one adding all necessary files.
PS: Also when doing something like this, it would be great to test it in Travis to ensure we don't break it in the future.
@@ -1,7 +1,7 @@ | |||
FROM alpine:3.5 | |||
|
|||
# Install dependencies | |||
RUN apk add --no-cache php7-session php7-mysqli php7-mbstring php7-xml php7-gd php7-zlib php7-bz2 php7-zip php7-openssl php7-curl php7-opcache php7-json nginx php7-fpm supervisor | |||
RUN apk add --no-cache curl php7-session php7-mysqli php7-mbstring php7-xml php7-gd php7-zlib php7-bz2 php7-zip php7-openssl php7-curl php7-opcache php7-json nginx php7-fpm py2-pip supervisor |
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.
This pulls whole python to the image, I'd prefer to avoid that.
; include files themselves. | ||
|
||
[include] | ||
files = /etc/supervisor.d/nginx.ini /etc/supervisor.d/php.ini |
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.
Couldn't this be handled somehow better? Having two almost same configuration files doesn't sound good for maintenance.
exit $ret | ||
fi | ||
|
||
# List processes | ||
docker exec "$NAME" ps faux | ||
echo -e "Result of ${PHPMYADMIN_DB_HOSTNAME} tests: ${GREEN}SUCCESS${NC}" |
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 echo -e
does not work in many /bin/sh implementations, it's better to avoid it (see Travis log for example).
Thanks your feedback @nijel. My mindset was only related to development mode and really the updates include unnecessary changes for production. So I will close this pull request and create a new with only docker-compose.testing.yml updates, because the compose test file not working. That's OK? |
I've tried to base on your changes in #111 (but it probably still needs some polishing). |
I've based on your changes and implemented solution with additional Dockerfile to overlay the production one for testing. Also the docker-compose.testing.yml is now used during CI to ensure it's kept consistent with future changes. |
Simulate the CI's tests with the docker-compose environment running in local. Adding the changes:
- Add pip and curl Alpine packages on Dockerfile
- Add env variable to run or not the tests
- Create MariaDB and MySQL supervisor process
- Create supervisor test config file
- Run tests inside or not container
- Run tests automatically on docker provision