Exclude node_modules folder from artifacts

MR in place and ready for review. I triggered KeyCDN and Decoupled pages but can't do the GTD (I was removed from the maintainers in order to test #3426311: Allow testing documentation pages via MRs → ).

Draft CR: https://www.drupal.org/node/3533973 →

I’m a bit wary of running yarn install 4x. That will be extra compute, which is generally more expensive than storage. And it is additional network requests, that have potential to get noticed by upstream providers. We don’t really have a good setup to know the real cost tradeoff.

If we were able to do things like skip eslint & stylelint when JS & CSS aren’t changed in a MR, this would be a great approach. That should be a net reduction in compute.

Making a separate node_modules job/artifact might be an option, although it’d be more wall time to get through each pipeline, don’t know if that’s a good idea.

For now I’ve raised the artifact size limit to 250M to give us a bit more breathing room.

Most jobs aren’t run if there aren’t files of certain type present (eg: no phpunit job will trigger if there are no php files), so that’s already taken.

With the new limit, let’s see how it holds.

This solution can be a plan B if needed or even useful if people have similar issues, so they can customize their integrations.

So I’ll close it for now.

Re-opening (possibly briefly, don't mind if it's closed again but I was looking for an issue like this and found this one) to link to 🐛 drupal_phpunit_find_extension_directories() uses infinite recursion ⇒ more directories = slower tests Needs work and https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

Short version is having node_modules when discovering phpunit tests can add potentially minutes to contrib pipelines.

We might be able to fix the core issue, but it's been stalled for years now. But I'm also wondering if there's a way to make the download of artifacts into the phpunit jobs more targeted so that node_modules is excluded? I've done that with individual files in the core pipeline for lint job caching but not directories yet.

For core we removed our composer build and lint stages because the time taken to composer or yarn install is a lot less than the time taken to get a gitlab runner to then run those and upload them as artifacts for the next stages to use 📌 Merge the build and lint stages in core MR pipelines Downport . There's also 📌 Consider merging the build and validate stages Active for gitlab_templates.

In reply to #3
@fjgarlin I will add you back as maintainer of GTD. But strangely I just tried and cannot trigger the downstream pipelines either. Just get the red "An error occurred while making the request."

In reply to #5 and #6

If we were able to do things like skip eslint & stylelint when JS & CSS aren’t changed in a MR, this would be a great approach. That should be a net reduction in compute.

We have 📌 Only run linting jobs if the files changed make sense for the job Active which sounds like what you want.

Perhaps we can go forward with this issue as opt-in, that way modules like XB could just turn a variable on and use this approach.

I did the above.

The new variable's default value makes everything behave the same as up until now, so no change will be pushed unless opted in, and we've documented it as well.

We don't need the change record in this case, so I'll remove that, but the MR is ready for review again.

@jonathan1055 - I was able to retrigger GTD pipelines again, thanks!
https://git.drupalcode.org/project/gitlab_templates/-/pipelines/538928

I made one more edit/suggestion in the doc page.

Do you think the GTD tests are enough? Shall I try this with Scheduler?

Yes please, try with scheduler. That's got way more tests.

I am assuming that "node_modules" is never needed for PHPUnit tests. In any case, this is opt-in and it is documented how to add it to any job, so it shouldn't be an issue. But yes, let's test scheduler with this feature turn on and off and see the results.

Test using MR381 with no changes, so installs node_modules as usual. Here is the full pipeline - all passed.
The composer log shows

Top 10 folders by size:
489	web/core
364	web/core/node_modules
118	web/core/node_modules/@ckeditor
66	web/core/modules
39	web/core/node_modules/ckeditor5
37	web/core/node_modules/ckeditor5/dist
30	web/core/node_modules/ckeditor5/dist/browser
18	web/core/node_modules/selenium-webdriver
17	web/modules/contrib
17	web/modules

The downloaded artifact zip is 142Mb. Unzipped 643MB

Test using MR381 with _COMPOSER_YARN_INSTALL: 0
Full pipeline all passed.
The composer log does not show web/core/node_modules. We see

Top 10 folders by size:
125	web/core
66	web/core/modules
17	web/modules/contrib
17	web/modules
16	web/core/assets
15	web/core/lib/Drupal
15	web/core/lib
15	web/core/assets/vendor
13	web/core/tests
13	web/core/assets/vendor/ckeditor5

The downloaded artifact zip is 57Mb. Unzipped 261MB, a significant decrease.

The eslint, stylelint and cspell jobs have this, which I don't understand, but is probably nothing to worry about:

➤ YN0007: │ @nightwatch/nightwatch-inspector@npm:1.0.1 must be built because it never has been before or the last one failed
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT 
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT > @nightwatch/nightwatch-inspector@1.0.1 build
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT > node preprocessExtension.js
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT 
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT 📂 Moving to src folder ...
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT 🚀 Creating .crx file ...
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT ✅ .crx file created successfully!

Note that this only tests ESlint, Stylelint and CSpell, because Scheduler does not have any Nightwatch tests. I've made a couple of comments in the MR but it all looks good.

Thanks for the review.

That YN00... can be ignored.

I think that the new snippet should capture the folder we're in at that moment and then cd backinto it after running yarn install.

Back to NW based on that. Would you be ok doing that small change? Otherwise I could too.

I have pushed the change to save then restore the current working directory. I also made the change to see if Stylelint fails when corepack enable is repeated in the script. But then I realised we do not yet have GTD coverage for Stylelint - it is the only remaining job left to add. We have had 📌 Add test coverage for stylelint job Active open for a while, so I have created a MR on that issue. Worth doing this properly now we have a direct need to test Stylelint.

100%, it's great that we found a case to do it. That will improve test coverage.

I also triggered the downstream pipelines for KeyCDN and Decoupled pages as they both have stylelint job: https://git.drupalcode.org/issue/gitlab_templates-3533957/-/pipelines/54...

Error in stylint: https://git.drupalcode.org/project/keycdn/-/jobs/5810304

Corepack must currently be enabled by running corepack enable in your terminal. For more information, check out https://yarnpkg.com/corepack.

Pipeline after the above change: https://git.drupalcode.org/issue/gitlab_templates-3533957/-/pipelines/54...
Job that was failing is now passing again, as expected: https://git.drupalcode.org/project/keycdn/-/jobs/5816533

Good, that confirms your comment in the MR that the "enable" is needed. We almost knew that was the case, but good to check. I have reverted that last commit. Now that MR26 is merged we get a downstream stylelint job too. Ready for review.

Also testing with _COMPOSER_YARN_INSTALL: 0 in the d10-plus branch here

Looks good. RTBC pending the removal of the temporary code. It will also need a follow up commit on the GTD to test this new variable.

I have removed the temporary lines and created MR27 on #3513408-13: Add test coverage for stylelint job → for adding _COMPOSER_YARN_INSTALL: 0

I also made a readability improvement - the tests all run and pass the same, hope that was OK after you RTBC.

This is now merged.

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