38 validate action version exists #39

award28 · 2023-03-08T14:35:27Z

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

bcheidemann

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:

Is tests/run now redundant?
If so, can we remove it and update .github/workflows/qa.yml to run cargo test instead
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 · 2023-03-29T17:49:45Z

tests/snapshot_tests.rs

+#[case("001_basic_workflow")]
+#[case("002_basic_action")]
+#[case("003_successful_globs")]
+#[case("004_failing_globs")]
+#[case("005_conditional_step_in_action")]
+#[case("006_workflow_dispatch_inputs_options")]
+#[case("007_funky_syntax")]
+#[case("008_job_dependencies")]
+#[case("009_multi_file")]
+#[cfg(not(feature = "remote-checks"))]


Shouldn't these tests run regardless of whether the remote-checks feature is enabled? (it would be nice for cargo test to run all tests by default)

Suggested change

#[case("001_basic_workflow")]

#[case("002_basic_action")]

#[case("003_successful_globs")]

#[case("004_failing_globs")]

#[case("005_conditional_step_in_action")]

#[case("006_workflow_dispatch_inputs_options")]

#[case("007_funky_syntax")]

#[case("008_job_dependencies")]

#[case("009_multi_file")]

#[cfg(not(feature = "remote-checks"))]

#[case("001_basic_workflow")]

#[case("002_basic_action")]

#[case("003_successful_globs")]

#[case("004_failing_globs")]

#[case("005_conditional_step_in_action")]

#[case("006_workflow_dispatch_inputs_options")]

#[case("007_funky_syntax")]

#[case("008_job_dependencies")]

#[case("009_multi_file")]

I like this as it is, because I don't like it when a test suite makes any more assumptions about the local environment than it absolutely needs to (and "I have a real, working, fully-open Internet connection" is still, in some cases, an incorrect assumption). I sometimes do dev without a working Internet connection, and a test suite that starts barfing red in all directions gives me the absolute pip.

In any event, I never trust developers (including myself!) to run all the tests before pushing (one cannot mandate that a pre-commit hook is in place in a remote repo, after all), so the most important thing is to make sure that CI is comprehensive -- which means, in this case, running the tests with remote-checks enabled.

It would be a nice "quality of life" thing if the test suite could print something at the end of a cargo test run saying "some tests weren't run because the remote-checks feature wasn't enabled", to give people a hint that re-running with extra features would be a good idea, but a lack of it wouldn't be a blocker for merge for me.

@mpalmer when you put it like that I completely agree :D

I see @mpalmer's point, however I made this decision for a different reason :) Interestingly, enabling remote-checks and running those first 9 tests resulted in different output for those tests! Hence why they were disabled for remote checks. We could alter those tests so they are idempotent to the issues they're testing, but I wanted to reduce the changes as much as I could.

bcheidemann · 2023-03-29T17:59:27Z

tests/011_remote_checks_failure/test.yml

+
+    steps:
+      - name: Checkout
+        uses: actions/checkouts@v2


This is a nitpick but it would maybe be clearer to rename this to something obviously wrong like actions/does-not-exist@v2. I had to re-read this a few times because I didn't spot the trailing "s" (possibly this is only an issue for me because I'm dyslexic!).

I'll update this in #48.

CONTRIBUTING.md

bcheidemann · 2023-03-29T18:34:38Z

src/validators/models.rs

+                // In this case, pull access requires authorization. There are simple cases that
+                // only require the bearer token retrieval followed by manifest access, but there
+                // are also others that require user credentials. For now, we should assume that
+                // the tag tag is valid. We could perform authentication, but that would requrie
+                // user creds and adds a whole lot of scope to this feature.
+                return Ok(());


Currently, we are logging a warning to the console in the NPM version when we skip validating a glob. This was a bit of a hack since I didn't anticipate any other use cases for warnings. However, I'm wondering if it would make sense to add record warnings in the validation state, e.g.

#[derive(Serialize, Debug)] pub struct ValidationState { // --snip-- pub warnings: Vec<ValidationWarning>, }

Then we could record and report a warning in this case.

Alternatively, we could also report this as an error but add a severity to errors e.g.

pub enum ErrorSeverity { Error, Warning, }

I guess if we went with the latter approach it would make more sense to rename errors to diagnostics but probably best to avoid breaking API changes.

What do you think?

I'm OK with breaking API changes when we're early in the lifecycle, and I don't have strong opinions on either of these approaches, but it does seem like a good idea to be recording problems for later consumption, rather than logging anything immediately.

bcheidemann · 2023-03-29T22:45:09Z

src/validators/models.rs

