Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pre-commit fails on multiple changed files #76

Open
aherrmann opened this issue Nov 2, 2024 · 3 comments
Open

pre-commit fails on multiple changed files #76

aherrmann opened this issue Nov 2, 2024 · 3 comments
Labels
nodejs Issue relates specifically to experimental NodeJS support pre-commit-integration Relates to the pre-commit integration

Comments

@aherrmann
Copy link

The pre-commit integration fails when more than one GH actions file is changed within a single commit.
The pre-commit check fails with

- hook id: action-validator
- duration: 0.12s
- exit code: 1

Usage: action-validator <path_to_action_yaml>

Arguments:
  <path_to_action_yaml>  Input file

Using strace reveals that pre-commit attempts to invoke action-validator with multiple files at once

execve("$HOME/.nvm/versions/node/v21.2.0/bin/action-validator", ["action-validator", ".github/workflows/ci.yaml", ".github/workflows/release.yml"], 0x564d7d63bd30 /* 72 vars */) = 0

Trying to invoke action-validator in the same way on the command line directly leads to the same error.

This seems related to #41, but I've raised it as a standalone issue since it occurred in the pre-commit integration specifically and that wasn't mentioned on that other issue. Feel free to close this as duplicate if you prefer to track this under #41 as well.

@mpalmer
Copy link
Owner

mpalmer commented Nov 3, 2024

By default, I think the pre-commit script is supposed to invoke the native action-validator binary. However, that supports multiple files (there's even a test case to prove it), and your strace output seems to show the NPM/WASM version running, which -- as you say -- doesn't support multiple files.

I can think of a few different interpretations of this issue:

  1. The current instructions for the pre-commit integration aren't clear enough that you need the native binary, in which case, a PR to fix that would be accepted.
  2. The pre-commit integration should be able to detect that it's running a "compatible" action-validator somehow, in which case, it's also PR time.
  3. The current state of the pre-commit integration is OK, you just enjoy living on the ragged edge and it's come unstuck, in which case we'll just close this off.

I'll whack a pr-welcome label on this, in case you think 1 or 2 are appropriate; otherwise, if you think 3 is the best interpretation, just close this off. If you think I've got completely the wrong end of the stick, further details would be appreciated.

@mpalmer mpalmer added pr-welcome This would be a good fix/feature, but the maintainer isn't planning on doing the work pre-commit-integration Relates to the pre-commit integration nodejs Issue relates specifically to experimental NodeJS support needsinfo This issue requires further information from the submitter to proceed labels Nov 3, 2024
@aherrmann
Copy link
Author

@mpalmer thanks for the swift reply! I was not aware of the distinction between the native binary and the npm version. I retried with the native binary and it does work as expected. I could not find clear instructions on this difference and the need for the native binary for the pre-commit integration in the README. So, I think 1. is the most appropriate.

@mpalmer
Copy link
Owner

mpalmer commented Nov 7, 2024

Thanks for the feedback. I'll see what I can do to make it more obvious that the npm version is very, very Not Recommended.

@mpalmer mpalmer removed pr-welcome This would be a good fix/feature, but the maintainer isn't planning on doing the work needsinfo This issue requires further information from the submitter to proceed labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nodejs Issue relates specifically to experimental NodeJS support pre-commit-integration Relates to the pre-commit integration
Projects
None yet
Development

No branches or pull requests

2 participants