🇬🇧United Kingdom @jonathan1055

Account created on 16 November 2006, over 18 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom jonathan1055

Rebased and fixed conflicts. I presume we do still want to work on this?
📌 Switch to using run-tests.sh by default Active is also being looked at again.

🇬🇧United Kingdom jonathan1055

As this is "closed won't fix" could @fjgarlin or @sime close the merge request? I was about to re-base and re-test this as I came to it form the MR listing page not the issue.

🇬🇧United Kingdom jonathan1055

Just for the record, the build.env file is read automatically and the variables with spaces and without quotes are loaded correctly in all script sections of the subsequent jobs that follow a composer job. The only place where the variables created in composer: script: are automatically available is in a custom composer: after_script: and this is where source build.env used to work, but now fails due to having a variable with spaces in. But the same can be achieved using the slightly more complicated version IFS=$'\n' && export $(<build.env) && unset IFS

An example of this is https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5132987#L1122

🇬🇧United Kingdom jonathan1055

The PHPstan next minor for 8.x-1.x is green

Fixed.

🇬🇧United Kingdom jonathan1055

I've tidied up the try-out code, renamed the script as requested, and removed the quotes. All runs ok in the latest downstream pipeline (the previous failure was before I rebased, and phpunit failed due to the quotes around project type, that was fixed yestterday)

Here's a test where cspell after_script calls !reference [ .show-ci-variables ] and it works cleanly
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/486624

This is ready for review.

🇬🇧United Kingdom jonathan1055

Just for good measure I have also change phpunit (previous minor) to run concurrent=1 and now we see a proper red failure, when running against the default-ref
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

🇬🇧United Kingdom jonathan1055

The change in this MR is definitely the right thing to do.

Regarding the passing phpunit tests, I think this problem only affects jobs running concurrent=1 (and hence using run-tests.sh) which is what we have in Max PHP version. The parameter there is --directory 'module's/custom/gitlab_templates_downstream which causes the error. With concurrent=0 (which is what we have in d10-plus 'current') the arguments is just the path the project i.e. /builds/project/gitlab_templates_downstream/web/'module's/custom/gitlab_templates_downstream and the phpunit binary can cope with this. Quotes are valid characters in the directory path, although I'm sure most people avoid them :-)

🇬🇧United Kingdom jonathan1055

I have just pushed this change in GTD d10-plus, so if we run that downstream pipeline it should pass when using this MR

🇬🇧United Kingdom jonathan1055

Yes, thanks for reporting this. When I added that chnage I never considered the project type to be in quotes, but as you say, it is valid.
In your original pipeline we can see PROJECT_NAME=group, PROJECT_TYPE='module' with the quotes.

Can you give a link to the pipeline where you are testing with this MR? We can also test with our downstream projects, which might actually be useful, so that we cover more edge-cases like this.

🇬🇧United Kingdom jonathan1055

MR350 tested here with concurrent=1 to use phpunit binary
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/484682

and with concurrent=0 to use run-tests.sh
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/484688

Both passed, but of course one run does not prove it, so will re-run a couple of time.

🇬🇧United Kingdom jonathan1055

Downstream pipelines for D10 and D7 show the Gitlab Templates version as required.

Here's a test, where the reference is using in cspell after_script

cspell:
  after_script:
    - !reference [ .show-ci-variables ]
    - echo "=== GITLAB_TEMPLATES_VERSION=$GITLAB_TEMPLATES_VERSION"
    - echo "=== VERSION_USING_PUTENV=$VERSION_USING_PUTENV"
    - echo "=== GITLAB_TEMPLATES_VERSION2=$GITLAB_TEMPLATES_VERSION2"
    - echo "=== VERSION_USING_PUTENV2=$VERSION_USING_PUTENV2"
    - source build.env
    - echo "GITLAB_TEMPLATES_VERSION=$GITLAB_TEMPLATES_VERSION"
    - echo "GITLAB_TEMPLATES_VERSION2=$GITLAB_TEMPLATES_VERSION2"

The value is displayed, but annoyingly it also shows the quotes, as they are written to build.env. But when an extra source build.env is added the variables do not have the quotes. This does not really matter, as the default display in Composer is clean. It is just unusual that the values read from build.env a second time appear to behave better than the initial read.
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5085925

🇬🇧United Kingdom jonathan1055

Thanks for linking the history of this. For completeness 📌 Improve concurrency: replace experimental package with core script Fixed is where we switched from paratest to run-tests.sh

🇬🇧United Kingdom jonathan1055

Updated title to show that this is where paratest was first added

🇬🇧United Kingdom jonathan1055

Here are two "before" examples of the problem. First running .show-ci-variables in a composer after_script fails, because we are trying to curl the file without the _CURL_TEMPLATES_REPO and REF being set.

Second, adding .show-ci-variables in another job after_script works, but is confusing and wasteful because we are downloading and executing the script again

🇬🇧United Kingdom jonathan1055

No, sorry to not be clear. The solution will be done here, but to replicate the problem we need a new issue in GTD.

🇬🇧United Kingdom jonathan1055

The ideas in #6 are definitely worth trying out. The new _RUN_TESTS_EXTRA variable would be good in any case, so that different options can be set for concurrent and non-concurrent, making it easier to switch (I already have some custom code to change the matrix value if runing concurrent=1, and having a specific "extra" variable for run-tests.sh would simplify that.

The problem will be in the logic about making decisions on what to give a warning about if phpunit_extra is not empty. We may also need to override and set allow_failure:true if we think that we've changed something that could cause the pipeline to fail.

Overall it is worth investigating, and setting out the detailed scenarios to test. I will rebase the MR, as it is 180 commits behind and has a conflict.

🇬🇧United Kingdom jonathan1055

Thanks for merging. I think I will do this in the same issue, so I have created a new branch. May not start the work just yet.

🇬🇧United Kingdom jonathan1055

@snehal-chibde Thanks, but the reason it works is that Scheduler provides the solid background specifically to solve this problem. See my comments in #2 and #4 above. Here is the css override that the project uses. If you edit your scheduler .css and remove those lines you will see that the background colour disappears.

🇬🇧United Kingdom jonathan1055

Thanks. I'd be happy to merge this now, then look at the second-level changes in a new MR. I know what is needed, but not sure the complication in the script is worth it. But it may be, so needs investigation. I have pushed one final commit, to address one @todo renaming a variable and removing one unused one. If the downstream pipelines all pass then I'm happy for this to be merged now, as it does touch a lot of code, therefore doing it while other MRs are quiet makes sense.

🇬🇧United Kingdom jonathan1055

Now that we have Gitlab Templates Downstream we could use our test theme to replicate this scenario. @liam morland if you would still like to progress this you can raise an issue in GTD where we can isolate and test this problem.

🇬🇧United Kingdom jonathan1055

Thank you nicxvan, this looks excellent, and exactly what I was proposing ;-) This resolves my comments in #12 & #19

🇬🇧United Kingdom jonathan1055

I have fixed the build and validate stages and pushed the changes. When moving the interruptible keyword I realised that we set the tdefault to true in the default: section, so there is no need to set this to true again in the jobs.

I have also now just fixed the remainder of the jobs - it wss a really quick and easy task, given the line number addition, and the clear output. Many of the fixes only involved moving one item.

I'm ... not surprised at having that many as we've been creating new jobs without thinking of the order

I added a count of the jobs checked and it is 97, so with only 27 jobs to fix it meant we had more than two thirds with no changes :-)

Attached is a screenshot of run-local-checks when all is clean.

🇬🇧United Kingdom jonathan1055

Regarding the actual order of keywords, the order used by Tin Plavec is good, and I would like to highlight two points:

  1. He has stage, needs, rules and when we use this order we get a total of 41 jobs with at least one keyword in the wrong place. But we have mostly used stage, rules, needs. If we switch to this order we only get 27 jobs with an incorrect order. So I propose we use this, as it more closely follows our practice so far, and is still a good logical grouping of those keywords.
  2. A major change in the new order is having allow_failure and artifacts after the script, whereas we have generally (but not always) had these before the scripts. There is a good logical reasoning for having them after, which came from Tin's blog, and which I have replicated in the comments in the code here. I will push some of these changes, then we can take a view on whether we like the look of it
