add support for database versions

[skodak]

Hi @stronk7 ,

I have decided to use docker for all my development and testing work on my M2 mb air, I did a few tweaks to add support for all MySQL, MariaDB and PostgreSQL database versions. I was wondering if you guys were interested in merging it.

Please note I did not test this on Windows because I do not have any computer capable of running it, sorry.

Ciao,
Petr

[skodak]

I have included this in #229, it goes one step further by eliminating the PHP specific db overrides used for mssql with PHP 5.6 which can be there solved with moodle-docker.yml overrides.

[stronk7]

Hi, nice one! Only a couple of comments?

1. Maybe we can add SQL*Server and Oracle to the equation too? We have the very same already implemented in sister moodle-ci-runner and can be useful to test incoming versions.
2. Surely this is my main point... could we try to keep only one place where the defaults are defined? Having 2 places (linux and windows scripts) we can forget about it.

About point 2 I was thinking something like (for example, for MySQL, same for all DBs):

1. We create a MOODLE_DOCKER_DB_VERSION_DEFAULT env variable in db.mysql.yml
2. In the very same file we do:

image: "mysql:${MOODLE_DOCKER_DB_VERSION:-${MOODLE_DOCKER_DB_VERSION_DEFAULT}}"

Disclaimer, not tested, may be it doesn't work. All I've been able to find is this, telling that it should work:

https://github.com/compose-spec/compose-spec/blob/master/spec.md#interpolation

But it's not in the docker docs:

https://docs.docker.com/compose/environment-variables/#substitute-environment-variables-in-compose-files

That way each DB would have its default in their corresponding .yml file and done.

Alternative to that is to have another, new, db.defaults.yml and create there one env var for every database (MOODLE_DOCKER_DB_VERSION_MYSQL, ....) and then apply to it also via expansion (that I don't know if works, as said above).

Or maybe via .env file, don't know. Just aiming to have the defaults defined once. What do you think?

Ciao :-)

Note: if point 2 becomes too complex... I'm happy merging this as is. We can always investigate later and have the defaults defined twice in the mean time. Just was guessing if, maybe, it was not hard to achieve it now.

[skodak]

I have updated the patch, in it I have removed the DB tweaks with PHP versions, I do not think anybody needs mssql driver from PHP 5.6 these days. I have also split the db.pgsql.yml from base.yml - it was useful when I rewrote the waiting for database instance in my other patch.

[stronk7]

I have also split the db.pgsql.yml

Will look to this soon, but +1 to that idea, I've thought about it N times, basically every time I review anything, better having it separated, yep.

[stronk7]

Much better, yay!

1. So the shell-like expansion is working, great! I was reading some minutes ago about compose versions and it seems that our version "2" means "2.0" and I did read somewhere that the expansion was working only in versions "2.1" and up. Anyway, will try later, but for sure the GHA tests have passed applying the defaults, so... great!
2. Would you please, add support for oracle (21) and mssql (2017-latest) ?
3. Why do we need the db.mysql.8.0.yml file? Without any difference in configuration or similar... it's not really needed, isn't it? Or is it just an "example" ?

Ciao :-)

[skodak]

MySQL 8.0 is not compatible with your 5.7 stuff, they deprecated and removed some features and it fails now.

I was playing a bit with docker version and I ended up using "2.4" because "2" was complaining about "platform", my understanding is that Docker Compose V2 pretty much ignores the version...

As far as mssql and oracle go I am not keen to work with those because Roseta emulation on M2 does not work properly with these docker instances - they crap out immediately.

You can see the results of my experiments in olms branch at https://github.com/skodak/moodle-docker/ - I suppose you might be interested in my new code that waits for databases inside the containers https://github.com/skodak/moodle-docker/blob/olms/assets/db/pgsql_wait.sh.

[stronk7]

Hi,

using my previous numbers...

1. I was worried about the version and then have read in various issues that, some time ago docker adhered (in fact is the reference implementation) to https://compose-spec.io . And the very fist line of the spec says: "The Compose file is a YAML file defining version (DEPRECATED), ..."
   So, surely, since some time ago (recent versions) that version doesn't matter anymore.

2. Well, it's just about to modify ONE line in the mssql and oracle files. Don't worry about testing, GHA will do for you.

3. Ah, I did not see / remember the 3 innodb settings. Curiously, I'm pretty sure that we can remove the 3, because they became default with MySQL 5.7, and I don't think that there is any interest into supporting 5.6 (we always can add a db.mysql.5.6.yml file if needed, but I'd not do it here). So maybe we can remove the 3 settings and unify 5.7 and 8.0 ?

And, I'm happy considering any PR, of course. How not!

Ciao :-)

[stronk7]

BTW, just came to my brain looking to recent changes to other CI tools... I think that mysql_native_password needs to be set as default auth plugin, because in MySQL 8 they changed it to another (securer) one that is not supported by all PHP versions. So maybe that setting needs to be added to the unique (supporting 5.7 and up) db.mysql.yml file.

[skodak]

I just initialised phpunit with mysql 8.0 image, so the password thing cannot be a problem

I'll have a look at the oracle and mssql now

[scara]

Hello Everyone,
strictly speaking using the argument option --default-authentication-plugin=mysql_native_password will give us more backward compat, I mean w/ (very) old PHP versions but given that we normalized PHP versions to the latest ones through moodlehq/moodle-php-apache:<M>.<m>, now it is IMHO safe to remove that configuration (maybe some comments in the code out there if someone would like to hack things for running a quite old PHP version):

When running a PHP version before 7.1.16, or PHP 7.2 before 7.2.4, set MySQL 8 Server's default password plugin to mysql_native_password or else you will see errors similar to The server requested authentication method unknown to the client [caching_sha2_password] even when caching_sha2_password is not used.

