Fix action-validator to run for each file on pre-commit hook

[award28]

Changes

• bin/run-action-validator is now an executable file.
• action-validator only takes a single file as input. This will fix pre-commit runs when multiple files are passed to entry script.

As an alternative, I'd be happy to raise a PR which let's action-validator accept multiple files as inputs. However, this would come with some questions:

1. What should happen if 1 file is valid but another is not? Or if 1 file is valid but multiple are not?
2. What should happen if 1 file fails? Does the CLI output to stderr on the first failure, or accumulate failures and report all of them?

[mpalmer]

As PR #31, action-validator does accept multiple files on the command line, and that PR was (I hope!) part of the 0.4.0 release. If you wind back your PR to include only the permissions change, I'll merge that ASAP, and we can investigate why the multiple file feature doesn't appear to be doing what you expect separately.

[award28]

As PR #31, action-validator does accept multiple files on the command line, and that PR was (I hope!) part of the 0.4.0 release. If you wind back your PR to include only the permissions change, I'll merge that ASAP, and we can investigate why the multiple file feature doesn't appear to be doing what you expect separately.

@mpalmer interesting - I have a feeling this is related to the cli being run with the all file names in a double quoted string. Let me try again with each of them as arguments.

[award28]

Yeah, I tried it a few different ways and it didn't work. Maybe I'm invoking the command incorrectly?

❯ action-validator
Usage: action-validator <path_to_action_yaml>

Arguments:
  <path_to_action_yaml>  Input file

#################
# This one worked
#################
❯ action-validator .github/workflows/continuous_deployment.yml

############################
# None from here down worked
############################
❯ action-validator .github/workflows/continuous_deployment.yml .github/workflows/continuous_integration.yml
Usage: action-validator <path_to_action_yaml>

Arguments:
  <path_to_action_yaml>  Input file
❯ action-validator ".github/workflows/continuous_deployment.yml" ".github/workflows/continuous_integration.yml"
Usage: action-validator <path_to_action_yaml>

Arguments:
  <path_to_action_yaml>  Input file
❯ action-validator ".github/workflows/continuous_deployment.yml .github/workflows/continuous_integration.yml"
File not found: .github/workflows/continuous_deployment.yml .github/workflows/continuous_integration.yml

[award28]

Something to consider - even if the action-validator cli command does accept multiple path arguments, the bin/run-action-validator shell script likely will not pass it properly, as it will pass all files in a single double quoted string.

[mpalmer]

Maybe I'm invoking the command incorrectly?

Your example invocations work for me. I'm pretty sure you're using a pre-0.4.0 version of action-validator. Try action-validator --version.

it will pass all files in a single double quoted string.

How so? The "$@" construction is a "magic" one for bash, in that the shell expands each argument and quotes it separately. From the "Special Parameters" section of bash(1):

When the expansion occurs within double quotes, each parameter expands to a separate word. That is, "$@" is equivalent to "$1" "$2" ...

[award28]

Super interesting!! I did not know about that expansion in bash! And you're definitely right - I installed it with npm the first time around, so maybe that's why. When I install it with npm and run it with --version, it isn't aware of that flag.

❯ action-validator --help
Usage: action-validator <path_to_action_yaml>

Arguments:
  <path_to_action_yaml>  Input file

[mpalmer]

Aaaaaah... NPM is, shall we say, somewhat experimental. Perhaps @bcheidemann can step in and explain what might be going on there.

[bcheidemann]

Hey @award28, thanks for flagging this! Support for installation via NPM is relatively new and it is not currently 1:1 compatible with the native binaries. @mpalmer and I have discussed this and the plan is for @action-validator/cli and the native binary to be 1:1 feature compatible.

To this end, the following changes are at the top of my todo list:

1. Adding multi-file support for @action-validator/cli
2. Unifying the test suite to ensure 1:1 compatibility between the native binaries and @action-validator/cli is maintained
3. Adding support glob validation for @action-validator/cli and @action-validator/core

Unfortunately, I haven't had time to address them this week as I've been suuuper busy at work and my free time has been mostly consumed by another project with a pretty tight deadline 🫠

I can't 100% commit to timelines yet but I'm hoping to have a PR for 1. and 2. this week. Number 3. will require a bit of thought since I ideally want to address #27 in the same PR, but I will aim to address that sooner rather than later fwiw.

[mpalmer]

Pardon me while I 🤦. 0.5.1 rolling out now.

[award28]

Hey @bcheidemann, thanks for all of this info! That's perfectly reasonable; even the fact that it works with npm at all is very cool/impressive.

I'm currently working on #38 to implement a remote_checks feature, but would be happy to contribute more if you want a hand with any of the npm work!

[bcheidemann]

I'm currently working on #38 to implement a remote_checks feature, but would be happy to contribute more if you want a hand with any of the npm work!

@award28 if you're interested in helping with the WASM support that would be great! Is there anything in particular you'd be interested in working on? I have a branch where I started working on no.2 above but no.1 and 3 are fair game :)

Although, I have been thinking a bit about how to make supporting NPM more sustainable since the current implementation requires JS glue for any kind of system access (network, filesystem, etc), and for that reason I've raised this issue to discuss the potential of pivoting from wasm-bindgen to WASI. If we go ahead with that, it might essentially solve no.3, so perhaps worth holding off on it until we have consensus from @mpalmer on which direction he wants to go with this 🤔

[award28]

@bcheidemann given you're on 2 and the approach for 3 is being reconsidered I can look into 1! Is there anything else to add with that work besides "multi-file input"? If not, I'll make an issue to track it and get started on it after the remote-checks feature is ready for review.

[bcheidemann]

@bcheidemann given you're on 2 and the approach for 3 is being reconsidered I can look into 1! Is there anything else to add with that work besides "multi-file input"? If not, I'll make an issue to track it and get started on it after the remote-checks feature is ready for review.

Excellent! Maybe stating the obvious, but my view is it should behave the same as the native binary does when provided a list of files. Otherwise I have nothing else to add :)