GeezerVison

Review requested:

• @nodejs/actions
• @nodejs/security-wg
• @nodejs/tsc

CC @nodejs/build @nodejs/linting

Reviewers, please review the commit '(review changes)'. The first commit removes the node_modules directory and may be hard to review in the github UI.

Everything is passing! The failure is because of #54229. The original PR didn't see any objections, is this good to land in the coming days?

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (1bcdba2) to head (6b95bab).
Report is 289 commits behind head on main.

@@            Coverage Diff             @@
##             main   #54231      +/-   ##
==========================================
+ Coverage   87.10%   87.33%   +0.23%     
==========================================
  Files         647      648       +1     
  Lines      181693   182321     +628     
  Branches    34869    34972     +103     
==========================================
+ Hits       158256   159225     +969     
+ Misses      16733    16371     -362     
- Partials     6704     6725      +21     

I'm +1 to this approach, but feel like there's probably folks who have more domain knowledge about this who should actually be the ones to approve it.

I don't think there was any specific reason why those linters were checked into the repo besides maybe offline functionality, but I think that's likely no longer needed.

Great! I'm glad y'all are on board! Seeing as the linting is passing (except for the unrelated issue), all looks good in terms of land-ability.

Although, I assume this needs a CI to land.

CI: https://ci.nodejs.org/job/node-test-pull-request/60949/

CI is passing. 🎉
Linting is passing except for an unrelated failured that was patched in 7327e44.

CI: https://ci.nodejs.org/job/node-test-pull-request/60955/

Everytime I run e.g. make lint-js on this branch, it runs npm. Can you make it sure npm is run only when there were changes in the package-lock.json?

FWIW, it's possible to achieve on-demand node_modules installation with Make by tracking the folder timestamp, but it's not a 100% thing as Make timestamp tracking works best with files. Here is how I do it in my projects:

node_modules: package-lock.json
	npm install --no-save
	@touch node_modules

FWIW, it's possible to achieve on-demand node_modules installation with Make by tracking the folder timestamp, but it's not a 100% thing as Make timestamp tracking works best with files. Here is how I do it in my projects:

node_modules: package-lock.json
	npm install --no-save
	@touch node_modules

I have it setup like:

tools/.jslintstamp:
	cd tools/eslint && $(call available-node,$(run-npm-ci))
	@touch $@

If the user wants to re-build the lint, they have to run make lint-clean.

Hey, I'd love to get some reviews if there aren't any more objections. I don't want to pressure anyone tho.

Any more changes needed?

Thanks for approving! Does anyone else have any more feedback? It's been a while since any reviews.

No objections in a week, is this author ready?

so this is not author ready because there are no recent Jenkins CI run. If you start one, it would be.

CI: https://ci.nodejs.org/job/node-test-pull-request/61796/

Per @aduh95's comment:

• There is a CI run in progress or completed. 🟢
• There is at least one collaborator approval.
• There are no outstanding review comments.

CI: https://ci.nodejs.org/job/node-test-pull-request/61882/

The MacOS CI appears to have failed to start, could someone restart it?

As for the linked linux OpenSSL one, see #54737

CI: https://ci.nodejs.org/job/node-test-pull-request/61895/

FWIW CI is 🟢 (Yay!)

Landed in 437e168