validate: add validate-sections to validate misspelled directives or directives in reana.yaml #564
Conversation
Codecov Report
@@ Coverage Diff @@
## master #564 +/- ##
==========================================
- Coverage 58.20% 58.14% -0.06%
==========================================
Files 20 20
Lines 2395 2399 +4
==========================================
+ Hits 1394 1395 +1
- Misses 1001 1004 +3
|
reana_client/utils.py
Outdated
| @@ -123,7 +123,11 @@ def _prepare_kwargs(reana_yaml): | |||
| try: | |||
| with open(filepath) as f: | |||
| reana_yaml = yaml.load(f.read(), Loader=yaml.FullLoader) | |||
|
|
|||
| try: | |||
| _validate_sections(reana_yaml) | |||
There was a problem hiding this comment.
Instead of implementing our own validation, I think it's simpler and more scalable to enrich the reana.yaml JSON Schema and define a more strict structure. Then we can catch the errors and display them properly.
Check the JSON Schema docs to see how to restrict to certain properties and so on: https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties
Related issues:
marcdiazsan
force-pushed
the
check-section
branch
from
1bcaed0 to
fda2ad9
Compare
| @@ -123,7 +124,6 @@ def _prepare_kwargs(reana_yaml): | |||
| try: | |||
| with open(filepath) as f: | |||
| reana_yaml = yaml.load(f.read(), Loader=yaml.FullLoader) | |||
|
|
|||
| workflow_type = reana_yaml["workflow"]["type"] | |||
There was a problem hiding this comment.
E.g. If we have typ instead of type we'll have an exception here without even reaching the JSON Schema validation (_validate_reana_yaml(reana_yaml)). Perhaps we can move the validation to be performed before and detect schema problems earlier?
diff --git a/reana.yaml b/reana.yaml
index 9fcfa12..4cf462c 100644
--- a/reana.yaml
+++ b/reana.yaml
@@ -11,7 +11,7 @@ inputs:
year_min: 1500
year_max: 2012
workflow:
- type: serial
+ typ: serial
specification:
steps:
- environment: 'reanahub/reana-env-jupyter:2.0.0'$ reana-client validate
==> ERROR: Something went wrong when trying to validate ~/code/reanahub/reana-demo-worldpopulation/reana.yamlI have no hints on what is wrong.
There was a problem hiding this comment.
Following the IRL conversation, I meant something like this:
diff --git a/reana_client/utils.py b/reana_client/utils.py
index 5bc4312..e840648 100644
--- a/reana_client/utils.py
+++ b/reana_client/utils.py
@@ -124,7 +124,16 @@ def load_reana_spec(
try:
with open(filepath) as f:
reana_yaml = yaml.load(f.read(), Loader=yaml.FullLoader)
- workflow_type = reana_yaml["workflow"]["type"]
+ workflow_type = reana_yaml["workflow"].get("type")
+ if not skip_validation:
+ display_message(
+ "Verifying REANA specification file... {filepath}".format(
+ filepath=filepath
+ ),
+ msg_type="info",
+ )
+ _validate_reana_yaml(reana_yaml)
+ validate_parameters(workflow_type, reana_yaml)
reana_yaml["workflow"]["specification"] = load_workflow_spec(
workflow_type,
reana_yaml["workflow"].get("file"),
@@ -139,16 +148,6 @@ def load_reana_spec(
f, Loader=yaml.FullLoader
)
- if not skip_validation:
- display_message(
- "Verifying REANA specification file... {filepath}".format(
- filepath=filepath
- ),
- msg_type="info",
- )
- _validate_reana_yaml(reana_yaml)
- validate_parameters(workflow_type, reana_yaml)
-
if not skip_validate_environments:
display_message(
"Verifying environments in REANA specification file...",So then the output is more informative:
$ reana-client validate
==> Verifying REANA specification file... /Users/marco/code/reanahub/reana-demo-worldpopulation/reana.yaml
==> ERROR: /Users/marco/code/reanahub/reana-demo-worldpopulation/reana.yaml is not a valid REANA specification:
==> ERROR: The `reana.yaml` directive "typ" is not valid.Note. Beware possible side effects, I haven't tested it deeply.
There was a problem hiding this comment.
I agree to move the _validate_reana_yaml but validate_parameters would need to load the specification file. I would rather separate them
reana_client/utils.py
Outdated
| invalid_section = ( | ||
| ".".join(e.path[1], e.message.split("'")[1]) | ||
| if len(e.path) > 1 | ||
| else e.message.split("'")[1] | ||
| ) |
There was a problem hiding this comment.
For simplicity, perhaps we can just use e.message here? Unless I'm missing something, the messages are quite descriptive:
"Additional properties are not allowed ('typ' was unexpected)"
marcdiazsan
force-pushed
the
check-section
branch
from
fda2ad9 to
f55d656
Compare
reana_client/utils.py
Outdated
| raise REANAValidationError( | ||
| '==> ERROR: The `reana.yaml` is not valid: "{0}".'.format(e.message) | ||
| ) |
There was a problem hiding this comment.
Couple of things:
- It'd be nice to keep the same colors for the error messages and rely on
display_message()instead of appending ourselves the"==> ERROR:"prefix - Error code is 0 when failing
| "oneOf": [ | ||
| { | ||
| "required": ["type","file"] | ||
| }, | ||
| { | ||
| "required": ["type","specification"] | ||
| } | ||
| ] |
marcdiazsan
force-pushed
the
check-section
branch
from
f55d656 to
a0ac90d
Compare
Update validate schema to restrict the directives allowed and to be consisten with the documentation Closes reanahub#562, Closes reanahub#404
marcdiazsan
force-pushed
the
check-section
branch
from
a0ac90d to
811d1a0
Compare
Closes #562