+                return Err(ValidationError::Unknown { 
+                    code: "docker_action_not_found".into(),
+                    detail: Some(format!("Could not find the docker action: {}", self.uses)),
+                    path: self.origin.to_owned(),
+                    title: "Docker Action Not Found".into(),
+                });


I think it would be good to add a new variant to the ValidationError enum for missign actions. Maybe something like ValidationError::UnresolvedAction.

bcheidemann · 2023-03-29T22:47:19Z

src/validators/models.rs

+        Err(ValidationError::InvalidGlob {
+            code: "invalid_uses".into(),
+            detail: Some(format!("The `uses` {uses} is invalid.")),
+            path:  self.origin.to_owned(),
+            title: "Invalid Uses".into(),
+        })


Maybe we could use ValidationError::UnresolvedAction as suggested above, or if this is worth covering with it's own error code then perhaps something like ValidationError::InvalidActionFormat.

I like having very granular error codes, so I'm in team "InvalidActionFormat".

mpalmer · 2023-04-01T23:28:36Z

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 · 2023-04-06T14:07:36Z

@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 · 2023-04-06T14:09:56Z

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:

Is tests/run now redundant?

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

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 · 2023-05-01T11:50:41Z

@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

Co-authored-by: Ben Heidemann <56122437+bcheidemann@users.noreply.github.com>

award28 · 2023-05-03T01:19:57Z

@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 · 2023-05-03T01:40:18Z

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

award28 and others added 30 commits March 7, 2023 08:57

Add sections to contributing guide

a11e734

Add rust environment setup.

fcf615a

Update CONTRIBUTING.md

90861f2

Add remote-checks flag

629d854

Capture remote_checks flag in all configs

8d7248e

Create skeleton for validate_remote_checks

7b10b14

First working prototype! Needs some cleanup, but getting closer

d3aa4b5

Working dyn trait

dac8ef2

Action validation is working! Time to get docker and path done

1443dc1

Path working - Docker in progress

be57be9

Docker images working :D

d2c4328

Working for all v2 docker protocols

2139d21

Reset audit workflow

03505b1

Remove unused remote checks flag

41fcda5

Remove spacing

fe6e3bd

Clean up code

3d5cbfd

Cleanup iter

baa6910

Remote option return value

5898f7c

Merge branch 'main' into 38-validate-action-version-exists

334bf46

Remove futures dep

f3e0b82

Remove super import

fb50f1a

Add docs for validator

aba30d4

Add model notes

1546226

Typo

c1ffa2b

spelling

3bdea4f

Running tests through rust testing

08d4af7

Cleanup

c95321a

Add remote-checks tests

4c16bfb

Create snap-snapshots feature

fcd0fb1

Move cfg

476df9b

award28 added 3 commits March 15, 2023 11:28

Update

ded9825

Update contributing.md

8e9d9ab

Update authors

6070859

award28 marked this pull request as ready for review March 17, 2023 04:37

bcheidemann reviewed Mar 29, 2023

View reviewed changes

Update CONTRIBUTING.md

53ffc60

Co-authored-by: Ben Heidemann <56122437+bcheidemann@users.noreply.github.com>

award28 mentioned this pull request May 3, 2023

CONTRIBUTING.md and Testing changes #48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

38 validate action version exists #39

38 validate action version exists #39

award28 commented Mar 8, 2023 •

edited

Loading

bcheidemann left a comment

bcheidemann Mar 29, 2023 •

edited

Loading

mpalmer Apr 1, 2023

bcheidemann Apr 2, 2023

award28 May 3, 2023

bcheidemann Mar 29, 2023

award28 May 3, 2023

bcheidemann Mar 29, 2023

mpalmer Apr 1, 2023

bcheidemann Mar 29, 2023

bcheidemann Mar 29, 2023

mpalmer Apr 1, 2023

mpalmer commented Apr 1, 2023

award28 commented Apr 6, 2023

award28 commented Apr 6, 2023

bcheidemann commented May 1, 2023

award28 commented May 3, 2023

award28 commented May 3, 2023

38 validate action version exists #39

Are you sure you want to change the base?

38 validate action version exists #39

Conversation

award28 commented Mar 8, 2023 • edited Loading

TODO

Background

Uses Validation

Non-Remote uses Validation

remote-checks Feature

Current Limitations

Future Improvements

Other Changes

Documentation

Testing

bcheidemann left a comment

Choose a reason for hiding this comment

bcheidemann Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpalmer commented Apr 1, 2023

award28 commented Apr 6, 2023

award28 commented Apr 6, 2023

bcheidemann commented May 1, 2023

award28 commented May 3, 2023

award28 commented May 3, 2023

award28 commented Mar 8, 2023 •

edited

Loading

Non-Remote `uses` Validation

`remote-checks` Feature

bcheidemann Mar 29, 2023 •

edited

Loading