🇪🇸Spain @fjgarlin

Cool. Feedback applied.

Closed MR796 (thanks!!) in favor of MR802.

MR with approach suggested in this thread: https://drupal.slack.com/archives/C01UHB4QG12/p1747231711012519

aha! found it!

https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Proj...

Two ways to go around it.
- We change that part, by adding sha1 around it.
- Or we add a “safe” ID https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

I think option 1 is good as it will apply to all plugins.

Actually, we were already using md5 by calling getQueryCacheKey. I changed it to "sha1".

However, given the above output in the issue summary, this is not related to storing the "query:...", it's got to be something else.

I created an MR with the fix: https://git.drupalcode.org/project/project_browser/-/merge_requests/802/...

The MR does not solve the issue. The problem is that "keyValue" has a limit for the "name" column

The issue is in this line, in PB code: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Enab...

I suggest that we "md5" (or whatever technique is best) the "$cache_key".
Something like this:
$cache_key = md5($this->getQueryCacheKey($query));

https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na... →

...
"title": "Google Cloud Text-to-Speech Augmentor",
...
"field_composer_namespace": "drupal/augmentor_google_cloud_text_to_speech-augmentor_google_cloud_text_to_speech",
...
"field_project_machine_name": "augmentor_google_cloud_text_to_speech",
...

The "field_composer_namespace" value is 82 characters long.
The data seems correct, so it must be something happening in the code.

No worries at all. I know it was a small sample size, but as it requires coordination and possible changes with the runner it can wait.

In that case. Let's close this for now and focus on the other issues.

Maybe we can try enabling this feature in the future in the gitlab runner, but for now, we're trying to keep changes to a minimum, and this is not a real issue that we are facing right now.

Thanks for the MR and the investigation.

Great. I'll do that then, so we can also rebase the other open MRs.

I think the diference should be noticed in the jobs using the artifacts.

But... I'm confused. It seems that the flag needs to be enabled on the runner (according to the documentation and this issue), as the default value seems to be "false".

But we haven't made any change to the actual running configuration in +1 year (other than the regular updates).

So my confusion actually comes from the core issue, where it says that it "just works": https://www.drupal.org/project/drupal/issues/3521894#comment-16090831 📌 Stop trying to cache node_modules in gitlab jobs Active . Maybe it's somehow enabled via the GitLab admin UI (I don't have access to this).

I want to say that given that this MR is so simple we should probably go ahead and merge it anyway. There is no negative effects and if (or when) the flag is enabled, we should notice some gains.

RTBC.

psf_ → credited fjgarlin → .

Yeah, once it's in beta phase, we should update to this CORE_NEXT_MINOR: 11.2.x-dev. Maybe we can do something in the script to check when 11.2.0-beta* is available.

Testing any pipeline that uses the legacy driver should be good enough, or any D7 pipeline.
I think the change should be pretty safe because it actually happened a few years ago https://github.com/SeleniumHQ/selenium/pull/11409 upstream.

I think that, other than the @todo, everything looks good. RTBC (pending the removal of the todo).

Oh cool, this is a nice improvement on GitLab side. Yeah, let's review URLs and backticks all over.

fjgarlin → created an issue.

nicxvan → credited fjgarlin → .

We should have a sort of before/after to see if it makes any difference.

fjgarlin → created an issue.

Added link to the project shown in the screenshot.

Maybe we can add the suggestions (eg: whitelist->allowlist). https://cspell.org/docs/api/cspell-types/interfaces/Settings#flagwords
That way it will not recommend derivatives of the words.

This was merged and a new tag was made, which is as well the new default-ref for all contrib.

Not in this case:
- (Contrib) https://git.drupalcode.org/project/gitlab_templates/-/blob/main/assets/....
- (Internal) https://git.drupalcode.org/project/gitlab_templates/-/blob/main/assets/i...

We'll need to add them to the internal one too.

The current redirection is something like this:
- "Create release link": https://git.drupalcode.org/project/config_notify/-/releases/new?tag_name...
- 301 to https://www.drupal.org/project/config_notify/releases/new → , which 404s

We could create a menu path like project/%machine_name/releases/new, and from that, fetch the project with that machine_name and redirect to https://www.drupal.org/node/add/project-release/NID →

The logic might be a bit trickier since machine_name does not need to be the same as the repository name.

---

Another option, since releases are fully managed via drupal.org, might be to turn off "Releases" for all projects (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96373)

NW for the chromedriver args. The rest looks good.

