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.

Sign up for GitHub

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

ENH Add MySQL 8.4 to matrix #112

Merged
merged 1 commit into from
Nov 19, 2024
Merged

ENH Add MySQL 8.4 to matrix #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions consts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const DB_MYSQL_57 = 'mysql57';
const DB_MYSQL_57_PDO = 'mysql57pdo';
const DB_MYSQL_80 = 'mysql80';
const DB_MYSQL_84 = 'mysql84';
const DB_PGSQL = 'pgsql';
const DB_MARIADB = 'mariadb';

Expand Down
38 changes: 29 additions & 9 deletions job_creator.php
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain the implementation to me? Seems like we should just be either:

  1. always outputting all DB versions we need from generatePhpToDBMap and relying on deduping to avoid duplicates
  2. Passing the prefer-lowest db into generatePhpToDBMap so it knows not to duplicate it, and then still outputting all the ones we do still need

And then we don't need to split code into the new getDBs() nor do we need a foreach ($dbNotAdded as $db) {?

There's probably some context I'm missing though, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

generatePHPToDBMap() makes an assumption that there are greater or equal number of PHP versions to DB versions (minus 1, which is manually added for the --prefer-lowest build). However we now have a situation where there are more DB versions that PHP versions

Previously for CMS 6, which only supports PHP 8.3, we had mariadb manually added for --prefer-lowest, then a single PHP version (8.3), and a single DB (mysql80). However now we mysql84 as well, hence the $dbNotAdded code

Copy link
Member

@GuySartorelli GuySartorelli Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should this just wait until after PHP 8.4 support is added for CMS 6 then? It feels like a fair amount of complexity is being added to an already pretty messy file.

As an aside, we should probably refactor this to be a few separate classes at some stage so it's easier to see where things are happening for maintenance purposes. But that's clearly not in scope for this PR.

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 don't think it needs to wait. I think it's worth while code to add as we'll end up in the situation again where there's more DB's than PHP's whenever we release a new major.

Original file line number Diff line number Diff line change
Expand Up @@ -311,12 +311,15 @@ private function createPhpunitJobs(
]);
} else {
$cmsMajor = BranchLogic::getCmsMajor($this->repoData, $this->branch, $this->getComposerJsonContent()) ?: MetaData::LOWEST_SUPPORTED_CMS_MAJOR;
$dbsAdded = [];
$db = in_array($cmsMajor, ['4', '5']) ? DB_MYSQL_57 : DB_MARIADB;
$matrix['include'][] = $this->createJob(0, [
'composer_args' => '--prefer-lowest',
'db' => in_array($cmsMajor, ['4', '5']) ? DB_MYSQL_57 : DB_MARIADB,
'db' => $db,
'phpunit' => true,
'phpunit_suite' => $suite,
]);
$dbsAdded[] = $db;
if ($cmsMajor === '4') {
if (!$this->doRunPhpCoverage($run)) {
// this same mysql pdo test is also created for the phpcoverage job, so only add it here if
Expand All @@ -334,14 +337,25 @@ private function createPhpunitJobs(
'phpunit_suite' => $suite,
]);
} else {
// phpunit tests for cms 5 are run on php 8.1, 8.2 or 8.3 and mysql 8.0 or mariadb
// phpunit tests for cms 5 are run on php 8.1, 8.2 or 8.3 and mysql 8.0, 8.4 or mariadb
$phpToDB = $this->generatePhpToDBMap();
$lastPhp = null;
foreach ($phpToDB as $php => $db) {
$matrix['include'][] = $this->createJob($this->getIndexByPHPVersion($php), [
'db' => $db,
'phpunit' => true,
'phpunit_suite' => $suite,
]);
$dbsAdded[] = $db;
$lastPhp = $php;
}
$dbNotAdded = array_diff($this->getDBs(), $dbsAdded);
foreach ($dbNotAdded as $db) {
$matrix['include'][] = $this->createJob($this->getIndexByPHPVersion($lastPhp), [
'db' => $db,
'phpunit' => true,
'phpunit_suite' => $suite,
]);
}
}
}
Expand All @@ -365,6 +379,18 @@ private function getIndexByPHPVersion(string $version): int
return array_search($version, $this->getListOfPhpVersionsByBranchName()) ?? 0;
}

/**
* Return a list of DBS to test against for the current CMS major
*/
private function getDBs()
{
$cmsMajor = BranchLogic::getCmsMajor($this->repoData, $this->branch, $this->getComposerJsonContent()) ?: MetaData::LOWEST_SUPPORTED_CMS_MAJOR;
if ($cmsMajor === '5') {
return [DB_MARIADB, DB_MYSQL_80, DB_MYSQL_84];
}
return [DB_MYSQL_80, DB_MYSQL_84, DB_MARIADB];
}

/**
* Generate a map of php versions to db versions
* e.g. [ '8.1' => 'mariadb', '8.2' => 'mysql80' ]
Expand All @@ -373,12 +399,7 @@ private function generatePhpToDBMap(): array
{
$map = [];
$phpVersions = $this->getListOfPhpVersionsByBranchName();
$cmsMajor = BranchLogic::getCmsMajor($this->repoData, $this->branch, $this->getComposerJsonContent()) ?: MetaData::LOWEST_SUPPORTED_CMS_MAJOR;
if ($cmsMajor === '5') {
$dbs = [DB_MARIADB, DB_MYSQL_80];
} else {
$dbs = [DB_MYSQL_80, DB_MARIADB];
}
$dbs = $this->getDBs();
foreach ($phpVersions as $key => $phpVersion) {
if (count($phpVersions) < 3) {
$map[$phpVersion] = $dbs[$key];
Expand All @@ -387,7 +408,6 @@ private function generatePhpToDBMap(): array
$map[$phpVersion] = array_key_exists($key, $dbs) ? $dbs[$key - 1] : DB_MYSQL_80;
}
}

return $map;
}

Expand Down
Loading