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

workflows: add validate function to workspace_root_path #397

Merged
merged 3 commits into from
Sep 15, 2021

Conversation

marcdiazsan
Copy link
Contributor

@marcdiazsan marcdiazsan requested a review from tiborsimko August 26, 2021 03:48
@marcdiazsan marcdiazsan self-assigned this Aug 26, 2021
@marcdiazsan marcdiazsan marked this pull request as draft August 26, 2021 03:51
type: object
properties:
workspaces_available:
type: list
Copy link
Member

@audrium audrium Sep 1, 2021

Choose a reason for hiding this comment

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

There is a small mistake here, it should be array instead of list. You can update reana_server openapi docs and publish it to reana-commons by doing the following:

cd reana_server
FLASK_APP=reana_server/app.py python ./scripts/generate_openapi_spec.py --publish
diff -q -w temp_openapi.json docs/openapi.json
mv temp_openapi.json docs/openapi.json
reana-dev git-submodule --update

edit: it seems you also have to specify types of list items, something like this:

...
properties:
  workspaces_available:
    type: array
    items:
      type: string
...

Copy link
Member

@audrium audrium left a comment

Choose a reason for hiding this comment

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

Working nicely, just leaving minor comments 🚀

reana_server/rest/workspaces.py Outdated Show resolved Hide resolved
reana_server/rest/workspaces.py Outdated Show resolved Hide resolved
reana_server/rest/workspaces.py Outdated Show resolved Hide resolved
@@ -401,10 +402,15 @@ def create_workflow(user): # noqa
workflow_dict["operational_options"] = validate_operational_options(
workflow_engine, reana_spec_file.get("inputs", {}).get("options", {})
)
workspace_root_path = validate_workspace(
reana_spec_file.get("workspace", {}).get("root_path", None)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: None is default option for .get(), no need to specify it

Request failed. Internal controller error.
"""
try:
if ADMIN_ALLOWED_WORKSPACES:
Copy link
Member

Choose a reason for hiding this comment

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

Please see my comment, I think this part could be simplified if we would store the final list in reana-commons

Copy link
Member

@audrium audrium left a comment

Choose a reason for hiding this comment

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

LGTM!

P.S. tests are passing locally

@audrium audrium marked this pull request as ready for review September 13, 2021 07:59
@codecov-commenter
Copy link

Codecov Report

Merging #397 (61e54de) into master (5d68c6c) will increase coverage by 0.12%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   49.92%   50.05%   +0.12%     
==========================================
  Files          22       23       +1     
  Lines        1957     1974      +17     
==========================================
+ Hits          977      988      +11     
- Misses        980      986       +6     
Impacted Files Coverage Δ
reana_server/rest/workspaces.py 57.14% <57.14%> (ø)
reana_server/factory.py 100.00% <100.00%> (ø)
reana_server/rest/workflows.py 48.32% <100.00%> (+0.24%) ⬆️

@audrium audrium merged commit 61e54de into reanahub:master Sep 15, 2021
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.

validate: check desired workspace value
3 participants