_GITLAB_TEMPLATES_REF value in variables needs updating, and provide override variables for Curl

Pipeline: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/84721
MR: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/115

Ready for review.

Wouldn't a more appropriate parent be #3404175: Adopt (semver) versioning for gitlab_templates → ? The issue we just completed did not actually cause this, and did not miss anything, did it?

True. Changing.

The changes looked OK so I tested mr115 without any variable, the log shows _GITLAB_TEMPLATES_REF=default-ref. Fine.

But then I added an override in my template, as requested

variables:
  _GITLAB_TEMPLATES_REF: main

but the log still shows

_GITLAB_TEMPLATES_REF=default-ref
_GITLAB_TEMPLATES_REPO=project/gitlab_templates

Pipeline is https://git.drupalcode.org/project/scheduler/-/pipelines/84817
Maybe I've misunderstood what the expected change is. I know that nothing should appear to change, but I did expect the environment variable to be updated in the log.

I'd like to suggest that we take the bird-view on this one, since that variable _GITLAB_TEMPLATES_REF is used in a number of different contexts:

• in pipelines of this project itself
• in downstream pipelines of this project
• in project pipelines, that include these templates

For the first 2 use cases, the value should always be the equivalent of $CI_COMMIT_REF_NAME, if I'm reading it correctly.

For the 3rd use case, there are several ways to define the version that should be used:

• there is the global default variable defined by the DA in GitLab
• projects may override that variable in their project related CI settings
• the variable may be overridden in the gitlab-ci.yml file
• or, the include statement defines the reference, which doesn't change the variable at all, but loads a different branch

Therefore, we should change this such that the include statement from the projects defines, what it needs, but internal references within the templates should be static and not overridable by a project variable.

Not sure, how we can achieve that, but I thought it may help to break the problem space down into some smaller components.

Yeah, I'm not sure if we're having a mix of global variable vs inheritance order.

These are the places where we are using the value in the repo:

We are only setting that value in:
- The ".gitlab-ci.yml" to run the child pipelines
- The default template, so it knows where to find the code
- The variables file, which we are probably assuming will override the global value. <-- this is the one the MR is changing
- The rest of the usages are in the main and main-d7 files to download the assets that are needed.

Not sure what else needs changing then.

The effect described in #7 indicates some unexpected override, which we may have to research.

But there is another potential danger, that the include statement gets the ref defined with e.g "1.1.0" and no variable will be overridden. Then, gitlab includes that ref, but the internal links use the variable. That results in a mismatch.

Solution: if internal links were hard coded, it wouldn't matter how the templates get included.

Do you suggest hardcoding them to the “main” branch ones?

I'm uncertain about the right approach, or even if my thought has any value in the first place.

If so, each branch would have to have their own hard coded ref. In that case, the pipeline should create that ref and store it in the current branch? A bit like a packaging process.

I'd probably investigate first the inheritance order, as that might be the easiest and desired approach.

Your suggestion is doable too, especially since we added the scripts/do-git-tags.sh, because that's something that we could probably automate.
- Search and replace the value for the tag
- Commit the changes
- Do the tag
- Revert the value

However, this approach would mess up the git history, as we only have one branch, and a few fixed and moving tags ('default-ref', '1.x-latest', '1.1.x-latest'...). Not sure all of this would be worth it. If we limit to one tag only, then it'd be just one additional commit, but more than that seems like overdoing.

I'd say let's investigate the inheritance/overriding first since that's the ideal approach.

Agreed, let's investigate first.

But let's nor forget that inheritance only works if a variable is used. If the include statement refers to a ref without a variable, there is nothing to inherit.

I wonder if this code ever did anything: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

Will try some options tomorrow.

I wonder if this code ever did anything

Probably, because that is likely where my testing in #7 is getting the _GITLAB_TEMPLATES_REF=default-ref value. I'm going to check that by changing it to a different non-valid value, just to see what happens. This will also help with your inheritance/overriding investigation.

I wonder if this code ever did anything

It seems that the answer is No

Here's my commit
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/115...

Tested on pipeline https://git.drupalcode.org/project/scheduler/-/pipelines/84999
'Show environment variables' has the values

_GITLAB_TEMPLATES_REPO=project/gitlab_templates
_GITLAB_TEMPLATES_REF=default-ref

and these are also repeated at the end of the log

https://docs.gitlab.com/ee/ci/variables/#cicd-variable-precedence

I searched for 'default-ref' within the whole gitlab_templates repo, and only found these four. I do not know where 'default-ref' is being set, it must be outside this project, at a higher level. You will know more about that than me.

Yup, "default-ref" is the value (from this tag https://git.drupalcode.org/project/gitlab_templates/-/tags/default-ref) set globally for the _GITLAB_TEMPLATES_REF variable via the admin UI in our GitLab instance.

I've removed the block, which was probably never being applied, as per https://docs.gitlab.com/ee/ci/variables/#cicd-variable-precedence.
The downstream pipelines do work, which makes sense. MRs or projects pointing at this fork/branch should work too.

Let's test this case a bit more tho.

I think the only issue remaining is about these:

We know that it will take the "default-ref" unless we override that with any of the options shown in step 2 in the link above.

2. These variables all have the same (highest) precedence:
- Trigger variables.
- Scheduled pipeline variables.
- Manual pipeline run variables.
- Variables added when creating a pipeline with the API.

So maybe we just need to add some testing instructions for when any of those three files are being worked on?. These files rarely change. For the rest of the cases, I think the current instructions are good.

Thoughts?

Just seen you have made a new commit. I had written the following, before I saw that:

Looking at the usages of $_GITLAB_TEMPLATES_REPO and $_GITLAB_TEMPLATES_REF in the list in #9 it seems that the main (only?) problem we are dealing with is where the symlink/phpcs.xml/expand_composer files are downloaded from 'default-ref' when we trying to run a pipeline on something than default-ref. These files are the scaffolding of the pipeline, so if we needed to test changes in those files it could be done by running a pipeline using the UI input form to specify the MR repo and branch, rather than have these hardcoded for testing in the top of the .gitlab-ci.yml file (like we've been doing for most tests). These files dont change much, and so are unlikely to be part of a 'new feature' that users running from 'main' will want, before the next update of default-ref. So maybe the problem is not as big as it first appeared.
$_GITLAB_TEMPLATES_REPO and $_GITLAB_TEMPLATES_REF have to stay in .variables.yml so allow the above.

$_GITLAB_TEMPLATES_REPO and $_GITLAB_TEMPLATES_REF have to stay in .variables.yml so allow the above.

Otherwise they won't be offered? Then let's bring them back.

Yes we need those variables back, thanks for that. As agreed on slack I am using the gitlab_templates testing MR96 to try out some things without undoing and re-doing work on MR115. Below is what I've done so far. It's a bit wordy, but it explains the process I have been through. You may already have realised my conclusions, but its worth me setting it all out, just for info.

(1) Use MR96 in a contrib project template, with project: issue/gitlab_templates-3408456and ref: 3408456-test-theory in the .gitlab-ci.yml
https://git.drupalcode.org/project/scheduler/-/pipelines/85459
The changes in MR96 are seen in the composer step, but the curl get files from project/gitlab_templates and default-ref as we discovered above.
The log has

_GITLAB_TEMPLATES_REPO=project/gitlab_templates
_GITLAB_TEMPLATES_REF=default-ref

(2) To get round this, and have the curl get the files from MR96, I tried running a pipeline via 'run pipeline' button on the contrib project MR 'pipelines' tab. But there is no form here, so do not get the opportunity to enter variables
https://git.drupalcode.org/project/scheduler/-/pipelines/85469
Log has the same as before
_GITLAB_TEMPLATES_REF=default-ref
_GITLAB_TEMPLATES_REPO=project/gitlab_templates
The changes in MR96 are seen, but again the files are curl'ed from default-ref

(3) To use the UI form to enter variables, I needed to trigger the pipeline from the contrib project "all pipelines" main page, not the MR page. The variable _GITLAB_TEMPLATES_REPO is in the form, so I changed this to issue/gitlab_templates-3408456. But there was no form entry field for _GITLAB_TEMPLATES_REF. I tried to start the pipeline, but it failed to get created, with error Pipeline cannot be run. Project `issue/gitlab_templates-3408456` reference `main` does not exist!
This is not unexpected, becauase that particular MR does not have a 'main' branch. I wanted to use the '3408456-test-theory' branch anyway.

(4) Try again with the form, _GITLAB_TEMPLATES_REPO set to issue/gitlab_templates-3408456, and I created a new row in the form table to specify _GITLAB_TEMPLATES_REF with value 3408456-test-theory. The pipeline still failed to get builts. I realised then that I had committed "ref: main" in the scheduler codebase. So having done this (as per our discussion yesterday about wanting to use 'main' so that I see any problems early) I discovered that there was no way I could use the pipeline UI form to trigger a pipeline to test a gitlab_templates MR. The 'main' was hardcoded in my project's .gitlab-ci.yml file.

(5) I decided to revert my commit of yesterday and put back ref: $_GITLAB_TEMPLATES_REF in the template, so that entering a value for this in the UI form would actually allow the pipeline to run. That variable when it had been missing before, with default value 1.0.x as expected. I thought that all variables contained in .variables.yml were availbale in the UI form, but obviously there is some filterig out [that's a separate question. I expect there is an easy answer].

Now finally the pipeline ran.
https://git.drupalcode.org/project/scheduler/-/pipelines/85486
The composer job shows Executing curl -OL https://git.drupalcode.org/issue/gitlab_templates-3408456/-/raw/3408456-test-theory/scripts/expand_composer_json.php and so we do get the changed version of expand_composer_json.php from MR96. In that script it shows In expand_composer_json.php _GITLAB_TEMPLATES_REF = 3408456-test-theory_GITLAB_TEMPLATES_REPO = issue/gitlab_templates-3408456

Does this have an impact on our advice to set ref: main or ref: 1.0.x etc in the contrib template? Is there another way, where we leave it at ref: $_GITLAB_TEMPLATES_REF and indicate via another variable which tag to use? That might not work, as it is probably going to hit the same problem, but worth thinking about ?

Thanks for the above testing, it really helped understanding the actual value per scenario.

Here is my suggestion.

I suggest that we do an approach similar to .simpletest-db, where we calculate a value, and then use that calculated value instead. All the references that would need changing happen in the .composer-base job and the phpcs one.

Maybe we introduce a new variable, empty by default, named GITLAB_TEMPLATE_REF_OVERRIDE, and then if set, use that instead.

So we'd do something like this (totally untested):

.calculate-gitlab-ref: &calculate-gitlab-ref
- [[ $GITLAB_TEMPLATE_REF_OVERRIDE == "" ]] && export GITLAB_TEMPLATE_REF_OVERRIDE=$_GITLAB_TEMPLATES_REF

..

.composer-base:
   ...
   script:
      - *show-environment-variables
      - *calculate-gitlab-ref
      - ...
      - curl -OL https://git.drupalcode.org/$_GITLAB_TEMPLATES_REPO/-/raw/$GITLAB_TEMPLATE_REF_OVERRIDE/scripts/symlink_project.php
      - ...

By default, as it is empty, it will take the _GITLAB_TEMPLATES_REF value, making it equivalent to what we have now (and BC for that matter), and only when overriden, we use the new value. This also allows for the overrides via the pipelines UI on manual runs, schedules, etc.

We can then go ahead and document this. Folks knowing enough to change the "ref" to a non-default one should be ok doing this small change. Also note that this override is only needed if you need changes from the referenced files (symlink, expand_composer_json and phpcs.xml.dist).

I'm going to implement the changes in the MR in case we want to test them.

The above suggestion is in code ready to be tested.

I like the idea of the override variable, that sounds as though it will work, and did some testing on the 8.x-1.x branch of Scheduler which has 'ref: main' in the committed code.

1. First I used the normal UI method of setting values in the form:
   _GITLAB_TEMPLATES_REPO = issue/gitlab_templates-3417987
   _GITLAB_TEMPLATES_REF left unchanged at 1.0.x
   Added new variable GITLAB_TEMPLATE_REF_OVERRIDE = 3417987-fix-ref-variable
   https://git.drupalcode.org/project/scheduler/-/pipelines/86045
   This did not actually use the MR115 changed code, because the committed branch has 'ref: main' in the template. So I think it ran with the 'main' branch from the issue fork issue/gitlab_templates-3417987
2. Run from UI with the original two variables:
   _GITLAB_TEMPLATES_REPO = issue/gitlab_templates-3417987
   _GITLAB_TEMPLATES_REF = 3417987-fix-ref-variable
   but without adding the new variable GITLAB_TEMPLATE_REF_OVERRIDE
   https://git.drupalcode.org/project/scheduler/-/pipelines/86093
   This likewise did not use MR115. Did not give the echo "Executing curl ..." This is still running with 'includes/include.drupalci.main.yml' from 'main' branch because that is in the committed codebase. So it is a bit of Catch22. I can't test the override variable via UI when the template has 'ref: main' in an attempt to override 'ref: main' precisely because the template has 'ref: main' in it. But if the MR115 was committed then it would probably work. It is because I am trying to test the new override feature, which actually the override feature before that testing will work!
3. I then tried to test it via a scheduler test MR with template containing the normal way of testing
   

     - project: issue/gitlab_templates-3417987
       ref: 3417987-fix-ref-variable
   

   and also adding

   variables:
     GITLAB_TEMPLATE_REF_OVERRIDE: 3417987-fix-ref-variable
   

   https://git.drupalcode.org/project/scheduler/-/pipelines/86073
   This run the new changes in MR115, and we see the echo "executing curl -OL https://git.drupalcode.org/project/gitlab_templates/-/raw/3417987-fix-re...". But it failed because in this scenario we also need a similar override for GITLAB_TEMPLATE_REPO - because curl was trying to get the file from repo 'gitlab_templates' but branch '3417987-fix-ref-variable' which does not exist.

I will add a similar override for REPO and see what that gives us. The names needs some attention, the original has GITLAB_TEMPLATES (plural) but the override has GITLAB_TEMPLATE_ (singular) so I will fix that too.

To avoid confusion, I'd say we should call them CURL_TEMPLATES_REPO and CURL_TEMPLATES_REF.
Overriding these values is needed only if you really need to test changes from the curl'ed files.

As for the testing in #28.

For 1 and 2, it makes sense to fail that way.

- project: ... / ref: ... dictates the templates code, so if we don't use the default variables, we cannot override it, because it's hardcoded in the code.
- CURL_TEMPLATES_REPO / CURL_TEMPLATES_REF dictates the version of the curled files only.

OK. Tested via MR using

- project: issue/gitlab_templates-3417987
    ref: 3417987-fix-ref-variable

and adding variables

variables:
  CURL_TEMPLATES_REPO: issue/gitlab_templates-3417987
  CURL_TEMPLATES_REF: 3417987-fix-ref-variable

and it works
https://git.drupalcode.org/project/scheduler/-/pipelines/86143

I removed the comment about "If you change the default value, you should set ... to be the same as set here" as that seemed confusing and unnecessary. It would only really be needed for gitlab_templates MRs and testing the changes of those files. It would only really have any effect on actual Contib live testing if a new feature on 'main' relied on a change in those files. I think that is unlikely. But I can put the comment back if you think it is necessary. The descriptions of the two new variables might also need adjustment.

We could reduce the names still further 'CURL_TEMPLATES_REPO' does that mean anything? It could be just 'CURL_REPO' ?

It would only really be needed for gitlab_templates MRs and testing the changes of those files

Not really, if somebody decides to use a fixed tag in their ref value, like 1.0.1, they'll get the default-ref curled files. But they might want to get the 1.0.1 version of the files too, and they'd need those variables set.

And since we're explaining the options in the template where they will need to do the changes, we should probably add those couple of lines too.

I like the short name, but I also like seeing the direct connection with the gitlab related ones, so CURL_TEMPLATES_REPO feels like the sweet spot.

OK, no problem. Yes there is a chance that the curl'ed files don't match, so we should keep them all in line, for safety. I will add back the comment and the example commented out variables.

Comments done, ready for review.
When this is committed I will be able to verify that testing via #28.1 and 28.2 will work.

Some small feedback about wording on the comments.

OK. I also need to remove the debug.

Tested also on 7.x, all good.
https://git.drupalcode.org/project/scheduler/-/jobs/731783

I will stop now, so won't push any more updates until you have had a chance to read all the description changes. I think I have addressed all your review notes.

I think I've gone through all the text and code changes and they all look good to me.
Given all the testing that we've done, I'm marking this RTBC from my end, but will wait until @jurgenhaas has had a look and weighs in too.

The only change that will be needed for maintainers wanting to "fully fix" the templates to a fixed version would be to add CURL_TEMPLATES_REF to the variables section, matching the same that they've set in the ref: line of the template.

The rest of the uses are MR-specific and are documented too.

Thanks for picking up the initial work and improving it @jonathan1055.

The issue title I changed in #34 was not an improvment, so restored the orginal title, and added better override detail.

Update here. I'd like to merge this tomorrow and tag it as 1.1.1 (as we're technically fixing a bug). I'd also like to make 1.1.1 the default tag that everybody gets in contrib, meaning that the rule refactoring we did a few weeks ago will go to everyone.

That's good, yes fine with me.

the rule refactoring we did a few weeks ago

#3414391-110: Define extra rule to jobs that have "needs: composer" →
It was actually only 8 days ago, but yes it would be good to move default-ref on. I have been using 'main' since then. If you commit at the beginning of your/our working day then we'll be here to support any problems when folks in the US start using it.

Based on https://www.drupal.org/project/gitlab_templates/issues/3343522#comment-1... → , can we quickly rename the variables.

From
CURL_TEMPLATES_REPO / CURL_TEMPLATES_REF

To
_CURL_TEMPLATES_REPO / _CURL_TEMPLATES_REF

This issue prompted that comment :-)
Done.

Thanks.

Ok. I'm merging this now and tagging as 1.1.1 (https://git.drupalcode.org/project/gitlab_templates/-/tags/1.1.1)

I will change the default tag on Monday to point to this one. This way we have today, tomorrow, and the weekend to test the latest (if we are using "main"), and then we can release it to everybody on Monday.

That's a good plan. I saw you had tagged 1.1.1 but not moved default-ref yet.
https://git.drupalcode.org/project/gitlab_templates/-/tags

This is a good system of release management. I don't know if/how it would work for a regular contrib module, but for the testing infrastrucure like this, where we want to roll out fixes fairly quickly and seemlessly, it is perfect.

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