Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(validation): add manifest validation #274

Merged

Conversation

vados-cosmonic
Copy link
Contributor

@vados-cosmonic vados-cosmonic commented May 6, 2024

Feature or Problem

This commit adds validation functions along with utility accessors to Manifest to enable checking WADM manifests for common errors.

As the validation functions are exposed they can be used by downstream crates (ex. wash) to validate WADM manifests or try to catch errors before a manifest is used.

This is up for review in draft state so we can talk about the design -- if it looks good, I'll do the following:

  • Finish adding all the known interfaces

Related Issues

wasmCloud/wasmCloud#2009

Release Information

next

Consumer Impact

Testing

Unit Test(s)

Unit tests with fixtures have been added

Acceptance or Integration

Manual Verification

@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch from 32004ba to 1ea2e06 Compare May 6, 2024 15:51
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks real nice so far, just wanted to double check on the interfaces (since I saw your comment about adding the rest) and around type aliases

@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented May 6, 2024

Some more stuff after discussion with @brooksmtownsend that we should probably add to this:

  • API that handles strings for easy validation (probably a small refactor to use this from file-path based one)
  • Merge checks from server/handler.rs

@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch 2 times, most recently from 2a3899d to 1661e91 Compare May 7, 2024 15:37
@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch 3 times, most recently from 1b64ffd to 70e50a8 Compare May 17, 2024 18:02
@vados-cosmonic vados-cosmonic marked this pull request as ready for review May 17, 2024 18:02
@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch from 70e50a8 to 31ad3ea Compare May 17, 2024 18:05
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit or two, a couple requests and a discussion point that I might just be overthinking. This looks great and I love the reusability of the library!

@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch from 31ad3ea to 1709fab Compare May 20, 2024 17:14
@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch 3 times, most recently from 5794742 to b969e2d Compare May 20, 2024 17:25
Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent job here. Just a few minor blocking things around the publicly exposed crate API and how we return errors to a user

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments all addressed 👍🏻

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments all addressed 👍🏻

This commit adds validation functions along with utility accessors to
`Manifest` to enable checking WADM manifests for common errors.

As the validation functions are exposed they can be used by downstream
crates (ex. `wash`) to validate WADM manifests or try to catch
errors *before* a manifest is used.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
@vados-cosmonic vados-cosmonic force-pushed the feat=add-yaml-validation-logic branch from ee19d97 to a5a2d6d Compare May 20, 2024 22:38
@thomastaylor312
Copy link
Contributor

Leaving it for @brooksmtownsend to merge since he was first reviewer

@thomastaylor312 thomastaylor312 merged commit a54277a into wasmCloud:main May 21, 2024
3 checks passed
@vados-cosmonic vados-cosmonic deleted the feat=add-yaml-validation-logic branch May 21, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants