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

[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:

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]

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")]

[mpalmer]

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.

[bcheidemann]

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

[award28]

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]

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.

If we want to use a Cargo feature to enable/disable this functionality at build time, we should be able to do this as follows:

#[fixtures(["tests/fixtures/*"])]
#[cfg_attr(
    feature = "remote-checks",
    fixtures::ignore(
        paths = "tests/fixtures/{001_basic_workflow,002_basic_action,...}",
        reason = "because the remote-checks feature was enabled. This causes different output for this test."
    )
)]
#[cfg_attr(
    not(feature = "remote-checks"),
    fixtures::ignore(
        paths = "tests/fixtures/{010_remote_checks_ok,011_remote_checks_failure}",
        reason = "because the remote-checks feature was disabled."
    )
)]
#[test]
fn snapshot(dir: &Path) {
    SnapshotTest::new(dir).execute();
}

This should do it.

test snapshot::_001_basic_workflow ... ignored because the remote-checks feature was enabled. This causes different output for this test.

test snapshot::_010_remote_checks_ok ... ignored because the remote-checks feature was disabled.

[bcheidemann]

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!).

[award28]

I'll update this in #48.

[bcheidemann]

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?

[mpalmer]

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]

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

[bcheidemann]

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.

[mpalmer]

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

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

[mpalmer]

Now that #48's spiritual successor has landed, are you willing/able to rebase this to get it landed, @award28?

[award28]

Hey @mpalmer ! Apologies about dropping off, I became quite busy and lost the time.

Let's do it 🏄 I'll take a peak at it this week and make some updates here.

[bcheidemann]

Is there a reason why this should be cargo feature, or could we make this a CLI flag instead? e.g. --allow-remote-checks. This might be a little more flexible than opting in at build time.

[award28]

@mpalmer let me know if there's anything I missed!

[bcheidemann]

Why not return the refs from collect_uses? e.g.

let refs = collect_uses(doc, state);

[bcheidemann]

It feels a little inelegant to check the action type here, since it's already checked further up the call stack and the two paths don't share any code:

collect_uses -- checks action type
validate_uses
run -- already knows action type

The additional check could be avoided by breaking up collect_uses into two functions (collect_uses_for_workflow and collect_uses_for_action):

stateDiagram-v2
    [*] --> run

    state type <<choice>>

    run --> type

    type --> validate_uses_for_workflow
    validate_uses_refs --> [*]

    state validate_uses_for_workflow {
        collect_uses_refs_for_workflow
    }

    validate_uses_for_workflow --> validate_uses_refs

    type --> validate_uses_for_action

    state validate_uses_for_action {
        collect_uses_refs_for_action
    }

    validate_uses_for_action --> validate_uses_refs

Loading

[bcheidemann]

@award28 I didn't have time to do a full review but a I did a quick scan and to me it all looks good. I think JS side should be easy enough to implement as well, but I'd be happy to pick that up once this is merged.