search

mpalmer / action-validator

Tool to validate GitHub Action and Workflow YAML files
GNU General Public License v3.0
271 stars 25 forks source link

feat: NPM support #26

Closed bcheidemann closed 1 year ago

bcheidemann commented 1 year ago

Closes https://github.com/mpalmer/action-validator/issues/25

Adds support for use as an NPM/Node module.

TODO:

bcheidemann commented 1 year ago

@mpalmer I've added a TODO list above based on your previous comment. It would be good to get an idea of what you think the priority order should be for this. Also which of these you want to check off before merging this PR, and which (if any) you think can be handled separately? If you have any guidance/pointers on that would also be appreciated (I'm relatively new to Rust so this is a bit of a learning experience for me 🙂).

bcheidemann commented 1 year ago

I've been working on improving the Node API with some success but encountered an unexpected behaviour of valicos serialize trait. I've raised an issue to determine if this is intended behaviour or a bug.

mpalmer commented 1 year ago

I think the critical threshold is to know if/when a future change breaks the node build, so having the test suite run successfully under node, and have workflow jobs that run that test suite, are the "baseline", I think. Having an automated release process is a prerequisite for making WASM/node a "first class citizen", but could be deferred to a separate PR -- although I'm not sure how much use a WASM-capable action-validator would be without released packages, so my hunch would be that would go into this PR (or one that would be expected to land more-or-less in parallel). Ergonomic improvements could be deferred to a later time, as long as node users are happy with API breakage, or the changes are backwards-compatible.

In summary, then, I'd say the order is:

  1. Make it build/function (seems like it's pretty much already done)
  2. Make sure I don't break anything when I'm making other changes (test suite, QA jobs)
  3. Ergonomic improvements that will be non-backwards compatible
  4. Automated releases (so it can be widely used)
  5. Further improvements to docs, API, etc

1 & 2 are the minimum for this PR, IMO; other parts could be deferred to other PRs, but I'm cool with you making this a Mega PR o' Doom if you prefer.

bcheidemann commented 1 year ago

@mpalmer Apologies for leaving this PR dangling. I've been away for work and haven't had much time to work on this.

I agree with the priorities as you set them out above. I think a Mega PR 'o Doom is probably the best way since, as you pointed out, we basically need to get to 4. for it to be useful anyway!

Current status of the above is:

1. Make it build/function

Yes, this is more or less done.

The only exception here (afaik) is that the glob function is not WASM friendly.

I'm thinking we can probably use a wrapper function with different implementations depending on the build type. This can call into JS to resolve the glob.

2. Test suite, QA jobs

It shouldn't be an issue to adapt/implement a runner for the current test suite, similar to the test/run script but asserting against snapshots of the returned JS object instead of stdout and stderr.

3. Ergonomic improvements that will be non-backwards compatible

EDIT: I've gone ahead and done the implementation as described below - hopefully it's ok but any feedback is very welcome! :)

I could do with a second opinion on this. It seems we have 3 main errors:

  1. Missing files / invalid globs
  2. Missing "needed" jobs
  3. Schema validation

For 3. I was initially thinking to return the Valico's ValidationState hence raising this issue and later this PR.

However, I see 2 main disadvantages here:

  1. The return type of the validate function would be... for lack of a better word, a bit weird, since ValidationState serializes to Maps instead of objects which doesn't fit well into the JS ecosystem.
  2. We would need to somehow return additional errors "in parallel" with the schema ValidationState since we don't rely on Valico for validating 1. and 2. For example, we might return { schemaValidationState: { /* ValidationState */ }, otherErrors: [ /* array of other errors not provided by Valico */ ] }.

I think my preference for addressing this would be to implement our own ValidationState and ValidationError types internal to this package. This way we can maintain control over serialization and the structure. I think this could be used by the underlying validation logic, keeping the data structures in use consistent across the CLI binary and the NPM library. Additionally, it would make distributing action-validator as a crate trivial.

What are your thoughts on this?

4. Automated releases

I haven't started on this but I have some experience with GitHub actions and publishing NPM packages so this should be fairly straightforward.

Would you want to continue publishing to @bcheidemann/action-validator or deprecate it and publish a new package?

5. Further improvements to docs, API, etc

I'll tackle this once the API is finalised :)

mpalmer commented 1 year ago

Don't worry about leaving things hanging; we've all got things to do.

The only exception here (afaik) is that the glob function is not WASM friendly.

Interestingly, there's recently been another bug report about how the current globbing support is out-of-step with GitHub's docs. I am in no way married to the current crate for globbing, so if you find one that's WASM-friendly and better than the current one (in terms of GitHub compatibility) I would be entirely open to a switch.

I think my preference for addressing this would be to implement our own ValidationState and ValidationError types

I'll level with you: the "failure reporting" in action-validator was originally "the simplest thing that could possibly work", and I've never been real happy with it. I'd be entirely open to your turning the errors that come out of Valico into something a little more... sensible, and returning those into the UI for more human-readable error reporting. The CLI error reporting is not, IMO, a backwards-incompatible interface (as in, if the CLI output changes, that's fine), so feel free to knock yourself out -- even if it's just a dump of the ValidationError type, rather than the error that Valico spits back, that's OK (if someone in the future wants a cleaner error report, "PRs welcome").