🇬🇧United Kingdom jonathan1055

I have added the line-number feature as requested, and also made some improvements to streamline the output, as discussed. This is ready for review, and I have not fixed the two main files yet, so that you can see the output.

But I will now fix them, so that we can see the changes.

🇬🇧United Kingdom jonathan1055

I have pushed changes to fix our .gitlab-ci.yml for keyword order. Also I have added indentation in the messages, and it looks much more readable.
https://git.drupalcode.org/issue/gitlab_templates-3518793/-/jobs/5034970

The changes to run-local-checks.sh may seem unrelated, but I wanted a way to run it with both the debug and the fix arguments at the same time. So to allow any way round, and also cover 'fix' and '--fix' I added pattern-matching on the word against $1 and $2

🇬🇧United Kingdom jonathan1055

Yes I've just checked on core 11.1-dev and it is still a problem - see new screenshot.
I have not seen it in a core drop-down, but as I said before, given that the equivalent styling in other themes has a solid background for the non-active menu items it should really be done for Claro too. The fix shown in #2 (without the initial .scheduler-admin-form identifier) could be added to the appropriate core .css file.

🇬🇧United Kingdom jonathan1055

Thanks for the investigation about eslint. I've just tested, and we can "unignore" our .gitlab-ci.yml, so I will open a separate issue for that.

Good comments on the MR. I had already done some more changes, so will push them now, then make the improvements you suggest.

Regarding the actual keyword order, I have not found any other scripts which are doing this work, and also not found any other discussions or best practice other than the link in #2. Tim's reasoning seams good to me, and in many cases we are already following it, so I'm going to add comments about the groups of keywords, then I could start fixing to see how much of a change it will be. After review, we can easily change the order to refine our decisions.

🇬🇧United Kingdom jonathan1055

There is a 'test-only changes' job which needs to be run and it should fail.

🇬🇧United Kingdom jonathan1055

A sniff and fixer for this has been written and it is part of PHP-CS-FIXER but I don't think this is currently loaded and available in Coder. What is the policy regarding installing new requirements vs writing our own sniffs?

🇬🇧United Kingdom jonathan1055

I've made the change to implement option (a) as I think this is your (and my) preferred way.

First test when then project does not have a 'web' directory - all runs as usual
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/473947

Now adding a rename in composer: before_script to create a pre-existing 'web' directory. The jobs fail as required.
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/474175
The composer 'max php version' is allowed to fail, but the PHPstan 'max php' then checks the composer exit code and does not run (which is right)

Finally, using the same rename to have a 'web' directory, but specify
variables: _WEB_ROOT: web-alternate
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/474194
Also we can see the new verbose output of mkdir showing that these directories have been created
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/4975069#L376

This is ready for review.

🇬🇧United Kingdom jonathan1055

Initial commit, with a php script to check the keyword order. It failed as intended, because I have not moved any of the keywords, and also left some undefined, to demonstrate the output.

Tangentially related, I noticed that the project's .gitlab-ci.yml is being ignored by the eslint check. We do not have a .eslintignore or .prettierignore file, and even using --no-ignore locally this file is still not tested. I adde --debug to check what happens in the pipeline, and it is ignored just like when running locally, and I don't know why.

2025-04-12T16:55:27.106Z eslintrc:ignore-pattern Check {
  filePath: '/builds/issue/gitlab_templates-3518793/.gitlab-ci.yml',
  dot: false,
  relativePath: '.gitlab-ci.yml',
  result: true
}
2025-04-12T16:55:27.106Z eslint:file-enumerator Yield: .gitlab-ci.yml but ignored

See this part of the log

🇬🇧United Kingdom jonathan1055

Hi Tunic,
Thanks for raising this. I can see that Image Styles Generator was created in June 2023 - do you know if you had tests running on DrupalCI before the switch over to Gitlab Pipelines? I recall that DrupalCI had exactly this behaviour and I discussed it with one of the core committers responsible for the testing system. I will try to find that issue.

The solution was that any "require": dependencies in the submodule should be repeated as "require-dev": dependencies in the main module. This does not impact the ability to install the main module when those dependencies are not available, but they will be loaded during the test set-up, which is exactly what you want.

🇬🇧United Kingdom jonathan1055

The above order is what we have mostly been following, but I thought it worth taking a look to see what other developers do, and/or if there is consensus

Tim Plavec on Medium suggests an order that is fairly close to what we are already are doing:
https://tinplavec.medium.com/gitlab-ci-cd-best-practices-i-recommend-aft...

extends
stage
needs
rules
tags    << we do not use tags 
image
environment << we do not use 
variables
script
allow_failure
artifacts
cache  << we do not use 

He does not have 'services' in his list. I have not found any other recommended orderings.

His idea of having artifacts at the end sounds quite good. They are logically produced by the script, at the end of the job.

It would also be quite easy to write a simple script to verify the order in our two files, but Gitlab Templates own .gitllab-ci.yml, and run this as part of our code checks. We won't be adding too many new items very often, but the script would help us to get the order right, and keep it so.

🇬🇧United Kingdom jonathan1055

Wow, that was one of the quickest MR commits. I was about to upload this screenshot to demonstrate what I meant about the 'find all'. So here it is, just for the record. Thanks for the merge.

🇬🇧United Kingdom jonathan1055

The D7 downstream pipeline yesterday failed for phpcs and phpunit, but the problem seems to be in the Composer jobs (but not anything changed in this MR). We get an odd error when trying the symlink

$ php symlink_project.php "$CI_PROJECT_NAME" "$DRUPAL_PROJECTS_PATH"
Retry later

See https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/49...
I note that we do not have a $COMPOSER_END_CODE check in D7 like we do for D10, so I think this needs to be added. It would highlight this error in the Composer job, and not give misleading info about phpcs or phpunit failures. But that is for another issue, as it will need separate testing.

🇬🇧United Kingdom jonathan1055

Thanks. I have pushed changes for items 4 and 5 and this is ready for review and feedback.

