[D7] GitLab CI integration for core

Perhaps worth postponing on #3387055: Configure GitLabCI matrix testing as matrix testing of different PHP versions and database drivers is also important to D7.

Not sure about the matrix testing and how exactly it will work in GitlabCI, but currently the testing in DrupalCI is set as follows:

PHP 5.6 & MySQL 5.5
PHP 7.2 & MySQL 5.6
PHP 7.4 & MySQL 5.7
PHP 7.4 & PostgreSQL 9.5
PHP 7.4 & SQLite 3.27
PHP 8.0 & MySQL 5.7
PHP 8.1 & MySQL 5.7
PHP 8.2 & MySQL 8
PHP 8.2 & pgsql-13.5

1. patches (or MRs) - triggers automatically PHP 8.1 + MySQL 5.7 and then we run manually all other combinations before pushed/merged, to verify it is working on all supported combinations
2. push - all above combinations are run automatically
3. there is no daily testing in 7.x branch

So I think this is a bit easier in comparission with D10, also given the fact that we have only phpcs (no other linting, etc). So if this could be set with the current config, I think we do not need to wait. But if the matrix testing will help here somehow, then OK.

But I think that we need to wait at least for the fix for test-only patches (#3386566: Add support for 'test only' changes to gitlab CI) + one additional blocker would be to add yml files to .htaccess, as D7 .htaccess is not blocking these currently (will create an issue for this).

Thanks @fjgarlin!

I have created a child issue to prevent access to YAML files, but this needs more discussion whether it is worth a change: #3387134: [D7] Prevent access to YAML files using .htaccess and web.config. So this is probably not a hard blocker (and it is possible that it will not be introduced).

Added some initial comments to the MR.

I am curious, if we should consider updating run-tests.sh as in D10:

+  if ((int) $args['ci-parallel-node-total'] > 1) {
+    $tests_per_job = ceil(count($test_list) / $args['ci-parallel-node-total']);
+    $test_list = array_slice($test_list, ($args['ci-parallel-node-index'] - 1) * $tests_per_job, $tests_per_job);
+  }

Can we benefit from it in D7 as well?

---------

If I checked the MR correctly, we are currently waiting for the test-only issue to get in to D10 (#3386566: Add support for 'test only' changes to gitlab CI), but other that that, it seems like the files does not contain anything special we need to wait for D10 issues? Is this correct?

Thanks!

Nice, the paralell runs looks good. Thanks for the great work here!

I was thinking, can we implement suggestions from these two issues/MRs as well?

#3387339: Integrate codequality reports into Gitlab - just for the PHPCS (if possible)
#3387959: Document new arguments in run-tests.sh - not sure about the wording, as it is not committed yet (we can keep an eye on the D10 issue), but just not to be forgotten

Other than that, I think this looks great and probably has all features/tweaks from D10 issue, which we need for the initial commit.

Just to summarize for reviewers:

The MR currently contains the base settings for D7 testing according to the D10 committed code (pipeline.yml, .gitlab-ci.yml, run-tests.sh), with these changes:

• tests combinations are according to the comment #4 (the current status in DrupalCI) -with the exception of PHP 7.2/MySQL 5.6, where the PHP 7.2/MySQL 5.7 is used instead (MySQL 5.6 was not working) and PHP 8.2/pgsql-13.5, where PHP 8.2/pgsql-14.1 is used instead (PostgreSQL 14 was not working in DrupalCI)
• .htaccess-parent part is needed for D7 to work correctly in the CI in subdirectory
• it includes this issue: #3387959: Document new arguments in run-tests.sh
• it includes this issue: #3386566: Add support for 'test only' changes to gitlab CI
• it includes this issue: #3387339: Integrate codequality reports into Gitlab
• SQLite testing is working unlike in the D10 setup
• default testing is against PHP 8.1 (as we have in DrupalCI currently)
• there is no linting in D7 currently, so the patch adds only PHP compatibility check via PHPCS and only on changed files (we cannot run Drupal standards check, as it was not possible to restrict PHP version to PHP 5.6)
• Drupal 7 is installed via drush si, so it needed to use different MySQL database name (not the system mysql database)

Other than that I think the code is pretty much the same as in the D10 (hopefully have not forgotten to mention anything).

We need to decide here: #3387134: [D7] Prevent access to YAML files using .htaccess and web.config, but then the idea is to get this in, so that we can start testing the GitlabCI integration.

Thanks!

Updated the IS with the summary.

@fjgarlin as these two issues are already committed in D10, do we need to update something in this MR?

#3386566: Add support for 'test only' changes to gitlab CI
#3387339: Integrate codequality reports into Gitlab

We need to go thru this with @mcdruid, so will update here if anything else is needed. Thanks!

Just getting up to speed with this issue - please forgive any silly questions :)

.setup-webroot: &setup-webserver
  before_script:
    - ln -s $CI_PROJECT_DIR /var/www/html/subdirectory
    - cp $CI_PROJECT_DIR/.gitlab-ci/.htaccess-parent /var/www/html/.htaccess
    - sudo service apache2 start

Does this mean we lose core's main .htaccess ?

If so, I'd think we want to append to core's file (or make replacements within it) rather than replace it?

No, we are not loosing the Drupal .htaccess, because Drupal is installed in a subdirectory /var/www/html/subdirectory and this special .htaccess is copied to the /var/www/html, so one level above it.

Ok, makes sense - thanks.

How about:

    # We need to pass this along directly even though it's set in the environment parameters.
    - sudo MINK_DRIVER_ARGS_WEBDRIVER="$MINK_DRIVER_ARGS_WEBDRIVER" -u www-data php ./scripts/run-tests.sh --color --concurrency "$CONCURRENCY" --url "$SIMPLETEST_BASE_URL" --verbose --fail-only ...snip...

Not sure we need the MINK related env variable(s) for D7? Can we remove them?

It seems like all tests are green even with MINK variables removed, see: https://git.drupalcode.org/project/drupal/-/pipelines/34972

So this indicate, that they were not used.

Nothing else jumping out at me.

IIUC committing these changes should not affect anything other than the gitlab ci set up (i.e. are unlikely to cause any regressions on running D7 sites - apart from if individual sites have set up their own gitlab-ci integrations and the new files here clash..?).

I'm happy for this to be committed.

Thanks for all the work that's gone into getting this all set up!

IIUC committing these changes should not affect anything other than the gitlab ci set up (i.e. are unlikely to cause any regressions on running D7 sites - apart from if individual sites have set up their own gitlab-ci integrations and the new files here clash..?).

Yes, I think this is correct. I do not think the small change in run-tests.sh (to introduce paralell runs) could be somehow disruptible and other from that there are only new files being added.

Thanks for confirming re. risk of regressions.

The latest change to configure retries looks good.

Happy for this to be committed - @poker10 let me know if you'd prefer that I did so, but I don't think you'd be "committing your own patch" here much.

Committed. Thanks everyone for working hard here to get this done!

as @mcdruid anticipated in #29 on Oct 23, 2023

apart from if individual sites have set up their own gitlab-ci integrations and the new files here clash..?)

the addition of Gitlab automation does clash with our use of Gitlab with Pantheon.