Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Docker] Switched image format and added PostgreSQL service #590

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

mnocon
Copy link
Member

@mnocon mnocon commented Aug 3, 2020

JIRA: https://jira.ez.no/browse/EZP-31620

This PR is the result of work done recently in docker-php, mostly ezsystems/docker-php#52.

I have:

Passing PostgreSQL build: https://travis-ci.org/github/ezsystems/ezplatform/builds/715567661

@mnocon mnocon force-pushed the switch-image-format branch 3 times, most recently from ce7ad95 to 615eada Compare August 5, 2020 19:07
@mnocon mnocon changed the title [WIP] [Docker] Switched image format [WIP] [Docker] Switched image format and added PostgreSQL service Aug 6, 2020
@@ -13,8 +13,7 @@ services:
nocopy: false

import-db:
# Using dev for now, because we need mysql client
image: ${PHP_IMAGE_DEV}
Copy link
Member Author

@mnocon mnocon Aug 6, 2020

Choose a reason for hiding this comment

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

dev image has been merged with the base image, so the mysql client is present in PHP_IMAGE

@mnocon mnocon force-pushed the switch-image-format branch from 24279aa to 6516991 Compare August 6, 2020 18:06
@mnocon mnocon changed the title [WIP] [Docker] Switched image format and added PostgreSQL service [Docker] Switched image format and added PostgreSQL service Aug 6, 2020
@ViniTou ViniTou self-requested a review August 7, 2020 07:27
@@ -10,8 +10,7 @@ DATABASE_PASSWORD=SetYourOwnPassword
DATABASE_NAME=ezp

## Docker images (name and version)
PHP_IMAGE=ezsystems/php:7.4-v1-node
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not sure if we should use 7.4 as default and not the lowest supported version.

Copy link
Contributor

Choose a reason for hiding this comment

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

For defaults we should pick what we recommend, needs for test matrix to make sure we do pass on lowest and highest supported version should be set using ENVs in CI system IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed it to use PHP7.3 by defult (recommended for Platform 2.5). Travis will run on PHP7.1 and 7.4 for testing.

doc/docker/base-prod.yml Outdated Show resolved Hide resolved
@andrerom
Copy link
Contributor

andrerom commented Aug 7, 2020

Besides inline comments db-stack.yml is using hardcoded mariadb version, unsure why.
Maybe @vidarl remembers as he added it as-is 3 years ago.

@vidarl
Copy link
Member

vidarl commented Aug 7, 2020

I think the reason for that was simply that I only tested docker stack on mariadb... But feel free to generalize it so it can be used with both, or rename it db-stack-mariadb.yml and make a db-stack-postgreql.yml counterpart if you think that is better

@mnocon
Copy link
Member Author

mnocon commented Aug 11, 2020

I've decided to keep db-stack as it is, as I haven't used Docker Stack yet and I'm not sure how to approach it (and renaming it to db-stack-mariadb.yml without creating a PostgreSQL alternative would just break existing scripts without any benefit for people).

This PR in its current state allows us to run tests on PostgreSQL on Travis, which was my initial goal - IMHO we can work on adding more PostgreSQL support in the future if needed.

@adamwojs adamwojs merged commit d58a11c into 2.5 Aug 12, 2020
@adamwojs adamwojs deleted the switch-image-format branch August 12, 2020 08:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

7 participants