Just to note (I'm sure you knew this already) that the syntax needs: [composer (next minor)] if perfectly valid without the quotes even when there are multiple words. So I did not add quotes, just to keep it cleaner, but if you don't like the look of it I can add them. ESlint/Prettier demands no spaces between [ and the first character, and if using quotes they have to be singles not doubles.

🇬🇧United Kingdom jonathan1055

The first one was actually already done (I had not updated my notes). So there are only five proposed changes.

🇬🇧United Kingdom jonathan1055

These changes have the potential to conflict with existing MRs but apart from the documentation MR there are currently only 3 other open and active MRs (where active = worked on in the last month) so now is a good time to do this. Those other earlier MRs will already be out of date and probably have conflicts.

I will make the changes in sequence, and keep them in separate commits for easier reviewing.

🇬🇧United Kingdom jonathan1055

Thanks. It's been a long time coming, both this issue and it's sibling 📌 Update to new driver when D11 "current" uses Nightwatch 3 Postponed
I'm pleased we found a good solution for the new INSTALLED_DRUPAL_VERSION variable.

🇬🇧United Kingdom jonathan1055

I've removed the test values for the downstream jobs. No more tests, so this is still RTBC if you are ok with the Pages doc changes.

🇬🇧United Kingdom jonathan1055

Thanks for running KeyCDN, the --verbose works as intended.

I've just updated the Pages doc page, probably after you marked this RTBC. Here's the updated one. I know that the changes to the documentation are more than just adding this extra variable. I have been making notes on each documentation page, and when a change is needed I've been bringing them into alignment with a standard style. I will put more details in the documentation issue summary.

I am guessing that we do not want to have these three _EXTRA variables set permanently for the downstream pipeline definitions, so I will remove them.

🇬🇧United Kingdom jonathan1055

For reference, in addition to the Experience Builder project mentioned above, Project Browser has also implemented this rather nicely. Although it looks like the code has also been shared between them already. We could borrow/learn from both.

🇬🇧United Kingdom jonathan1055

I tried the folllowing, to add the EXTRA variable just for one variant

'→ GTD D10 Plus':
  extends: .downstream-base
  trigger:
    strategy: depend
    project: project/gitlab_templates_downstream
    branch: d10-plus
  'phpstan (max PHP version)':
    variables:
      _PHPSTAN_EXTRA: --debug
  'nightwatch (max PHP version)':
    variables:
      _NIGHTWATCH_EXTRA: --verbose

But the pipeline produced the following error

jobs:→ gtd d10 plus config contains unknown keys: phpstan (max php version), nightwatch (max php version)

Oh well, it was a guess but I had a hunch it would not work. So it means setting the variable for all variants (unless you know of a better syntax?)

🇬🇧United Kingdom jonathan1055

Remember that we can pass variables to the downstream pipelines

Yes absoluetly, I'd not forgotten that. But I had forgotten that KeyDCN has a pages job, so thank's for the tip.

Regarding the backticks, I was thinking we would just delete them if not serving any rendering process. But if you still want quotes of some sort around those words then let's leave them as backticks, as they are clean an unobtrusive, and they do imply a 'code' word even if not rendered. Double-quotes would have a different kind of meaning.

🇬🇧United Kingdom jonathan1055

First commit pushed. We could use the downstream pipelines to test PHPStan and Nightwatch, but we do not have a Pages job yet - will be done in 📌 Add documentation .md files to test the pages job Active

I did notice that the variables.yml file has lots of words in the descriptions in backticks implying "code" formatting. These do not get rendered as anything special in the Run Pipeline UI form, see attached. Is there anywhere else that this file is displayed/rendered where those backticks are actually useful? Or maybe they signify something even without any different rendering.

🇬🇧United Kingdom jonathan1055

Good that you like the last changes, I was very happy when I discovered how to make it both better, clearer, and more efficient all at the same time :-)

Here's a Scheduler test overriding the 'current' job with the legacy values
'show environment variables' has all 3 variables as expected, and the old driver values are used.

I'll remove all the temporary test code now.

🇬🇧United Kingdom jonathan1055

I have made a futher change, that is both more efficient and also gets us better information. I noticed that .show-context was getting the real \Drupal::VERSION via vendor/autoload.php and this snippet was being called from several jobs. So instead, I have replaced the composer show with that php -r autoload.php and store the variable that way. Then when *show-context is called we simply write the value out, instead or running the php command. The value of DRUPAL_CORE is only displayed when it differs from the actual installed version. We can see the benefit of this in this next minor job where we do now get 11.2 showing.

🇬🇧United Kingdom jonathan1055

The swap was made when you were first adding the quotes around each character, and I did not notice that when I made the first suggestion to revert. Some editing programs make auto-corrections when typing one quote it inserts two, so it can be easy to make these types of unintended change :-)

Now the MR affects 319 files not 320 which confirms the revert. Still RTBC.

🇬🇧United Kingdom jonathan1055

I don't know if this helps, but attached is a comparison of the logs of the different failure on re-running. This is one particular test, and the first run shows "element not found" but the second says "stale element reference".

🇬🇧United Kingdom jonathan1055

Yes, this is still RTBC but the revert was not quite right, the quotes were swapped round, which could cause other unnecessary conflicts. I have made the suggestion in the MR to fix this.

🇬🇧United Kingdom jonathan1055

I have made the change to create the new variable MINK_DRIVER_ARGS_WEBDRIVER_DEFAULT and I think this does simplify the readability and understandability of the change.

Tested in the downstream pipelines and all OK. Testing with more complex tests (eg Scheduler) is still producing variable and unpredictable results. I ran a job and it failed, then I re-ran it in the same pipeline with no other changes and the failures were different. These tests have passed with the new webdriver before, and I expect they will again. Could you trigger the other downstream pipelines please? I will also runa test with a customized override to make sure that is still catered for.

🇬🇧United Kingdom jonathan1055

jonathan1055 changed the visibility of the branch mr257-3475581-use-w3c-driver-in-current to hidden.

🇬🇧United Kingdom jonathan1055

Regarding Drupal 7 I have answered my own question, and we do not need to do it. There are only two variants, both using the same D7 core version. Technically it would be possible to add other variants to test earlier D7 releases but that is pointless. So there is no need for the new variable.

This is ready for review. Could you trigger the other downstream pipelines? I've made reminders in the MR to remove the extra test lines.

🇬🇧United Kingdom jonathan1055

Reading the comments in #13 and #18-23 it seems like this issue is not going to be progressed? if so, can we close it?

🇬🇧United Kingdom jonathan1055

I rebased, and triggered all of our new GTD downstream pipelines. All passed except "PHPUnit (max php version)" in d9-basic and d10-plus

🇬🇧United Kingdom jonathan1055

This was "closed (duplicate)" in November 2023, so MR35 can be closed too. Thanks.

🇬🇧United Kingdom jonathan1055

This is "closed (works as designed)" so MR48 can be closed.

🇬🇧United Kingdom jonathan1055

This issue has been "closed (works as designed)" so @berdir would you be able to close MR54 that you opened, please? I'm tidying up the Merge Request queue and trying to close the ones that are done, so we can more easily see the active issues.

🇬🇧United Kingdom jonathan1055

Would you like me to get this mergeable again, and so we can continue work on it?

🇬🇧United Kingdom jonathan1055

@taran2l Did you have any more to add regarding #20 and the not having access to the logs when CI_DEBUG_SERVICES is on? Is it only maintainers of the project that can read the log? We could add a line about this in our documentation page about this.

Also MR107 can be closed.

🇬🇧United Kingdom jonathan1055

I'm trying to tidy up the MR queue so we don't waste time rebasing things that have been discarded.

There are two MRs here that were merged (259 and 293) and two that are still open but very out-of-date (258 and 261).

This issue was fixed on 25 November, so I presume there is nothing in those MRs that we still need? So can the non-merged ones be closed?

🇬🇧United Kingdom jonathan1055

jonathan1055 changed the visibility of the branch 3475974-pass-variables to active.

🇬🇧United Kingdom jonathan1055

Setting to NR as this change could be committed, and just to let you know, I'm not planning on working on the actual phpstan fixes. They could be done here or in a follow-up issue.

🇬🇧United Kingdom jonathan1055

Migmag MR13 now correctly runs PHPStan jobs.

There is still some investigation required as to why your original phpstan-baseline.neon caused the failure.

🇬🇧United Kingdom jonathan1055

That's good. The phpstan jobs now run as expected and report actual warnings.

🇬🇧United Kingdom jonathan1055

I have replicated your error by using your exact phpstan-baseline.neon file in our Gitlab Templates Downstream testing project. Here is the failing pipeline

My understanding is that the baseline file usually has just errors to ignore, not other config parameters. These other config parameters usually go in the project's phpstan.neon config file. This may only be a convention of practice, not a rule, but I tried changing the phpstan-baseline.neon file so that it did not contain the excludePaths and the job then passed perfectly! Here is the passing pipeline

I have opened a new MR on 📌 Fix testing on most recent Drupal core versions Active to test this in your project.

🇬🇧United Kingdom jonathan1055

I'd like to open a new MR here to test (and potentially fix) the phpstan problem.

🇬🇧United Kingdom jonathan1055

That third call to phpstan uses

--allow-empty-baseline

which should allow the baseline file to not exist. At least it did when we tested those changes back then.

In your committed 1.8.x repo you just have the default phpstan-baseline.neon file, so I was wondering where the custom-phpstan-baseline.neon name came from? Did you set this via pipeline UI variable?

Looking back through your pipeline history, PHPstan passed '3 months ago' here
https://git.drupalcode.org/project/migmag/-/jobs/3697585

Just trying to understand the background of the problem.

🇬🇧United Kingdom jonathan1055

Hi huzooka,
Thanks for reporting this. You say since 21 December, so it is likely that 📌 Tweak PHPStan config so paths are always correct and baseline is more usable Active has something to do with this, as those changes where made default in version 1.6.11 released on 18 December. Just for verification, would you be able to run a test pipeline using version 1.6.10? You can do this via the pipeline UI form. You need to enter the value for _CURL_TEMPLATES_REF as 1.6.10. This variable is right at the end of the form.

