All variants run the same Drupal version when the pipeline is triggered via web

Checking the pipeline, the issue seems to be already from the composer jobs.

All of them seem to be bringing:

  - Installing drupal/core-dev (10.2.x-dev 71d714d)
  - Installing drupal/core-recommended (10.2.x-dev f84ec2d)

I created an MR for branch https://git.drupalcode.org/issue/gitlab_templates-3441816/-/tree/3441816... which can be tested. It shows some debug in the expand_composer file.

The change was made in #3437434: Composer (next major) no longer works after 11.x updated to Symfony 7 →
This is the commit

All the composer jobs in here https://git.drupalcode.org/project/gitlab_templates/-/pipelines/150167 are showing the RIGHT version of Drupal core. And the same for all the nightwatch and PHPUnit jobs, it shows the correct version.

Could it be that you have some variables defined in the CI/CD section for that project? like _TARGET_CORE

Well, that's good. It must be something wrong in my pipeline. I only run the current core automatically, but have the others as manual, to be triggered when required. Maybe that has something to do with the problem?

Could it be that you have some variables defined in the CI/CD section for that project? like _TARGET_CORE

No I don't set any TARGET_CORE versions anywhere.

Is it something to do with the wrong image/artifact being used in the phpunit jobs? I recall there had been an issue where they are cached but named the same, and subsequent jobs were not using the correct one.

The composer jobs were already showing always 10.2.x for all variants, so it's not the phpunit.

All the jobs on the new test, using MR190 are showing the correct Core version for the variants
https://git.drupalcode.org/project/scheduler/-/pipelines/150192

I can also see CI_PIPELINE_SOURCE=web in that job, meaning that it was triggered from the UI.
When we do this, a massive form with all variables is shown with the values prepopulated from the variables file.

This would have set _TARGET_CORE for all variants, as UI defined values have higher priority than file-based variables.

We should document this.

Changing title to document the issue. Also changing issue description with proposal.

We can also document that if you want to run via Web UI, delete the _TARGET_CORE variable, so that the variants then pick up the correct value from the job definitions in main.yml

I did exactly that 🙂

Please review https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/190... and feel free to change the language around it. I added it to a couple of places.

Thanks for changing the 'value' to 'variable'.

You have reverted the debug added to expand_composer, but I thought that could be useful info in the log and could have stayed in.

Maybe we can do that in another issue where we evaluate the output of each job / script and see if we should add improvements to it, to improve the overall experience. I'd say that it's not needed for this MR right now, but I'd be ok improving the overall experience of the logs on a separate issue.

I also had an idea to create a .pre checking job to warn about this. In psudeo-code

stage .pre
rules if CI_PIPELINE_SOURCE=web && (any of the opt_in variables = 1): always
  if not, never.
variables: 
  NEXT_MAJOR_CORE: !reference [composer next major, variables, TARGET_CORE]
  NEXT_MINOR_CORE: !reference [composer next minor, variables, TARGET_CORE]
(and repeat for two PREVIOUS variants)
script:
if NEXT_MAJOR_CORE == TARGET_CORE wite messaage and exit 1
repeat

Totally untested, but its hopeful that the version values are retrived from the actual jobs, thus allowing to detect when they have been overridden.
This job would not often get added, and when it is, if the target_core variable has been correctly deleted from the form, it would end green and can be ignored. But if, like I did first, the variants were all running the same version, this job was end with a warning, could even be red, to alert that something is wrong.

Worth trying at least.

It'd be great if !reference [composer next major, variables, TARGET_CORE] would give us the non-overriden value.
Feel free to give it a go at it.

Does the GitlabUI prompt for non global variables ?

If not is there a way around this by not using TARGET_CORE directly? Add a JOB_TARGET = TARGET_CORE by default with the next jobs overriding JOB_TARGET?

That’s an idea worth exploring @cmlara. Thanks for the suggestion!

cmlara, that's worth checking, so I just pushed this and will test it.

I think this problem of hiding the variables we do not want changed has already cropped up before. But I can't see the issue in the open list.

I have started testing this on Scheduler MR83. The composer job correctly has access to the new variable IS_THIS_IN_THE_FORM which is defined under the .non-global-vars ref
https://git.drupalcode.org/project/scheduler/-/pipelines/151802

However, when you click 'run pipeline' on a MR you do not get the UI form, the pipeline immediatlely runs. Maybe I need to go to the issue fork repo and run a pipeline from there.

Here's the pipeline I ran from the issue repo, not the MR
https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/151808

The form did not contain the IS_THIS_IN_THE_FORM variable, but it IS available in the Composer jobs
https://git.drupalcode.org/issue/scheduler-3356800/-/jobs/1381137#L294

So, we can use this method to hide the variables that we don't want in the form, in addition to re-working/resetting the core version values for the variables that are used in the composer variants.

That’s great! Just to clarify, we want _TARGET_CORE to show in the form, for the normal variant. But we should use a different variable for the variants with this approach of internal variables.

I’m on the phone today and tomorrow but happy to get into more detail on Monday. In any case, happy for you to try things out during the weekend and i can review on Monday.

I took a slightly different approach, and for the core version and php values for the variants we do not need them in the variables file, they can simply be defined right in the variant job that needs them. Then the new .create-environment-variables snippet can create and export updated values for _TARGET_CORE and _TARGET_PHP.

In my MR composer after_script the variables are printed for ease of checking. Here is the pipeline, using all variants, run from an MR
https://git.drupalcode.org/project/scheduler/-/pipelines/152009

Here is the same branch, triggered via 'run pipeline'. Just to make the point, I set _TARGET_CORE = 10.0 in the form.
https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/152020
Initially it all looked fine. All the composer jobs have the desired core version, including the 'current' composer running D10.0. But the phpunit jobs all also have core 10.0. This is because I was seeing what happened when I removed the override variables from the phpunit variants. I was wondering if the values exported and written to build.env would take precedence, but they don't. The form _TARGET_CORE is take precedence. Easy to fix, just put back the removed variables in the variant jobs. It may also need a call to .create-environment-variables in the .phpunit-base but I will add that in a following commit.

The phpunit jobs now have the correct core versions
https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/152043

phpunit (max php) only has php 8.1 but I think this is because I have restricted the core to 10.0 on the form. I will run again with a higher core to verify that.

Not 100% validated this, however considering your comment in #27 and basic GitLab knowledge i suspect this to be a cause:

$_TARGET_PHP is needed for image selection, it can't be set inside the job as that is too late (image is already running). We have to set it in the variables.

See https://git.drupalcode.org/project/gitlab_templates/-/blob/d38ad0bb65d83...

Thanks cmlara. Yes that makes sense. I re-submitted the form with _TARGET_CORE=10.2.2 and then the Composer Max PHP job did get PHP 8.3 https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/152119

So, this does work, but I'm not happy with it. Because _TARGET_CORE remains in the form, every variant job (not just the Composer variants) needs to call *create-environment-variables to get the correct override. Other newly-created variables are always automatically available in every subsequent job. But because this one is in the form that always takes precedence, so any custom before_script or after_script in a variant job will also have to call !reference [ .create-environment-variables ]. That could be documented, but its an overhead and is tedious, and likely to cause more support work.

Can we consider again the reason why we have to keep _TARGET_CORE in the UI form? Could we not have a differently-named variable in the form, and then that value would get used for the real _TARGET_CORE in the plain Composer job. The environment variable could then be created and all jobs would have the right valuess, automatically, with no more calls to .create-environment-variables required everytime the value is needed.

Any contrib pipelines that have values for _TARGET_CORE in their .gitlab-ci.yml will continue to work exactly as now. The variable is still being acted on, it's just not in the 'run pipelines' form. I think that would solve all the problems. We would need to check what happens with any scheduled pipeline with a saved value for _TARGET_CORE in the form. But that should not be a major problem to solve.

Great to see some progress in here.

I'm going to explore an idea where we wouldn't need adapting nor calling create-environment-variables. I think the code might be simpler, but I won't know until I try.

Thanks. I am also happy to try a new MR where we change the form variable name, and have _TARGET_CORE as hidden. But if you are doing your idea, I will wait to see how that goes first.

I think MR https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/193... shows what I had in mind.

• _TARGET_PHP and _TARGET_CORE remain unchanged, but they are used only for the current versions.
• Two new internal variables called TARGET_PHP_IMAGE and DRUPAL_CORE are introduced. These are the ones that will always determine which version of PHP and which version of Drupal to run things on.
• Then, for the current jobs, we set the two internal variables to the _TARGET_PHP and _TARGET_CORE values, and for the variants, we set the two new variables to what the variant should run.

This way, if people change _TARGET_CORE, it will change only the current variant, but the other variants will remain unchanged. If they would want to change the variants versions (advanced tweaking), they'd need to overwrite the value of CORE_MAJOR_DEVELOPMENT for example.

