DavidAnson / markdownlint-cli2

A fast, flexible, configuration-based command-line interface for linting Markdown/CommonMark files with the markdownlint library
MIT License
350 stars 48 forks source link

Pin node version in pre-commit hooks #437

Open mschoettle opened 4 hours ago

mschoettle commented 4 hours ago

We run pre-commit in our CI pipeline and it suddenly started failing yesterday with the following error when initializing the markdownlint-cli2 hook:

[INFO] Installing environment for https://github.com/DavidAnson/markdownlint-cli2.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/root/.cache/pre-commit/repojphqc849/node_env-default/bin/node', '/root/.cache/pre-commit/repojphqc849/node_env-default/bin/npm', 'pack')
return code: 254
stdout: (none)
stderr:
    (node:192) ExperimentalWarning: Support for loading ES Module in require() is an experimental feature and might change at any time
    (Use `node --trace-warnings ...` to show where the warning was created)
    npm warn tarball tarball data for file:/root/.cache/pre-commit/repojphqc849/ (null) seems to be corrupted. Trying again.
    npm warn tarball tarball data for file:/root/.cache/pre-commit/repojphqc849/ (null) seems to be corrupted. Trying again.
    npm error code ENOENT
    npm error syscall lstat
    npm error path /root/.cache/pre-commit/repojphqc849/arsers/jsonc-parse.js
    npm error errno -2
    npm error enoent ENOENT: no such file or directory, lstat '/root/.cache/pre-commit/repojphqc849/arsers/jsonc-parse.js'
    npm error enoent This is related to npm not being able to find a file.
    npm error enoent
    npm error A complete log of this run can be found in: /root/.npm/_logs/2024-10-17T02_41_01_730Z-debug-0.log
Check the log at /root/.cache/pre-commit/pre-commit.log

I was able to reproduce this locally as well after running pre-commit clean to get rid of the cache. In the log I noticed that node 23.0.0 is used. I am not entirely sure what causes this (I see that it is looking for arsers/jsonc-parse.js instead of parsers/jsonc-parse.js.

pre-commit hooks can specify a language_version so I propose to use this. nodeenv actually has the ability to use the LTS version so that would make it consistent with the Dockerfile:

https://github.com/DavidAnson/markdownlint-cli2/blob/da711fa8b30421506086eb2dc7ea0a6c3cb27b9f/docker/Dockerfile#L15

DavidAnson commented 3 hours ago

While I completely agree this change is appropriate, when I made it four years ago I was told very clearly by the pre-commit team that they did NOT want tools doing so and I had to revert the change. I don't agree that performance is more important than correctness, but it's their tool and people who choose to use it should play by their rules.

If that team's stance has changed in the last four years, I would love to hear that. If you can please point to documentation or reach out to one of them to confirm this is now acceptable practice, I am happy accept your PR to fix this.

Here is the relevant conversation: https://github.com/igorshubovych/markdownlint-cli/issues/157