Validate posted schemas #69
Conversation
* inputType is defined recursively to support nested_parallel_by_operation_matrix_inputs * these messages are here instead of in a comment because json doesn't support comments
Not sure if these are correct. Setting them to generate a discussion about what the correct values should be.
|
I wish the schema validation could provide assurances that the inputs, tasks, and edges in the posted json actually specify a valid graph. I discussed this with @davidlmorton who points out that this may not be an important problem to overcome and that run time validation on the endpoint can handle this sort of validation too. I think we also agreed that readability of the posted json is perhaps more important than imposing a structure which enables a json schema to make assurances that the inputs, tasks, and edges in the posted json actually specify a valid graph, especially since the endpoint can do additional validation. |
| return '', 201, { | ||
| 'Location': url_for('workflow-detail', workflow_id=workflow_id) | ||
| } | ||
|
|
||
| except ValidationError as e: | ||
| LOG.exception(e) | ||
| return {'error': e.message}, 400 | ||
| except exceptions.InvalidWorkflow as e: |
There was a problem hiding this comment.
Can we remove the InvalidWorkflow exception class?
There was a problem hiding this comment.
I think we need to keep the InvalidWorkflow exception class for now. It is still being used in the backend to validate dag task names when a workflow is posted. We might also add additional validation constraints that we cannot constrain in the json schema, but wish to catch at the time the workflow is posted.
…n taskDictionary
Therefore the edgeList must contain at least two edges
|
@mark-burnett said:
Is your statement limited to DAGs specified as a method of a task or can it be taken to include the DAG at the top-level of a workflow? I might be using the terminology more loosely than you are, but if we use the same "workflow" type for the top level and for sub-DAGs, then they would have to have the same minProperties value. If we set the minProperties for the taskDictionary to 1, then we should also set the minItems on the edgeList to be 2. |
|
I agree that we should try to use the same schema for the I agree that it also makes sense to set |
|
A travis-ci environment update is causing the test failures. @mkiwala is going to look into updating our tests so that they are compatible with the new environment. |
|
I merged the fixes for the travis-ci failures into this branch. The failure was due to honcho being unable to find the Procfile, even though the correct Procfile location was correctly specified on the honcho command line. The command line parsing in honcho depends on an unsupported behavior of the pythyon argparse library. Under python 2.7.9 that unsupported behavior changed, and caused honcho's command line parsing to break under certain circumstances. The workaround is to change the ordering of arguments to honcho so that the subcommand (e.g., |
|
+1 Changing the document's top level structure can happen in a separate PR. |
This PR attempts to define what json is valid for posting to the workflow service. The schema I present here is based in part on some work already done by @mark-burnett, but deviates from that work somewhat. Some areas that I've already identified for discussion are:
Existing test cases were updated to support the definition of tasks and the removal of the environment property at the top-level.
Closes #65