Open award28 opened 1 year ago
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).
@mpalmer no worries, been very busy with work recently. This feedback sounds good, I'm happy to split this apart into two different PRs!
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 fromtests/run.mjs
.In addition to the comments I left, I have the following questions:
Is
tests/run
now redundant?If so, can we remove it and update
.github/workflows/qa.yml
to runcargo test
insteadI think some minor updates are needed to
tests/run.mjs
(updatingtest
totests
) and also thevalidation_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.
@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
@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.
Once #48 is merged, I'll rebase this branch against main
and update the PR based on the feedback.
TODO
README.md
to include a section on theremote-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
ValidationThe
uses
validation from the schema store does not perform any validation on the value which is provided. However, this value can't be anystring
; 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
FeatureThe real benefit of this PR is the
remote-checks
feature flag. With this enabled, the user is givingaction-validator
permission to perform validation that can only be confirmed through network calls. For theuses
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
Future Improvements
with
attributes on uses statementsOther Changes
Documentation
CONTRIBUTING.md
file with sections covering environment setup, running the validator locally during development, and writing tests.Testing
test
totests
so rust can find it automatically.tests/run
shell script in thetests/snapshot_tests.rs
to take advantage of rusts testing capabilitiessave-snapshots
feature flag to easily update outdated snapshots