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

Add --remote option to endpoint delete #1538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LeiGlobus
Copy link
Contributor

@LeiGlobus LeiGlobus commented Apr 18, 2024

This adds functionality to delete endpoints when a local config is not present.

It differs from the original story in that this no longer uses t he --remote option, but just as part of normal delete, with the usual --yes and --force options behaving as they should intuitively.

A few notes:

  • Modified get_ep_dir_by_name_or_uuid() to take an extra parameter with default so it doesn't raise if an UUID is specified that does not exist on the system, and also to pass the uuid on as an argument
  • Added a new wrapper name_or_uuid_arg_non_local that takes advantage of the above without affecting the existing name_or_uuid_arg
  • Note that the happy deletion path calls the Globus Compute Client's delete_endpoint() which has the same name but is a direct API call via web_client.delete_endpoint()

@joshbryan-globus
Copy link
Contributor

[sc-31206]

@joshbryan-globus
Copy link
Contributor

[sc-32452]

Copy link
Contributor

@rjmello rjmello left a comment

Choose a reason for hiding this comment

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

This looks good, but I think a simpler, more intuitive path forward is to refactor the --force flag to attempt to delete the endpoint locally and remotely, pushing through exceptions for each operation. If the endpoint cannot be found locally, then we would try to delete it remotely with a provided UUID. If the user does not provide a UUID, we can raise an informative exception that asks them to do so.

Another path might be to drop the --force flag in favor of --local and --remote flags. By default, we would attempt to perform both operations, but if the user specifies one of these flags, then we would only attempt the corresponding operation.

compute_endpoint/globus_compute_endpoint/cli.py Outdated Show resolved Hide resolved
Comment on lines 815 to 819
if not yes:
yes = click.confirm(
f"Are you sure you want to delete endpoint <{ep_dir.name}>?",
abort=True,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably move this to the top of the function and out of the if remote: block so that we always give a warning.

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 agree that it's worthwhile moving the confirmation to the top and changing existing logic a little.

@LeiGlobus
Copy link
Contributor Author

This looks good, but I think a simpler, more intuitive path forward is to refactor the --force flag to attempt to delete the endpoint locally and remotely, pushing through exceptions for each operation. If the endpoint cannot be found locally, then we would try to delete it remotely with a provided UUID. If the user does not provide a UUID, we can raise an informative exception that asks them to do so.

Another path might be to drop the --force flag in favor of --local and --remote flags. By default, we would attempt to perform both operations, but if the user specifies one of these flags, then we would only attempt the corresponding operation.

I did think about doing something similar but went with coding exactly as the story dictated, figuring that to deviate would warrant a scrum/dev thread instead of just changing the behavior between me and the reviewer.

I will summarize the current and other (your) path in a dev thread to gather opinions, potentially to be discussed as a ramp item tomorrow afternoon if it isn't clearly resovled before.

@LeiGlobus LeiGlobus force-pushed the delete-remote-endpoint-sc-31206 branch from e587b6b to 9e019be Compare May 21, 2024 20:45
@LeiGlobus LeiGlobus requested a review from rjmello May 21, 2024 20:46
@LeiGlobus LeiGlobus force-pushed the delete-remote-endpoint-sc-31206 branch 2 times, most recently from 8280ada to 9e6a324 Compare May 21, 2024 21:03
@LeiGlobus LeiGlobus force-pushed the delete-remote-endpoint-sc-31206 branch 3 times, most recently from abfde7d to 46818e4 Compare June 5, 2024 19:40
@LeiGlobus LeiGlobus force-pushed the delete-remote-endpoint-sc-31206 branch from 46818e4 to 49c0a3c Compare June 5, 2024 19:42
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.

None yet

3 participants