add logout to funcx endpoint

[LeiGlobus]

Added logout subcommand to funcx-endpoint. See linked SC: https://app.shortcut.com/funcx/story/18074/support-funcx-endpoint-logout

There is a slight modification to funcx_sdk.funcx.sdk.login_manager.manager.logout to return bool instead, plus see the associated TODO comment.

I expect to remove the TODO in LoginManager.logout and funcx_endpoint.cli.logout_endpoint before merging (the TODOs are to ensure PR comment)

EDIT all the TODOs were removed along with most of the proposed (and rejected) extra logic/params.

[shortcut-integration]

This pull request has been linked to Shortcut Story #18074: Support FuncX endpoint logout.

[LeiGlobus]

[LeiGlobus]

See above screenshot for example usage - First funcx_endpoint start forces login (no previous logins). Second 'start' does not require re-auth as expected, after 'logout', 3rd start requires re-auth (confirming logout token clearing), and if login is aborted, 2nd 'logout' shows that no tokens were found to revoke

[LeiGlobus]

@sirosen LoginManager.logout was your original code, putting this on your radar in case you had any comments on whether we should support resource_server specific logouts or if you mind the bool return value of if tokens were found and removed.

[khk-globus]

Looks generally on target, save for a suggestion regarding giving more information to the user.

The one thing that does appear to be lacking is tests.

[khk-globus]

Is this ... expected behavior?

$ funcx-endpoint logout
1661971619.692372 2022-08-31 14:46:59 WARNING MainProcess-1517847 MainThread-139622829592576 funcx_endpoint.cli:193 logout_endpoint No tokens were found to revoke during logout attempt

$

As I didn't specify an environment ... what do we expect to happen? From which environment did it attempt log me out?

Perhaps --environment should be required? Or should there be a single required positional argument?

[sirosen]

Kevin asked me for my thoughts on this, so very briefly:

• Consider whether or not --environment should have hidden=True set if it's going to be an option (also consider: can it just be an env var). Users generally don't know and don't care about various non-production environments, so avoid cluttering --help.
• Similarly, users don't know and should not need to care what a Resource Server is. Don't show them information which presumes that they care about the implementation at the same level that the implementers do. If you want such information, you can always add it (later) as a --verbose option.
• In globus-cli, we introduced an --ignore-errors option to handle the fact that a user might want to force logout OR they might want to abort if credential revocation fails. That may or may not be a good paradigm to follow here as well.
• I like that this makes token revocation a responsibility of the LoginManager. globus-cli could do with that refinement too -- it's clearer than making the logout command do it.

[joshbryan-globus]

I left a few, let me know if you have any questions.

[LeiGlobus]

• Consider whether or not --environment should have hidden=True set if it's going to be an option (also consider: can it just be an env var). Users generally don't know and don't care about various non-production environments, so avoid cluttering --help.
  Yes users generally don't care about dev/production environments, this is mostly for developer use. For us, it isn't that big a deal to clear all env auths instead of adding this env option.

So with Josh's suggestion below, I'll remove the environment param altogether for now.

• Similarly, users don't know and should not need to care what a Resource Server is. Don't show them information which presumes that they care about the implementation at the same level that the implementers do. If you want such information, you can always add it (later) as a --verbose option.

Yes the resource_server ones are entwined anyway for most consents and most users don't care about individual ones. Especially in this case, removing individual ones aren't relevant to the original shortcut story. Removing references and logic here.

• In globus-cli, we introduced an --ignore-errors option to handle the fact that a user might want to force logout OR they might want to abort if credential revocation fails. That may or may not be a good paradigm to follow here as well.

Adding a --force option re: Josh currently running endpoints which will require a bit more refactoring in the next commit.

[LeiGlobus]

Is this ... expected behavior?

$ funcx-endpoint logout
1661971619.692372 2022-08-31 14:46:59 WARNING MainProcess-1517847 MainThread-139622829592576 funcx_endpoint.cli:193 logout_endpoint No tokens were found to revoke during logout attempt

$

As I didn't specify an environment ... what do we expect to happen? From which environment did it attempt log me out?

Perhaps --environment should be required? Or should there be a single required positional argument?

See other comments - removed --environment as I mostly agree with sirosen and Josh that it is too dev-specific, but did change the resulting user-facing output

[LeiGlobus]

Looks generally on target, save for a suggestion regarding giving more information to the user.

The one thing that does appear to be lacking is tests.

Let's discuss offline, but as logout_endpoint just calls LoginManager.logout directly, there isn't much PR specific logic to test. (most of the logic is Click option related, the rest LoginManager related, so should be added in separate PR if desired IMO). The get_running_endpoints() refactor is slightly testable but not much.

[LeiGlobus]

Added logout subcommand to funcx-endpoint. See linked SC: https://app.shortcut.com/funcx/story/18074/support-funcx-endpoint-logout

There is a slight modification to funcx_sdk.funcx.sdk.login_manager.manager.logout to return bool instead, plus see the associated TODO comment.

I expect to remove the TODO in LoginManager.logout and funcx_endpoint.cli.logout_endpoint before merging (the TODOs are to ensure PR comment)

edit

[LeiGlobus]

Added sys.exit() non-

zero behavior to logout, if any tokens were actually found/revoked. See attached screenshot example (which doesn't show re-auth scenarios which I will leave to the reviewer to test if desired, which should also work)

[LeiGlobus]

Redid the logout messages (in log.info and in ClickException command return). See all 4 scenarios below: (In order, they are 1) Logout failure with running endpoint 2) Logout success with running endpoint and --force 3) logout success with no running endpoints 4) logout failure when tokens were already cleared

[khk-globus]

Well done, mate! That was the "5-minute" PR from hell, right? Heh.

[LeiGlobus]

@joshbryan-globus I think the --force flag resolves the comment? But it's still showing changes requested. You may have to click something again