[D7] GitLab CI integration for core

Perhaps worth postponing on 📌 Configure GitLabCI matrix testing Active 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 ( 📌 Add support for 'test only' changes to gitlab CI Needs review ) + 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).

Happy to wait. I will leave everything ready and follow up on that issue, then apply here.

We were typing at the same time :-)

1. Would be achieved with easy tweaking from this proposal.
2. Can be easily achieved too.
3. No action needed.

So I will continue working based on the actions needed for 1 and 2 and will watch the "test-only" issue closely.

What's in the MR already is almost the equivalent of what landed in D10+ today.

I will work tomorrow on the changes suggested in #4.

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: 📌 [D7] Prevent access to YAML files using .htaccess and web.config Active . So this is probably not a hard blocker (and it is possible that it will not be introduced).

Update:
- The matrix and the rules for it mentioned in #4 are all in place now.
- I also took the coding standards task out of each matrix combination as this only needs to be run once.

The only remaining task is the "test-only" job, which I am going to address next.

Taking test-only approach from 📌 Add support for 'test only' changes to gitlab CI Needs review . WIP.

Successful test-only run: https://git.drupalcode.org/issue/drupal-3387052/-/jobs/90810

This is now ready to review. All feedback from #4 was addressed.

MR: https://git.drupalcode.org/project/drupal/-/merge_requests/4765

- The pipeline with the whole matrix can be seen here: https://git.drupalcode.org/issue/drupal-3387052/-/pipelines/20433
- The test-only job can be seen here: https://git.drupalcode.org/issue/drupal-3387052/-/jobs/90810 (see also related Drupal 10+ issue 📌 Add support for 'test only' changes to gitlab CI Needs review )
- The code already has many of the follow-ups that were created for D10+ in it.

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 ( 📌 Add support for 'test only' changes to gitlab CI Needs review ), 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!

I'll see about the options for "run-tests.sh". Good suggestion.

We have no dependency on the other issue as I implemented the suggestion from it here. They might divert a bit but the core functionality is and will be the same.

I'll work on your feedback above today. Thanks!

All feedback from the above comments and MR has been addressed.
This is ready for review again.

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?

✨ Integrate codequality reports into Gitlab RTBC - just for the PHPCS (if possible)
📌 Document new arguments in run-tests.sh RTBC - 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.

Good suggestions, both of them are implemented now. I took the text from the D10 issue.
See a forced phpcs error code quality report here: https://git.drupalcode.org/issue/drupal-3387052/-/pipelines/21428/codequ...

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: 📌 Document new arguments in run-tests.sh RTBC
• it includes this issue: 📌 Add support for 'test only' changes to gitlab CI Needs review
• it includes this issue: ✨ Integrate codequality reports into Gitlab RTBC
• 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: 📌 [D7] Prevent access to YAML files using .htaccess and web.config Active , 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.

Do we need to tag some specific people so they can RTBC + feedback/commit this MR?
Core D10+ is already turning off some DrupalCI activity in favor of GitlabCI and it'd be great if we can start testing things here too.

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

📌 Add support for 'test only' changes to gitlab CI Needs review
✨ Integrate codequality reports into Gitlab RTBC

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

📌 Add support for 'test only' changes to gitlab CI Needs review is in place already.
✨ Integrate codequality reports into Gitlab RTBC is in place already too.

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?

Based on DrupalCI. It's used here: https://git.drupalcode.org/project/drupalci_testbot/-/blob/dev/src/Drupa...

And the method is not overriden in https://git.drupalcode.org/project/drupalci_testbot/-/blob/dev/src/Drupa....

That doesn't mean we shouldn't remove it, but that's why I decide it to leave it. We can try without it and see if everything still works tho.

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

So this indicated, 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!

Added something really small but really useful that was added here 📌 GitLab should retry jobs that fail outside test failures. Fixed . Sometimes the gitlab runners have some hiccups that might make the job fail but a simple retry fixes it, so this small addition does that automatically.

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.

poker10 → credited andypost → .

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

Automatically closed - issue fixed for 2 weeks with no activity.

as @mcdruid anticipated 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.