Ref.: https://www.php.net/manual/en/mysqli.requirements.php

HTH,
Matteo

[skodak]

I've added the mysql_native_password just to be sure, it seems to be working the same for me

[stronk7]

now it is IMHO safe to remove that configuration

Yeah, nice you found that page, telling which php versions do support caching_sha2_password. I was looking for it and was unable to find the information.

For some reason I thought that only PHP 7.4 supported it, but it's clear that, starting with 7.1.x, all our images support it. That means Moodle 3.2 and up. So I agree that we don't need it anymore, all our PHP versions support the new auth plugin.

Sorry for the ping-pong, @skodak !

[skodak]

should I revert it then? @stronk7 ?

[stronk7]

Ok, and, hopefully latest round...

1. Can we, please, make the mssql default to be 2017-latest. I've some issues about trying 2019 here and surely it will become latest, so better if we have it fixed to the 2017 one.
2. Can we try to unify the 2 mysql files, as commented above, the 3 "innodb" settings aren't needed in MySQL 5.7 either (defaults already have them enabled).

With that... and GHA passing plus some quick windows testing locally... I'm happy to merge this, it's really a nice addition, much requested / needed, yay!

Ciao :-)

[stronk7]

should I revert it then? @stronk7 ?

Ah, sorry, didn't see this. Yeah, I think we don't need to force the old auth, coz all our PHP images support the new).

Ciao :-)

[skodak]

I was wondering was wondering if it could be merged, but then I decide I do not want to be wasting time testing all combinations, up to you then...

also you should be switching to a new collation utf8mb4_0900_as_cs to make it behat more like PostgreSQL

[stronk7]

Yeah, I'm using here utf8mb4_es_0900_as_cs in some of my dev instances since recently, they seem to work fine. Finally they came with sensitive collations apart from bin, that's good.

Ok, let's keep the 2 files separated, we can cleanup those 5.7 settings later, not in a hurry.

Going to perform some local tests (mainly Windows) and, with GHA passing, this is ready, thanks!

[stronk7]

Grrr, I'm getting (again) some syntax errors with Windows... trying to trace them down... BTW have noticed that, in the .cmd the PHP override has not been deleted.

Ciao :-)

[stronk7]

Uhm.. it's something from yesterday's MOODLE_DOCKER_DB_PORT... looking...

[stronk7]

Wow, just changing this, that was introduced by #226 , from:

diff --git a/bin/moodle-docker-compose.cmd b/bin/moodle-docker-compose.cmd
index 3966ac4..848f8b6 100644
--- a/bin/moodle-docker-compose.cmd
+++ b/bin/moodle-docker-compose.cmd
@@ -46,7 +46,7 @@ IF "%MOODLE_DOCKER_DB_PORT%"=="" (
             SET MOODLE_DOCKER_DB_PORT=127.0.0.1:%MOODLE_DOCKER_DB_PORT%
         )
         SET filedbport=%BASEDIR%\db.%MOODLE_DOCKER_DB%.port.yml
-        IF EXIST %filedbport% (
+        IF EXIST "%filedbport%" (
             SET DOCKERCOMPOSE=%DOCKERCOMPOSE% -f "%filedbport%"
         )
     )

I get the script working. Certainly I cannot understand it, specifically because I tested it and also, because we have other IF EXIST cases in the script and they aren't using the quotes.

Anyway, I'm not going to spend more time, preparing an extra commit with the needed changes. Will share once I have it running ok locally.

[stronk7]

I've taken the liberty of adding one extra commit to you branch, @skodak , with various CMD fixes:

07bb7cc

@scara , some of them are related with pieces of work / fixes that you were involved with, if you can take a look to them, great!

With that commit applied here, I've tested both the DB port (#226) and the DB version (this) and everything is, apparently, working ok:

C:\Users\stronk7\git_moodle\moodle-docker>bin\moodle-docker-compose.cmd up -d
...

C:\Users\stronk7\git_moodle\moodle-docker>bin\moodle-docker-compose exec db psql --version
psql (PostgreSQL) 12.12 (Debian 12.12-1.pgdg110+1)

C:\Users\stronk7\git_moodle\moodle-docker>SET MOODLE_DOCKER_DB_VERSION=14

C:\Users\stronk7\git_moodle\moodle-docker>bin\moodle-docker-compose.cmd down
...

C:\Users\stronk7\git_moodle\moodle-docker>bin\moodle-docker-compose.cmd up -d
...

C:\Users\stronk7\git_moodle\moodle-docker>bin\moodle-docker-compose exec db psql --version
psql (PostgreSQL) 14.5 (Debian 14.5-1.pgdg110+1)

[skodak]

thanks @stronk7 , your patch looks good

[scara]

Hi @stronk7,
LGTM but for image values not always set with double quoting. It doesn't hurt but for a policy point of view 😉.
From https://docs.docker.com/compose/compose-file/#image I think it should be fine to remove the double quoting where added or already present since from a YAML perspective it will be always a string - different story for ports.
Any thought here @skodak ?

HTH,
Matteo

[stronk7]

pong?

[skodak]

I am no yaml expert, it works for me, if anybody wants to improve it then please submit patches

[stronk7]

Do I assume that means that you aren't going to remove the four (x2) quotes from the mariadb, mysql, mysql8 and postgres ymls?

No problem with that, I'm more that happy adding a extra commit applying the changes and explaining your rationale, so you introduced them, but don't remove them, because you aren't a yaml expert.

Seriously? Using your own words... grrr! 😆

Ciao :-)

[skodak]

I do not really get what is the problem, before my patch there already was a mix of quoted and unquoted image values. I introduced them by accident when I was copy/pasting during refactoring of my other branch.

[stronk7]

Thanks @skodak, will process this soon!