38 validate action version exists

[award28]

TODO

• [ ] Update the README.md to include a section on the remote-checks feature flag.

Background

This PR resolves #38, adding a feature flag to enable remote checks. When the flag is enabled, action-validator performs remote checks to validate that a provided action exists, as well as the specified commit/tag/version. There are a few other additions as well, covered in the release notes.

Uses Validation

Non-Remote uses Validation

The uses validation from the schema store does not perform any validation on the value which is provided. However, this value can't be any string; it must be one of a few values.

There are a few variations for each, but the core difference is that a valid uses is either a GitHub Action, a Docker image, or a path to an action. We can perform non-remote checks for each of these by making sure they match the expected uses type. Further, for a path value, we can validate that the path exists.

remote-checks Feature

The real benefit of this PR is the remote-checks feature flag. With this enabled, the user is giving action-validator permission to perform validation that can only be confirmed through network calls. For the uses statement, we perform http requests to verify a given action exists or a docker image exists. There are some limitations to this, listed below.

Current Limitations

• 401 responses from a Docker Registry are assumed as available
• Private github actions can't be found

Future Improvements

• Validate with attributes on uses statements
• Perform docker auth to confirm images when we receive a 401
• Use async tokio main to enable async requests
• Validate action in supplied path is actually an Action

Other Changes

Documentation

• Updated the CONTRIBUTING.md file with sections covering environment setup, running the validator locally during development, and writing tests.

Testing

• Updated the test directory name from test to tests so rust can find it automatically.
• Recreated the tests/run shell script in the tests/snapshot_tests.rs to take advantage of rusts testing capabilities
  • Parameterized the snapshot tests
  • Added the save-snapshots feature flag to easily update outdated snapshots

[mpalmer]

Sorry, @award28, I missed that you'd marked this ready for review.

I'm in agreement with most of @bcheidemann's comments (I'll respond to those on which I've got a differing opinion shortly). If you're up for it, I'd really appreciate it if you could pull out the dev instruction improvments and test suite run changes into their own PRs, because (a) I'd like to get those in ASAP, and (b) I find it quite difficult to wrap my head around reviewing changes with multiple different purposes (I get some sort of mental whiplash flicking back and forth).

[award28]

@mpalmer no worries, been very busy with work recently. This feedback sounds good, I'm happy to split this apart into two different PRs!

[award28]

This looks great :) I think moving the snapshot test suite to Rust makes a lot of sense since it will be easier to maintain and makes it easier to test @action-validator/cli with the same runner. If @mpalmer approves, once this is merged I plan to integrate the tests from tests/run.mjs.

In addition to the comments I left, I have the following questions:

1. Is tests/run now redundant?

2. If so, can we remove it and update .github/workflows/qa.yml to run cargo test instead

3. I think some minor updates are needed to tests/run.mjs (updating test to tests) and also the validation_state.snap.json files need to be added for the new test cases.

Regarding point number 3, I am happy to do that myself or to give further instruction on what needs to change.

@mpalmer what do you think? Would be great to get your input on this PR when you have time since I think it probably needs to go in ahead of further Node API related re-factoring & re-factoring of the test suite.

@bcheidemann This all sounds good to me! I didn't remove the shell script for the run since I wasn't sure if we would want to go with this approach but I'm good to remove it based on @mpalmer's feedback.

[bcheidemann]

@award28 let me know if there's any way I can help with this :) I have some time off work this week so would be happy to help any way I can

[award28]

@bcheidemann thanks for the hand, Ben! I've definitely been distracted with other stuff and haven't had the time to address this feedback. I'm going to work on extracting the testing changes/contributing.md into their own PR tonight. Once that's done, it'd be great to get some feedback on that.

[award28]

Once #48 is merged, I'll rebase this branch against main and update the PR based on the feedback.