Can you give a link to this pipeline, then I can check how the processing differs from the later run. Thanks.

🇬🇧United Kingdom jonathan1055

Good, let's keep it all here. I have renamed the variable to INSTALLED_DRUPAL_VERSION as suggested in the MR.

I note that there are some temporary changes to undo in the downstream jobs definition.

Would it be a good idea to also create this new variable in the D7 pipeline? It won't be quite the same becauase Drupal is not installed until the phpunit jobs. There is no specific need for it yet in D7, this issue has no other changes for D7, so maybe not worth the effort.

🇬🇧United Kingdom jonathan1055

Was it this commit?
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/341...
This is only changing the echo of the statement, as it was missing the --xml argument that was already in the executed command.

🇬🇧United Kingdom jonathan1055

we are also changing the path of the generated files in here, not just creating empty ones.

That was not the intention, and I may be wrong but don't think we are changing the paths. What specific bit make you think this?

🇬🇧United Kingdom jonathan1055

As this change now creates a new environment variable DRUPAL_CORE_VERSION which is generic, is created in the composer job and can be used anywhere, do you think that this work should really be done in a separate issue, with a separate entry in the changelog.md? It would be done first, then return here when merged, just for the phpunit driver changes. I'm OK to leave as-is and do all in the same MR because it is not a very complicated change, (and it also reduces work for both of us) but just thought I would check with you.

🇬🇧United Kingdom jonathan1055

Applied you suggestion and responded on the MR.

Downstream d10 current with concurrent=0. The log is clean with no warnings and we see that phpunit-ignore.xml is created

Downstream d10 max PHP with concurrent=0. The log is clean with no warnings and we see that junit.xml is created

This is ready for review.

🇬🇧United Kingdom jonathan1055

Good work. Just for the record, I can see you have released both 1.8.1 and 1.9.0 today, with 1.9.0 as new default. Was this in case there are problems with the new images then people can revert to 1.8.1 but still get those earlier improvements?

🇬🇧United Kingdom jonathan1055

Thank you. That's a great find :-)

🇬🇧United Kingdom jonathan1055

@murz I am interested to know how you discovered the environment variable FORCE_COLOR=1

🇬🇧United Kingdom jonathan1055

Thanks to all here and on Slack for working to solve this.

I have also tested this MR on Scheduler here, and everything works - including loading PHP7.4 for Drupal 8.

RTBC+1 and when merged hopefully we can make a new release soon.

🇬🇧United Kingdom jonathan1055

As we have the last follow-up issue I think we can set this issue to fixed.

We can contiue to use MR8 as a general testing merge request, to try out things or use a specific Gitlab Templates MR. It does not matter that this issue will eventually become closed.

🇬🇧United Kingdom jonathan1055

Thanks. I have triggered our downstream testing projects, and the Nightwatch current and next minor (running Nightwatch 3.9) are now in color.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
It also does not affect the older versions. Does this need to be added to any documentation page and/or echo the variable to the log? Not sure it is necessary but just asking.

🇬🇧United Kingdom jonathan1055

Thanks, merged.

I see now it is being discussed in this slack thread about docker.

Given than our Drupal 7 template only uses run-tests.sh there is nothing to port to main-d7.yml. And the d10-profile and d10-theme branches do not run any phpunit tests. So this is all done now.

🇬🇧United Kingdom jonathan1055

MR16 is ready for review
https://git.drupalcode.org/project/gitlab_templates_downstream/-/merge_r...

To test the change I triggered a pipeline from UI with incorrect mink webdriver values, causing the javascript tests to be skipped. The jobs fail. Note - Max PHP ends with a warning, because the tests are skipped. It is running concurrent=0 for every variant because the UI form variable takes precedence over the job-level variables
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Another from UI, this time with concurrent=1 and also bad mink driver args - all jobs end green because run-tests.sh does not cater for the 'fail-on-skipped' concept
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Maybe if run-tests.sh has a long future, which I presume it does, we could ask for that enhancement? It should be quite easy, just pass the argument through to phpunit. Or is there a way we can do that from this end? It's a feature which is quite important for control of pipeline success/failure.

Production build 0.71.5 2024