There were some variables related to the version of Drupal that were not used/needed on variants (as we were already bringing the right composer variant artifacts), so I removed those in this MR too.

Downstream pipelines seem to be running ok.

I think that this approach is much cleaner and less disruptive, but I'm open to any opinions or changes.

This looks just right. It it logically equivalent to where I was heading after #29, to separate out the form variables from the ones we actually use to control the jobs. Having new internal variables is even better.

There were some variables related to the version of Drupal that were not used/needed on variants (as we were already bringing the right composer variant artifacts)

Yes I had noticed that, and was going to ask you about whether they could be removed. So, yes I agree.

I will repeat the tests from before, to make sure, but this does look exactly right for solving the problem.

Run from MR - all variants have the correct core & php version
https://git.drupalcode.org/project/scheduler/-/pipelines/153386

Re-run via form, with _TARGET_CORE=10.0 and _TARGET_PHP=8.0
https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/153476
I know that the ordinary 'composer' job fails because 10.0 needs php 8.0, so that's OK.
But the 'composer next minor' and 'composer previous minor' also fail because they are being restricted to php 8.0. Is this the intended behaviour? I understood that the form variables would only influence the 'current' composer job, and the variants would always be run with the correct (internal) values.

Also, the 'PHPstan Max PHP' passed in the first pipeline (when run via MR), but failed in the second (run via form). The phpstan version is the same in both (1.10.67), as is the php version (8.3.6). So I would have thought that the result should have been the same? Maybe its because the 'composer php max' is using my override _TARGET_CORE which is 10.0. So even though the 'max php' version is set correctly the job still uses the override _TARGET_CORE. Maybe that is intended and expected?

But the 'composer next minor' and 'composer previous minor' also fail because they are being restricted to php 8.0. Is this the intended behaviour?

I'm inclined to say yes. If a variant is not requiring a particular PHP version (like the "PHP max" ones), then we should go with whatever is set.

--

Maybe its because the 'composer php max' is using my override _TARGET_CORE which is 10.0. So even though the 'max php' version is set correctly the job still uses the override _TARGET_CORE. Maybe that is intended and expected?

Let me review this, maybe I missed some case in the code, not sure.

That is the correct assumption. https://git.drupalcode.org/issue/scheduler-3356800/-/jobs/1397735 fails because for the "max PHP" variant, we use the core version that has been set, in this case 10.0.0.

Again, I think this is the intended behaviour as it is not a next or previous minor or major, it's just current + PHP version.

So I think the two cases presented in #35 and #36 are "working as designed". We only "force" a PHP or a Drupal version where needed, and otherwise we take the values set for the "current" version, which are what _TARGET_CORE and _TARGET_PHP do.

Thanks for that. Yes I'm ok with 'works as designed'. Of course, we make the jobs pass, but that would be restricting the input 'requested' values, and reducing the combinations and flexibility of testing. It was definitely worth investigating these cases though.

On the original MR there was an additional paragraph on the doc page. It's not all relevent now, but I will grab the bit that is and add it to this MR.

Also from that first MR we do still have the possibilty of using the method to hide the variables that we don't recommend being overridden in the form. That might work quite well, and we can leave MR190 open to look at that, but after MR193 has been merged.

I've added the one paragraph from the previous MR. I will test this on Scheduler 7.x too.

Thanks for adding that paragraph from the other MR. I also commented on what would be the only remaining part from MR190 not present at all in MR193. I think we already have a section like that in the current templates

Based on comments from #34 to #39 from @jonathan1055 and the additional MR review from @cmlara, is that a RTBC or do we want to do more testing or changes here?

7.x MR test https://git.drupalcode.org/project/scheduler/-/pipelines/154085
All looks OK (the phpunit tests fail at php8.2 but that's the fault of the way the tests are written)

I was going to re-run it from UI with _TARGET_CORE=7.97 but looking at your MR changes the _TARGET_CORE and DRUPAL_VERSION variables do feature in the d7-main changes.

Just realised I should have tried _TARGET_D7_CORE. Does your change cover that yet?

Correct, there are no version variants on D7, only PHP ones, which are covered. You are right, the variable to use there is _TARGET_D7_CORE.

The 7.x test with _TARGET_PHP=8.0 ran correctly. So I'd say this is is RTBC if you and @cmlara are also happy with it.

Great. Thanks for the additional testing and reviews. I've merged this now.

I'm not entirely sure if there was any part of MR190 that still needed discussion. Happy to continue the discussion in the MR.
I'm marking this issue as fixed for now at least.

I have merged 'main' into MR190 and removed the unwanted work I started. I have just left the new .hidden-global-variables to experiment with moving into here all the variables that we do not want showing in the form.

Perfect. Let us know the results of your tests.

I tried to use <<: !reference [.hidden-global-variables] in the top job-level variables: but the test gave an unhelpful error Undefined error (01HW5FKXP41Z1HSWQTDT32H1EY)
https://git.drupalcode.org/project/scheduler/-/pipelines/154333
Do you have any idea why the <<: !reference syntax gave the error above?

I have moved the definition into main.yml (temporarily, just to see if it works) and it appears to.
https://git.drupalcode.org/project/scheduler/-/pipelines/154343
But we obviously do not want to do that (apart from making the file unusable, these are needed in main-d7.yml)

Another idea might be to have the hidden variables in a separate file that is included into main.yml and main-d7.yml, instead of the !reference. Not tried that yet.

I haven't seen that error before and it's not very helpful, but I suspect it is because the <<: YAML syntax might not support references.

Then the extends syntax is supposed to only be used inside a job: https://docs.gitlab.com/ee/ci/yaml/#extends

I seem to recall that for the variables to be offered in the the UI form, they need to have value: and description: set, so maybe the possible solution for now might be to change something from:

  _CONFIG_DOCKERHUB_ROOT:
    value: "drupalci"
    description: "The root namespace on Dockerhub to pull containers from."

to

  _CONFIG_DOCKERHUB_ROOT: "drupalci"

--

There is #3440568: 2.x: Reduce number of variables in GitLab CI Templates → which talks about variables and what should be offered in a 2.x future improvement.

GitLab CI link to the previous workaround: https://docs.gitlab.com/ee/ci/pipelines/index.html#prefill-variables-in-...

Removing the separate value: and description: keywords worked perfectly. Those variables were not in the 'run pipeline' form but were defined correctly in the pipeline jobs. Here's the test which I ran via the form
https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/161406

Next step - I'm going to see if these variables can be moved into a separate file, so that we have a clear distinction between variables to be modified and variables that in most circumstances should not be modified. At least, they should not automatically appear in the form. The name can always be typed in the form as a new variable with a value. Also for highly customised pipelines the values can be set in the project .gitlab-ci.yml. But we are making the form more user-friendly by removing them.

That worked well too. I could override a variable in the hidden-variables file by adding that name into the form, but those variables are not shown by default. https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/161434

MR190 is ready for initial review and feedback. If you like this idea I will move all of the "do not override" variables into the new file.

This is working with the hidden-variables.yml. I had to make an adjustment in check-versions.php to cater for the name: value syntax all on one line.

I also made an improvment to the messages. Previously the $dev_releases were not being generated when it hit the early exist for "Drupal $tag is out!". But this gives misleading info that the development releases are invalid. Compare the current 'main' job
https://git.drupalcode.org/project/gitlab_templates/-/jobs/1483568
with the updated version in this MR which correctly only reports one error
https://git.drupalcode.org/issue/gitlab_templates-3441816/-/jobs/1486193

Pushed another improvement to the script to avoid fatal PHP runtime errors if the versions endpoint file cannot be read. Just needs to skip all the remainder of the processing if step 1 gives a non-zero exit_code. Attached is the log before and after, when the $xml read fails.

The progress here looks great, but I think it should be in a separate issue and we should leave this one marked as fixed.

The problem we had when this issue was opened is fixed now, and this new work on "not showing certain variables in the pipeline form" deserves a new issue in my opinion. We can just copy the changes from MR190 to there.

Very good point. Will do. The check-version.php enhancements should be separate too.

The check-versions improvements are now in #3444778: Improve the check-versions job output →

For moving the variables, is it OK if I re-open #3343522: Hide variables that should not be overridden (upstream request for GitLab) → ? That issue has some good background on it, and explains how we got this point. There was no MR on that issue, so it would be a clean start with the changes from MR190.

Sure thing! Let's reopen that one.

MR190 can be closed, now that all work is moved to MR197 on #3343522: Hide variables that should not be overridden →

jonathan1055 → changed the visibility of the branch 3441816-check-version-of-core to hidden.

fjgarlin → closed merge request !190

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