Would you want to continue publishing to @bcheidemann/action-validator or deprecate it and publish a new package?

I lean towards publishing it under a namespace that "the project" controls, so I'd probably go with @action-validator/action-validator, if that's the usual approach that NPM projects adopt. Does that seem reasonable and in line with NPM norms?

bcheidemann commented 1 year ago

nterestingly, there's recently been https://github.com/mpalmer/action-validator/issues/27 about how the current globbing support is out-of-step with GitHub's docs. I am in no way married to the current crate for globbing, so if you find one that's WASM-friendly and better than the current one (in terms of GitHub compatibility) I would be entirely open to a switch.

Ah good to know! Maybe an opportunity to kill two birds with one stone :)

I'll level with you: the "failure reporting" in action-validator was originally "the simplest thing that could possibly work", and I've never been real happy with it. I'd be entirely open to your turning the errors that come out of Valico into something a little more... sensible, and returning those into the UI for more human-readable error reporting. The CLI error reporting is not, IMO, a backwards-incompatible interface (as in, if the CLI output changes, that's fine), so feel free to knock yourself out -- even if it's just a dump of the ValidationError type, rather than the error that Valico spits back, that's OK (if someone in the future wants a cleaner error report, "PRs welcome").

Thanks for confirming :) The simplicity is definetly appreciated as somone relatively new to Rust - it's very easy to understand what the code is doing!!

I lean towards publishing it under a namespace that "the project" controls, so I'd probably go with @action-validator/action-validator, if that's the usual approach that NPM projects adopt. Does that seem reasonable and in line with NPM norms?

My initial thought was to publish it under action-validator but a) that's already taken and b) I think it's good to have an NPM scope as you suggested so future packages can be published under the same scope. Maybe we could go with something like @action-validator/lib? Then we can also publish @action-validator/cli for a CLI version of this package distributed via NPM.

mpalmer commented 1 year ago

Maybe we could go with something like @action-validator/lib? Then we can also publish @action-validator/cli for a CLI version of this package distributed via NPM.

If that's a common idiom in the NPM ecosystem, I'm fine with going with that.

bcheidemann commented 1 year ago

@mpalmer I think this is now ready for review :)

I haven't made any changes to glob yet since this PR is already quite big and I figured maybe we can defer the glob implementation to a separate PR. Would you be happy with this? I also didn't do any changes to the UI yet since I will defer this to another PR as you suggested :+1: I can't think of anything else that needs to be done but let me know if I've missed something!

Once this is merged, you will need to create the NPM organisation "action-validator" and add NPM_TOKEN as a secret for the repo. Then you can create a release and it will automatically publish both the core and cli packages to NPM with the release version.

Technically, since the two packages deploy in parallel and core has a peer dependency on cli, it could be that someone tries to install the latest version of cli but the latest version of core is not yet published, in which case things would break a bit... I am happy to fix this as part of this PR if you want or can do it in a follow-up.

bcheidemann commented 1 year ago

@mpalmer Thanks for reviewing this 🙂

Normally I am a proponent of squash on merge since I prefer to keep my PRs small and focussed, but in this case I think it makes sense to break it up a bit.

I have quite a busy schedule this weekend but I'll try and address the implementation of ValidationError and tidy up the commits if I can make time or if not then ASAP next week.

bcheidemann commented 1 year ago

@mpalmer I've made the requested changes and tidied up the commit history =)

mpalmer commented 1 year ago

WE ARE LANDED! I'll create an NPM account/key/etc, add it as a repo secret, and then make a release to see this puppy fly!

mpalmer commented 1 year ago

The release CI job seemed to run OK, so I presume that everything is hunky dory in NPM land. Winnah!

bcheidemann commented 1 year ago

The release CI job seemed to run OK, so I presume that everything is hunky dory in NPM land. Winnah!

I've tested it out and it seems to be working perfectly :) Thanks for your support and guidance on this @mpalmer!

There's a couple things which were left out of this PR that I'll probably look into soon if you're happy to accept PRs for them?

  1. Multi-file support for @action-validator/cli
  2. Glob validation support for @action-validator/* (and possibly addressing https://github.com/mpalmer/action-validator/issues/27)
mpalmer commented 1 year ago

More than happy to accept PRs for both of those. Also, what's the chances of getting the testsuite to use @action-validator/cli when run in "WASM mode", rather than reimplementing all the logic from the run shell script in run.mjs? I notice that the JS CI job is failing, because of 009_multi_file, and rather than adding multi-file support to run.mjs, it'd be "cleaner" to just use the CLI package to run equivalently to the native binary CLI, and avoid duplicating the test runner logic. WDYT?

bcheidemann commented 1 year ago

@mpalmer I completely agree. Initially I hadn't planned to add @action-validator/cli, hence the need for run.mjs. But now that it exists, it would be better not to have two test harasses doing essentially the same job.

For now, I have raised a PR to update the snapshot so the CI will pass (https://github.com/mpalmer/action-validator/pull/34).

When I add multi-file support to @action-validator/cli then I will also remove run.mjs and get run to use @action-validator/cli when running in WASM mode as suggested.