action-validator icon indicating copy to clipboard operation
action-validator copied to clipboard

pre-commit fails on multiple changed files

Open aherrmann opened this issue 1 year ago • 3 comments

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 https://github.com/mpalmer/action-validator/issues/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.

aherrmann avatar Nov 02 '24 10:11 aherrmann

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 avatar Nov 03 '24 05:11 mpalmer

@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.

aherrmann avatar Nov 06 '24 12:11 aherrmann

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 avatar Nov 07 '24 08:11 mpalmer