cli: implement share-add command #683
Conversation
DaanRosendal
force-pushed
the
feature/share-add
branch
3 times, most recently
from
3f75e8c
to
508acbd
Compare
Adds a new command to the CLI to share a workflow with a user. Closes reanahub#680
DaanRosendal
force-pushed
the
feature/share-add
branch
from
508acbd
to
fdeaa1d
Compare
| help="Optional date when access to the workflow will expire for the given user(s) (format: YYYY-MM-DD).", | ||
| ) | ||
| @click.pass_context | ||
| def share_workflow_add( |
There was a problem hiding this comment.
nitpick:
| def share_workflow_add( | |
| def workflow_share_add( |
There was a problem hiding this comment.
Done
The changes were applied to the following superseding PR: #692
| @click.option( | ||
| "-v", | ||
| "--valid-until", | ||
| help="Optional date when access to the workflow will expire for the given user(s) (format: YYYY-MM-DD).", | ||
| ) |
There was a problem hiding this comment.
We can already check whether the date is valid here, see for example https://github.com/reanahub/reana-server/blob/426e8a2b1968c6f3f17e01163c2b173c23389b9b/reana_server/reana_admin/cli.py#L777
There was a problem hiding this comment.
Thanks for the suggestion. Added.
The changes were applied to the following superseding PR: #692
| $ reana-client share-add -w myanalysis.42 --user [email protected] | ||
|
|
||
| $ reana-client share-add -w myanalysis.42 --user [email protected] --user [email protected] --message "Please review my analysis" --valid-until 2025-12-31 |
There was a problem hiding this comment.
| $ reana-client share-add -w myanalysis.42 --user bob@example.org | |
| $ reana-client share-add -w myanalysis.42 --user bob@example.org --user cecile@example.org --message "Please review my analysis" --valid-until 2025-12-31 | |
| \t $ reana-client share-add -w myanalysis.42 --user bob@example.org | |
| \t $ reana-client share-add -w myanalysis.42 --user bob@example.org --user cecile@example.org --message "Please review my analysis" --valid-until 2025-12-31 |
To show the example commands indented when calling reana-client share-add --help
There was a problem hiding this comment.
Done
The changes were applied to the following superseding PR: #692
| share_errors = [] | ||
| shared_users = [] | ||
|
|
||
| if workflow: |
There was a problem hiding this comment.
No need to do if workflow: ... else: display_message(...), as this is already handled by @add_workflow_option. I think there are still some commands that do this, but it's a leftover from the past (that we should probably clean up at some point 😅 )
There was a problem hiding this comment.
Nicely spotted. Changed
The changes were applied to the following superseding PR: #692
| share_errors.append( | ||
| f"Failed to share {workflow} with {user}: {str(e)}" | ||
| ) | ||
| logging.debug(traceback.format_exc()) |
There was a problem hiding this comment.
nitpick: You can use logging.exception("... message ...") instead of calling logging.debug manually
There was a problem hiding this comment.
Done
The changes were applied to the following superseding PR: #692
| except Exception as e: | ||
| logging.debug(traceback.format_exc()) | ||
| logging.debug(str(e)) | ||
| display_message( | ||
| "An error occurred while sharing workflow:\n{}".format(str(e)), | ||
| msg_type="error", | ||
| ) |
There was a problem hiding this comment.
In which cases can we end up here? All the exceptions are already caught by the other try ... except block, aren't they?
There was a problem hiding this comment.
You're right! Removed it
The changes were applied to the following superseding PR: #692
| f"{workflow} is now read-only shared with {', '.join(shared_users)}", | ||
| msg_type="success", | ||
| ) | ||
| if share_errors: |
There was a problem hiding this comment.
No need to check if share_errors: ..., as if the array is empty then the for loop will not do anything
There was a problem hiding this comment.
Nicely spotted. Changed
The changes were applied to the following superseding PR: #692
| ) | ||
| if share_errors: | ||
| for error in share_errors: | ||
| display_message(error, msg_type="error") |
There was a problem hiding this comment.
If there are errors (here or before), then reana-client should exit with non-zero exit code
There was a problem hiding this comment.
This would cancel out any subsequent errors. E.g. imagine someone shares with 20 people at a time where some emails have typos; the current implementation would then show all errors after 1 command execution, while the implementation you suggest requires the user to run the command again after each error to find the next error.
We can still change this if you prefer the latter scenario for a reason that I'm not seeing.
There was a problem hiding this comment.
Yeah, it's good that the errors are all shown at the end! What I meant was to add something like
if share_errors:
sys.exit(1)at the end of the command, so that in case of errors reana-client returns an appropriate status code. No need to call sys.exit just after the error happens.
| help="Optional message that is sent to the user(s) with the sharing invitation.", | ||
| ) | ||
| @click.option( | ||
| "-v", |
There was a problem hiding this comment.
The shorthand option -v is normally used for --verbose in other scripts, so I think we should either change it to another letter or even drop the shorthand version. What do you think?
There was a problem hiding this comment.
Changed it to allow for --valid-until only, removed the -v shorthand.
The changes were applied to the following superseding PR: #692
|
Late comment: JSON output is missing, but if you prefer we can tackle this later on |
|
@mdonadoni I can not reply directly to your last comment so I'll do it like this. I created an issue for this so we don't forget about it: #712 |
|
Included in #692 |
Adds a new command to the CLI to share a workflow with a user.
Closes #680