-
Notifications
You must be signed in to change notification settings - Fork 46
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
cli: implement share-add command #683
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,12 +15,8 @@ | |||||||||||||
import traceback | ||||||||||||||
|
||||||||||||||
import click | ||||||||||||||
from jsonschema.exceptions import ValidationError | ||||||||||||||
from reana_commons.config import INTERACTIVE_SESSION_TYPES, REANA_COMPUTE_BACKENDS | ||||||||||||||
from reana_commons.errors import REANAValidationError | ||||||||||||||
from reana_commons.validation.operational_options import validate_operational_options | ||||||||||||||
import yaml | ||||||||||||||
|
||||||||||||||
from jsonschema.exceptions import ValidationError | ||||||||||||||
from reana_client.cli.files import get_files, upload_files | ||||||||||||||
from reana_client.cli.utils import ( | ||||||||||||||
add_access_token_options, | ||||||||||||||
|
@@ -51,6 +47,9 @@ | |||||||||||||
validate_input_parameters, | ||||||||||||||
validate_workflow_name_parameter, | ||||||||||||||
) | ||||||||||||||
from reana_commons.config import INTERACTIVE_SESSION_TYPES, REANA_COMPUTE_BACKENDS | ||||||||||||||
from reana_commons.errors import REANAValidationError | ||||||||||||||
from reana_commons.validation.operational_options import validate_operational_options | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
@click.group(help="Workflow management commands") | ||||||||||||||
|
@@ -67,6 +66,13 @@ def workflow_execution_group(ctx): | |||||||||||||
logging.debug(ctx.info_name) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
@click.group(help="Workflow sharing commands") | ||||||||||||||
@click.pass_context | ||||||||||||||
def workflow_sharing_group(ctx): | ||||||||||||||
"""Top level wrapper for workflow sharing.""" | ||||||||||||||
logging.debug(ctx.info_name) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
@workflow_management_group.command("list") | ||||||||||||||
@click.option( | ||||||||||||||
"-w", | ||||||||||||||
|
@@ -451,12 +457,12 @@ def workflow_start( | |||||||||||||
\t $ reana-client start -w myanalysis.42 -p sleeptime=10 -p myparam=4\n | ||||||||||||||
\t $ reana-client start -w myanalysis.42 -p myparam1=myvalue1 -o CACHE=off | ||||||||||||||
""" | ||||||||||||||
from reana_client.utils import get_api_url | ||||||||||||||
from reana_client.api.client import ( | ||||||||||||||
get_workflow_parameters, | ||||||||||||||
get_workflow_status, | ||||||||||||||
start_workflow, | ||||||||||||||
) | ||||||||||||||
from reana_client.utils import get_api_url | ||||||||||||||
|
||||||||||||||
def display_status(workflow: str, current_status: str): | ||||||||||||||
"""Display the current status of the workflow.""" | ||||||||||||||
|
@@ -586,12 +592,12 @@ def workflow_restart( | |||||||||||||
\t $ reana-client restart -w myanalysis.42 -o TARGET=gendata\n | ||||||||||||||
\t $ reana-client restart -w myanalysis.42 -o FROM=fitdata | ||||||||||||||
""" | ||||||||||||||
from reana_client.utils import get_api_url | ||||||||||||||
from reana_client.api.client import ( | ||||||||||||||
get_workflow_parameters, | ||||||||||||||
get_workflow_status, | ||||||||||||||
start_workflow, | ||||||||||||||
) | ||||||||||||||
from reana_client.utils import get_api_url | ||||||||||||||
|
||||||||||||||
logging.debug("command: {}".format(ctx.command_path.replace(" ", "."))) | ||||||||||||||
for p in ctx.params: | ||||||||||||||
|
@@ -1373,7 +1379,7 @@ def workflow_open_interactive_session( | |||||||||||||
Examples:\n | ||||||||||||||
\t $ reana-client open -w myanalysis.42 jupyter | ||||||||||||||
""" | ||||||||||||||
from reana_client.api.client import open_interactive_session, info | ||||||||||||||
from reana_client.api.client import info, open_interactive_session | ||||||||||||||
|
||||||||||||||
if workflow: | ||||||||||||||
try: | ||||||||||||||
|
@@ -1458,3 +1464,84 @@ def workflow_close_interactive_session(workflow, access_token): # noqa: D301 | |||||||||||||
sys.exit(1) | ||||||||||||||
else: | ||||||||||||||
display_message("Cannot find workflow {} ".format(workflow), msg_type="error") | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
@workflow_sharing_group.command("share-add") | ||||||||||||||
@check_connection | ||||||||||||||
@add_workflow_option | ||||||||||||||
@add_access_token_options | ||||||||||||||
@click.option( | ||||||||||||||
"-u", | ||||||||||||||
"--user", | ||||||||||||||
"users", | ||||||||||||||
multiple=True, | ||||||||||||||
help="Users to share the workflow with.", | ||||||||||||||
required=True, | ||||||||||||||
) | ||||||||||||||
@click.option( | ||||||||||||||
"-m", | ||||||||||||||
"--message", | ||||||||||||||
help="Optional message that is sent to the user(s) with the sharing invitation.", | ||||||||||||||
) | ||||||||||||||
@click.option( | ||||||||||||||
"-v", | ||||||||||||||
"--valid-until", | ||||||||||||||
help="Optional date when access to the workflow will expire for the given user(s) (format: YYYY-MM-DD).", | ||||||||||||||
) | ||||||||||||||
Comment on lines
+1486
to
+1490
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. Added. The changes were applied to the following superseding PR: #692 |
||||||||||||||
@click.pass_context | ||||||||||||||
def share_workflow_add( | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done The changes were applied to the following superseding PR: #692 |
||||||||||||||
ctx, workflow, access_token, users, message, valid_until | ||||||||||||||
): # noqa D412 | ||||||||||||||
"""Share a workflow with other users (read-only). | ||||||||||||||
|
||||||||||||||
The `share-add` command allows sharing a workflow with other users. The | ||||||||||||||
users will be able to view the workflow but not modify it. | ||||||||||||||
|
||||||||||||||
Examples: | ||||||||||||||
|
||||||||||||||
$ 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 | ||||||||||||||
Comment on lines
+1502
to
+1504
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
To show the example commands indented when calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done The changes were applied to the following superseding PR: #692 |
||||||||||||||
""" | ||||||||||||||
from reana_client.api.client import share_workflow | ||||||||||||||
|
||||||||||||||
share_errors = [] | ||||||||||||||
shared_users = [] | ||||||||||||||
|
||||||||||||||
if workflow: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nicely spotted. Changed The changes were applied to the following superseding PR: #692 |
||||||||||||||
try: | ||||||||||||||
for user in users: | ||||||||||||||
try: | ||||||||||||||
logging.info(f"Sharing workflow {workflow} with user {user}") | ||||||||||||||
share_workflow( | ||||||||||||||
workflow, | ||||||||||||||
user, | ||||||||||||||
access_token, | ||||||||||||||
message=message, | ||||||||||||||
valid_until=valid_until, | ||||||||||||||
) | ||||||||||||||
shared_users.append(user) | ||||||||||||||
except Exception as e: | ||||||||||||||
share_errors.append( | ||||||||||||||
f"Failed to share {workflow} with {user}: {str(e)}" | ||||||||||||||
) | ||||||||||||||
logging.debug(traceback.format_exc()) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: You can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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", | ||||||||||||||
) | ||||||||||||||
Comment on lines
+1529
to
+1535
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In which cases can we end up here? All the exceptions are already caught by the other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right! Removed it The changes were applied to the following superseding PR: #692 |
||||||||||||||
|
||||||||||||||
if shared_users: | ||||||||||||||
display_message( | ||||||||||||||
f"{workflow} is now read-only shared with {', '.join(shared_users)}", | ||||||||||||||
msg_type="success", | ||||||||||||||
) | ||||||||||||||
if share_errors: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nicely spotted. Changed The changes were applied to the following superseding PR: #692 |
||||||||||||||
for error in share_errors: | ||||||||||||||
display_message(error, msg_type="error") | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are errors (here or before), then reana-client should exit with non-zero exit code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed 👍 |
||||||||||||||
|
||||||||||||||
else: | ||||||||||||||
display_message(f"Cannot find workflow {workflow}", msg_type="error") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
# -*- coding: utf-8 -*- | ||
# | ||
# This file is part of REANA. | ||
# Copyright (C) 2018, 2019, 2020, 2021, 2022 CERN. | ||
# Copyright (C) 2018, 2019, 2020, 2021, 2022, 2023 CERN. | ||
# | ||
# REANA is free software; you can redistribute it and/or modify it | ||
# under the terms of the MIT License; see LICENSE file for more details. | ||
|
@@ -17,12 +17,11 @@ | |
from click.testing import CliRunner | ||
from mock import Mock, patch | ||
from pytest_reana.test_utils import make_mock_api_client | ||
from reana_commons.config import INTERACTIVE_SESSION_TYPES | ||
|
||
from reana_client.api.client import create_workflow_from_json | ||
from reana_client.config import RUN_STATUSES | ||
from reana_client.cli import cli | ||
from reana_client.config import RUN_STATUSES | ||
from reana_client.utils import get_workflow_status_change_msg | ||
from reana_commons.config import INTERACTIVE_SESSION_TYPES | ||
|
||
|
||
def test_workflows_server_not_connected(): | ||
|
@@ -988,3 +987,42 @@ def test_yml_ext_specification(create_yaml_workflow_schema): | |
result = runner.invoke(cli, ["validate", "-t", reana_token]) | ||
assert result.exit_code != 0 | ||
assert message in result.output | ||
|
||
|
||
def test_share_add_workflow(): | ||
"""Test share-add workflows.""" | ||
status_code = 200 | ||
response = { | ||
"message": "is now read-only shared with", | ||
"workflow_id": "string", | ||
"workflow_name": "string", | ||
} | ||
env = {"REANA_SERVER_URL": "localhost"} | ||
mock_http_response, mock_response = Mock(), Mock() | ||
mock_http_response.status_code = status_code | ||
mock_response = response | ||
reana_token = "000000" | ||
runner = CliRunner(env=env) | ||
with runner.isolation(): | ||
with patch( | ||
"reana_client.api.client.current_rs_api_client", | ||
make_mock_api_client("reana-server")(mock_response, mock_http_response), | ||
): | ||
result = runner.invoke( | ||
cli, | ||
[ | ||
"share-add", | ||
"-t", | ||
reana_token, | ||
"--workflow", | ||
"test-workflow.1", | ||
"--user", | ||
"[email protected]", | ||
"--message", | ||
"Test message", | ||
"--valid-until", | ||
"2024-01-01", | ||
], | ||
) | ||
assert result.exit_code == 0 | ||
assert response["message"] in result.output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to allow for
--valid-until
only, removed the-v
shorthand.The changes were applied to the following superseding PR: #692