Integration without "ref" failed on templates version calculation.

fjgarlin → credited dpi → .

Slack conversation + credits to the people who helped in the thread so far.

fjgarlin → credited cmlara → .

Example with and without -f option in curl:

With (no file is downloaded):

curl -fOL https://git.drupalcode.org/project/gitlab_templates/-/raw/default-ref/scripts/get-gitlab-templates-version.php
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (56) The requested URL returned error: 404

Without (a file with that name and the 404 HTML is downloaded):

curl -OL https://git.drupalcode.org/project/gitlab_templates/-/raw/default-ref/scripts/get-gitlab-templates-version.php 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2206    0  2206    0     0   6999      0 --:--:-- --:--:-- --:--:--  7003

It is good that we are tackling this now, as one of the things on my "need to raise an issue to improve this" is to detect when a curl operation fails (for any reason) and give a helpful informative message. Several times in the past, on other issues and support requests, a failed curl was not noticed and the developer was trying to decipher a starge error message much later in a subsequent job.

Maybe we should not limit this to just the case when ref: is missing but try to check the outcome of all curl operations? Can we check that curl succeeded to get the file. Maybe read the file, check it it not empty?
I made this note, but have not tested it:

    - CURL_RESPONSE=$(curl -I  https://git.drupalcode.org/$_CURL_TEMPLATES_REPO/-/raw/$_CURL_TEMPLATES_REF/scripts/symlink_project.php)
    - echo "CURL_RESPONSE=$CURL_RESPONSE" | grep 'HTTP/2 200' || echo "curl failed" && exit 1

I also saw in Project Browser

# Download the file from the Drupal repository
curl -o drupal_core_phpcs.xml.dist https://git.drupalcode.org/project/drupal/-/raw/11.x/core/phpcs.xml.dist?ref_type=heads
# Check if the download was successful
if [ $? -ne 0 ]; then
  echo "🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥"
  echo "Error: Failed to download $DRUPAL_URL"
  exit 1
fi

We can also use your idea of checking for a mismatch between the _CURL variables and/or between the variables are the branch being used.

Actually the -f option you linked to should make detection of the failure code easy. Maybe we can write a reusable code-snippet in the tempate to do a curl operation using a variable as the input file name and chekc the outcome, give message etc. Then we call this for all the curl operations?

Happy to do the curl fail detection for all curl calls. It needs to be with -f as it will otherwise create a file which won't be empty, so it can be misleading. Also, this way we only need one call, and not two.

Good idea on a possible reusable snippet, if that's possible.

I have pushed some changes to show proof of concept
Using GTD general testing MR8 with no customisations - composer is clean
gitlab_templates_downstream/-/pipelines/489340

Then with a temporary change to MR253 renaming get-gitlab-templates-version.php so that the curl fails, this is now detected and we can exit the job with a useful message.
gitlab_templates_downstream/-/pipelines/489373

I presume if a curl does fail we want to exit the job immediately? I think that is probably best, so it is clearer what has gone wrong. Some of the later processes in the job could probably succeed and would mask the actual problem.

Ready for feedback. If you are happy with this approach then I will look a making a reuable *execute-curl code snippet to use in all places.

If left some feedback in the MR. I think that in most cases, we might want to exit with an error, but in one of the curls, where it's just extra information, we could just display a warning and continue.

Thanks for the feedback in MR. I have listed the nine curl statements that we have in main.yml. There are four in main-d7.yml.

Many of the curl statements are in conditional logic, which makes having a re-usable code snippet impossible for this task. At least I have never seen a reference used inside script logic, I don't think that can be done. I can think of two options to proceed (you may have more ideas)

1. We replicate the return code checking in each of the nine curl statements. This is "simple" but also tedious, and it duplicates lines many times. Making an improvment would mean 9 x the changes
2. We have a php script that can be called in place of the curl which can be inside a conditional block.

I favour option 2, not only because it will reduce the size of the template file and make maintenance easier, but it will also allow easy parameter input, making it more flexible, for example whether to exit or give a warning, the target folder for where to write the file, etc

Agree, using a reference inside a script might not be an option at all.

I see both 1 and 2 as good options to be honest, so I'll let you go with whatever you find more comfortable.

1. 9x changes is not that bad given the size of the templates already, and the logic is simple enough. The logic here is simple and repetitive.
2. We could do the -f logic for that first one, and then use the PHP call in the others. The logic here is a but more complex but reusable.

Yes I like that combination. The first curl (to get the 'curl' php script) is done at the start of the composer step, long-hand as in the POC alreday done. This php file will not be removed, and can then be used for all the subsequent curl requirements, of which some will be in conditional logic. It will be stored at the top-level $CI_PROJECT_DIR but won't be symlinked so should not interfer with anything.

I switched from a php script to a bash .sh as this replicates the existing curl statements more closely. Also addressed the feedback in the MR.

For test coverage, in main.yml for D10+ there are nine curl statements. Some of them are conditional on the project not having its own version of the file, as noted:

3 in composer
- scripts/get-gitlab-templates-version.php
- scripts/expand_composer_json.php
- scripts/symlink_project.php
All of these are executed in every Composer job

1 in phpcs
- assets/phpcs.xml.dist
Only executed when the project does not have its own phpcs.xml, so this is tested by GTD branches.

1 in phpstan
- assets/phpstan.neon
Only executed when the project does not have its own phpstan.neon, so this is tested by GTD branches.

2 in cspell
- scripts/prepare-cspell.php (always executed)
- assets/.cspell.json - only when the project has no .cspell.json, so is tested by GTD branches

1 in phpunit
- scripts/prepare-phpunit-xml.php (always executed)

1 in test-only changes
- scripts/test-only.sh
Not currently tested in any GTD - needs specific testing elsewhere

I left some more feedback. Most of it is superficial and minor. I think the logic is perfect.

It's also great that we can cover all cases but 1 (the test only) via the GTD!

It's also great that we can cover all cases but 1 (the test only) via the GTD!

Yes it is. I have opened ✨ Add coverage for test-only changes job Active to see if we can automate that too.

The GTD pipelines (two of them) are all passing green, so I will rename two of the files to force a curl error, to check what we get.

With the .sh script file temporarily renamed in the repo the D10Profile composer job failed as intended
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

Likewise D7Basic composer failed as intended
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

Great. Left another comment on one @todo.

Once that is addressed and the files are renamed back to what they need to be (remove the "-zzz") this will be good to go.

D7 expected failure in .get-utility-files
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

Likewise D9-basic expected failure in .get-utility-files
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

When the script name is restored, the composer jobs run, and we get the intended failure in phpcs job (produced by the get-file-via-curl.sh script)
D7 https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
D10 https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

I've checked "test-only changes" with this Scheduler MR, all works OK
https://git.drupalcode.org/project/scheduler/-/jobs/5193386

All debug removed, and file names restored, so this is ready for final review and checking the downstream jobs.

I made some really small MR suggestions to change from this

**************************************************************************************************
 [ERROR] Curl failure
 _CURL_TEMPLATES_REPO=issue/gitlab_templates-3522611  _CURL_TEMPLATES_REF=3522611-curl-return-code  FILENAME=assets/phpcs.xml.dist
 Job ending with EXIT_CODE=22  
**************************************************************************************************

to this

**************************************************************************************************
 [ERROR] Curl failure
 - _CURL_TEMPLATES_REPO=issue/gitlab_templates-3522611
 - _CURL_TEMPLATES_REF=3522611-curl-return-code
 - FILENAME=assets/phpcs.xml.dist
 - FULL_URL=https://git.drupalcode.org/issue/gitlab_templates-3522611/-/raw/3522611-curl-return-code/assets/phpcs.xml.dist

 Job ending with EXIT_CODE=22  
**************************************************************************************************

Yes that's clearer. I have applied the 3 suggestions as-is, but the indentation will need to be checked in the actual jobs, as the line breaks add one space at the start. So I'll re-do the false file names to check the failures, then reset.

.get-utility-files as per MR commit
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
and with indentation fixed
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

get-file-via-curl.sh as per MR commit
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
and with indentation fixed
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...

Filenames restored, all pipelines run green.

Everything looks good now. I just made some really small suggestions about substituting a variable (it could have been a follow-up but it kind of made sense to address it here in one go).

Back to NW just based on that, everything else looks perfect and I'm really happy with the solution that you put together. I even learned the difference between calling a script (runs in a new shell) vs using source (runs in the same shell), so thanks for that too 🙂

All feedback addressed, ready for final review and running all the downstream pipelines for a final check.

I think it all looks good. Thanks so much for this. I think it's a great refactoring.

Merged.

Thanks. It was on my list of "this will be better for everyone in Gitlab Templates if I did it"

I'm sure you have this in hand, but just a reminder that CHANGELOG.md does not yet have these three commits:
https://git.drupalcode.org/project/gitlab_templates/-/compare/default-re...

That sounds like a really good list 💙

Yup, I have the 3 commits stashed in the changelog and will commit them once we do the next release.

The use of $CI_SERVER_URL default variable is problemantic. One suggestion was to use another variable that defaults to $CI_SERVER_URL, so if someone runs this on a non git.drupal.org infrastructure, things can be overridden and things still work.

Thanks for reporting the issue @heddn. I've opened and created an MR to fix the issue in 🐛 Fix CI_SERVER_URL usage as it changes on external instances Active .

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