-
Notifications
You must be signed in to change notification settings - Fork 12
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
CLI: add better validation on workflows #681
Comments
Hey @josephjclark , While working on the issue, I came across a test in To address this, I've considered adding more validation functions to the
Also, I had a doubt about this statement "A step with an adaptor should also have an expression (steps without expressions are fine)". This basically means if the step has an adaptor, it should have an expression as well, but if an adaptor is missing there is no need for an expression. Have I interpreted it correctly? This approach does not break any tests for the runtime except for the one mentioned. Could you please review this and let me know if this aligns with what you had in mind? Additionally, do you have any suggestions or improvements for this approach? |
Hi @SatyamMattoo , That See these validations implemented in code is making me a little anxious. I'd like to think about this a bit more after I've had my coffee. I'm worried we're being too strict - maybe this should go into the CLI, not the core runtime. I'm not sure - let me think it over.
This is correct! |
Thank you @josephjclark, I'll update the test accordingly. Regarding the validations in the code, I completely understand your concern. If you have any further thoughts or suggestions, I will be more than happy to help. |
@josephjclark I am also looking into the issue and I think the approach provided by @SatyamMattoo of Custom Validation Functions is correct way to go as here we are validating the structure of the workflow, steps, and options in the execution plan.
Your concern is also valid so I think we can iterate over other approach which can be :-
|
@SatyamMattoo can you open a PR with your work so far please? @Pin4sf We heavily use JSON schema across our stack - if we were to introduce a schema here, that would be the way to go. But we already have a formal type declaration in TypeScript. I'd rather use that than duplicate it, |
@josephjclark I am currently working on this issue under C4GT, will raise a PR shortly |
@Dharssini This is not a C4GT DMP issue. If you want to apply for one of those you have to apply to the portal directly. This issue is being worked on by @SatyamMattoo and does not require more input. Thanks! |
When loading a workflow JSON, the CLI should do more validation work, warning users when a hand-made workflow is incorrect.
The runtime actually has a
validate-plan.ts
file - it should be acceptable to just add extra validation rules here. They should bubble up to the CLI (for example, a workflow.json which specifies a start step not defined in the workflow will throw a validation error, ie{ options: { start: "x" }, workflow: { steps: [ { id: "a" }] } }
Validation rules should include:
workflow
key at the root, and theworkflow
must have asteps
key, which should be an array.options
objectPlus anything else that may be useful
All validation rules should be thoroughly unit tested.
A big risk is that adding extra validation will break unit tests, which often delight in using invalid workflows. I think there's already a
skipValidation
option which we can pass to the runtime and we may need to add this. If this is breaking lots and lots and lots of tests we may want to re-think (perhaps validation should be opt-in, and we need to change the CLI to always pass avalidate: true
flag)The text was updated successfully, but these errors were encountered: