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: extract workspace-root-path from a new top section from .yaml file #395

Closed
wants to merge 1 commit into from

Conversation

marcdiazsan
Copy link
Contributor

It passes a workspace_root_path as a parameter to rwc

Closes reanahub/reana-client#545

@marcdiazsan marcdiazsan requested a review from tiborsimko August 18, 2021 19:25
@@ -401,10 +401,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 = reana_spec_file.get("workspace", {}).get(
"workspace_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.

Do we have to call it workspace_root_path? Isn't it a bit redundant having workspace at the top level?
IMO we could simplify it and use just path or root_path. What do you think?

...
workspace:
  path: /eos/home-s/simko/myworkflows
...

BTW, is it definite to have workspace at the top level? I thought we were discussing it here.

PS. For get(), you don't need to specify None as it's the default value.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used workspace_root_path as it is not the full workspace_path, the full path would also have the workflow_uuid. So I agree that root_path would be a good simplification

Copy link
Member

@audrium audrium Sep 2, 2021

Choose a reason for hiding this comment

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

Looks good, +1 for workspace_path.
Also, @marcdiazsan, for get(), you don't need to specify None as @mvidalgarcia mentioned

Edit: perhaps we can close this PR since all the commits are included in another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this one can be closed as is included into the other PR

…aml file

It passes a workspace_root_path as a parameter to rwc

Closes reanahub/reana-client#545
@codecov-commenter
Copy link

Codecov Report

Merging #395 (0e508d4) into master (5c718b8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
+ Coverage   49.92%   49.94%   +0.02%     
==========================================
  Files          22       22              
  Lines        1957     1958       +1     
==========================================
+ Hits          977      978       +1     
  Misses        980      980              
Impacted Files Coverage Δ
reana_server/rest/workflows.py 48.20% <100.00%> (+0.12%) ⬆️

@audrium
Copy link
Member

audrium commented Sep 10, 2021

Closing since all the commits from this PR are included in another one

@audrium audrium closed this Sep 10, 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.

workspace: expose workspace choice to users
4 participants