From the core issue (comment #7):

It looks like chromedriver have deprecated --whitelisted-ips and replaced it with --allowed-ips, let's see if this works.

We should do this here too I guess.

--

Re change record: yes, let's do it and link to the core issue. As you say, the default is to allow failure, so the overall pipeline result shouldn't be affected. I can make a quick slack post once we merge it, pointing at the change record.

--

Re build.env, yes, it should be ignored in the cspell job.

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 .

I introduced a new variable for this altogether instead of relying on CI_SERVER_URL.

The MR is ready for review. The downstream pipelines are running: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/495616

Note that we don't have any external integration to run this on, but given that we are hardcoding the URL to the correct value, it should just work.

fjgarlin → created an issue.

Merged.

The MR looks good and the tests above show it's working as expected. RTBC.

Great investigation and even greater that the fix will be just one one (yay for reusable snippets).

Here's a test showing that the ultimate value of the active...

I think you forgot to add the link there.

Thanks for the new MR.

I'd say that the last part deserves a separate issue to be investigated and addressed separately.

@oskar_calvo será el host este miércoles (gracias!!)

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 link that was removed in the previous edit is: https://web.archive.org/web/20220320234512/https://www.drupal.org/node/1...

It’s D6 + 13 years old so not sure how relevant it can still be.

Remove dead link to  https://www.drupal.org/node/150220 →

Merged.

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

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 🙂

Sure. Well spotted.

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  
**************************************************************************************************

Yeah, I think so, we now have +3.5K projects running GitLab CI (https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b...)

This has now even passed the usage of DrupalCI ( https://www.drupal.org/project/drupalci/issues/3412417#comment-15410492 → - October 2023).

Any projects wanting help can:
- https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →
- Go through https://project.pages.drupalcode.org/gitlab_templates/
- Ask for help via the community #gitlab channel
- Create an issue in this queue asking for help

I updated the issue summary to reflect that.

Pipeline passed: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/492104
Merging.

fjgarlin → created an issue.

🐛 Attributes are not rendered when showing the source code Active was merged.

🐛 Attributes are not rendered when showing the source code Active was merged. We'll need to re-evaluate this once the change is deployed and the code is re-parsed.

Merged as this will unblock some other critical issues. We can create follow-ups if needed.

This will go with the next deployment round to api.drupal.org.

Thanks so much!

The code looks good and phpstan is green again. The failing tests are not introduced here.
RTBC.

Something like this:

  rules:
    - if: '$FORCE_TEST_ONLY_JOB == "1"'
       when: always
    - *opt-in-current-rule
    - *skip-test-only-changes-rule
    - changes:
        - '**/tests/**/*Test.php'
      when: manual

We can only do it on one branch, or just do it for the parent issue (the one of failing curl) and then revert the change.
Or we can add a new rule that will trigger it, like setting a variable, which we could set in the gitlab_templates project when calling the downstream pipelines.

PHPStan is now happen. PHPCS is not, one variable became unused after the change.

The failing tests in the CI are mostly unrelated to the changes here, and defo not a blocker.

This is how it's rendered in the tests:

The code looks good and it opens the door to some of the other critical issues.
RTBC.

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.

We could somehow alter a file using the before_script and maybe we also need to override the rules section for the "test-only job" on one of the files so it always runs as well.

Trying to add some tests.

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

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

I made a suggestion in the MR and also added a @todo as PB will be part of core eventually.

Actually, as they have different purposes. We can leave it like that.

We already use a "versionless" user agent in some parts, like here: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/scripts/...
curl_setopt($ch, CURLOPT_USERAGENT, 'Curl.ProjectBrowser');

So I suggest to modify it slightly so we can do a regex that covers this and the new one with the version. I'll make a suggestion in the MR.

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.

The templates set the version of Drupal expected for the variant being run. If you want to keep the one set in the module's composer.json you need to add a variable to the composer job as shown in here https://project.pages.drupalcode.org/gitlab_templates/jobs/composer/#pac...

But yeah, if you want to test against the HEAD, the NEXT_MINOR variant is exactly that at the moment.

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 putting the results of the testing. They are really useful.

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.

Postponing based on the previous comment.

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

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

fjgarlin → created an issue.

avpaderno → credited fjgarlin → .

Thanks for rebasing.

Yeah we can investigate, but we will merge only if we find it makes a real difference, which based on #6 / #7, we couldn't really prove.

Merged.

This looks good to me. Thanks for addressing the feedback. RTBC.

Great, thanks for the additional testing and the extra digging.
Merging this. Thanks for reporting and testing!

Running: https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Weird, we can see the quotes in the output but it seems to think that it's an ok-name 🤷

Regardless, the change on this MR still makes sense in my opinion.

I think he's trying in this issue: https://git.drupalcode.org/project/group/-/merge_requests/204/diffs

Yup, you'd just need to change the project and ref value in your .gitlab-ci.yml file point to this fork and branch and you'd be testing the change.
https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/

Untested, but ready for review.

Thanks for reporting, it is definitely a bug! We'll look into it.

Possible workarounds for now are to set the variable as you did or to remove the single quotes.

Merged.

Also created follow-up issue 📌 Unpin chromedriver version and use latest or highest possible version Postponed .

fjgarlin → created an issue.

Tried the "Merge" widget in this issue 📌 Pin chromedriver version to avoid random test failures Active and it worked.

fjgarlin → changed the visibility of the branch 3521685-ping-webdriver to hidden.

Credits.

Credits.

This is merged now. Thanks!

This is merged now. Thanks!

fjgarlin → created an issue.

fjgarlin → made their first commit to this issue’s fork.

fjgarlin → changed the visibility of the branch 7.x-3.x to hidden.

fjgarlin → made their first commit to this issue’s fork.