ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 6 forks source link

Validate that the workflow payload is correct #136

Closed agrare closed 1 year ago

agrare commented 1 year ago

part of: #130

Rules from ASL spec https://states-language.net/

Top-level fields

States

Transitions

Timestamps

Data

Reference Paths

Payload Template

Intrinsic Functions

There are more...

https://github.com/ManageIQ/floe/issues/130

kbrock commented 1 year ago

Why is this WIP? This is a beauty and needs to get merged stat ;)

Maybe we should move the criteria into the parent issue? and add check boxes? Also noticed (in AWS specs) that some of the specs are across all states, and others are state specific. Maybe we can separate these 2 concepts so it is easier to follow?

agrare commented 1 year ago

Why is this WIP? Maybe we should move the criteria into the parent issue? and add check boxes?

Yes I was seeing how far I could get through all of the rules but I think this is in a pretty good spot, I'll move the list to the parent issue and take this out of WIP

agrare commented 1 year ago

Okay I wanted to get the Next state checks for Choice done, have added that now

Fryguy commented 1 year ago

Are there any ordering issues here? That is, are the states validated as they are being processed, and if so, could a state defined after a reference to it cause issues? For example, I see the Choice state validates the "Default", but what if the Default state is defined after the Choice state. I feel like that is a common layout and would have been caught in specs, but the way I'm reading the code, it feels like it could still happen?

agrare commented 1 year ago

That's why we're validating the payload rather than the parsed states objects when checking for things like "does Default exist in States"

Fryguy commented 1 year ago

Oh I see, so for https://github.com/ManageIQ/floe/pull/136/files#diff-76feb017cc942fa02bf72a61cb066c70342a42039ace6610b861b9de0a5463a9R52, the payload of the higher order workflow is already parsed and validated at this point.

Thanks :+1:

Fryguy commented 1 year ago

super-super-minor, but it makes me wonder if the method should be named validate_payload to make it clear we are validating against the raw payload.

agrare commented 1 year ago

but it makes me wonder if the method should be named validate_payload to make it clear we are validating against the raw payload.

Possibly? I was thinking there could be a number of things that we are validating and the raw payload is just one of them though

miq-bot commented 1 year ago

Checked commit https://github.com/agrare/floe/commit/8b0590d5a04036624090edb2c8717e3bc26f42ba with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint 14 files checked, 0 offenses detected Everything looks fine. :+1: