Restructure command tree / refactor repo layout #119
Conversation
Does the initial and major, noisy step of moving things into a `commands` subpackage. We discussed that this wasn't necessary, but as top-level modules are about to multiply significantly it feels like a valuable first step. Only two commands are actually changing locations in this commit: `get-identities` and `ls` as proofs of concept. Everything else remains as it was.
Everything in globus_cli.services.transfer moves to one of three locations: Most move to globus_cli.commands Everything in the transfer.helpers moves to a new module at `globus_cli.services.transfer`, some other functionality moves into that module. Line-by-line shlex processing moves to globus_cli.parsing On that last note, fixes globus#103 by inspecting exit status on caught SystemExits. Because this does the dramatic refactor, it's also fair to say this resolves globus#71
This is a rename that we agreed upon with the new restructuring.
Renamed to `endpoint is-activated`. New implmenetation uses check_activation_requirements, rather than the autoactivate call, and can check the lifetime of activation for an endpoint. Resolves globus#42
sirosen
added
enhancement
Command names are changed, so travis must change, obviously.
|
|
||
| # autoactivation always means success, so short-circuit out of here | ||
| if res.supports_auto_activation: | ||
| success('{} does not require activation') |
There was a problem hiding this comment.
I don't think that this part is correct.
Using an endpoint that definitely requires activation, is-activated tells me it doesn't require activation:
~$ globus endpoint show 44e6ddac-e818-11e5-97d5-22000b9da45e
Display Name: XSEDE Gordon
ID: 44e6ddac-e818-11e5-97d5-22000b9da45e
Owner: xsede@globusid.org
Activated: False
Shareable: True
~$ globus endpoint is-activated 44e6ddac-e818-11e5-97d5-22000b9da45e
44e6ddac-e818-11e5-97d5-22000b9da45e does not require activation
For what it's worth: the webapp decides whether or not to display the "Activate" tab in Manage Endpoints based on whether the endpoint document has an expires_in of -1 on the endpoint document. This same value also appears to be available in the activation requirements document.
There was a problem hiding this comment.
Why is "auto_activation_supported": True on this endpoint? Do we know?
If an endpoint like go#ep1 is used, I don't want to be telling people, incorrectly, that they need to go and activate it manually. "expires_in": -1 will only be set if they've already activated it, which isn't right either.
Does supporting autoactivation mean something other than what I think it means?
There was a problem hiding this comment.
Why is "auto_activation_supported": True on this endpoint? Do we know?
A quick glance at transfer tells me that (almost?) all endpoint are going to come back with auto_activation_supported: true--the criteria is: shared endpoints, gcp endpoints, S3 endpoints, any endpoint with a myproxy server defined, any endpoint with an oauth server defined.
There was a problem hiding this comment.
Dave and I were just discussing this, actually. I'm going to switch this to look at expires_in=-1, per his suggestion. I also want a property on the SDK response that encodes that check in the future.
Apparently there is a wider class of endpoints which support autoactivate than I realized, but they don't guarantee that autoactivation will succeed, which seems to be the key difference.
There was a problem hiding this comment.
Yes, I think "ok, I'll try and get a valid credential from Nexus" is a legitimate auto-activation operation, so that's why it always comes back as supported.
Supporting autoactivation is actually insufficient -- many endpoints support autoactivation but do not have the "eternally active" status that we're looking for. Turns out that expires_in=-1 actually does mean this, even if an endpoint hasn't been autoactivated yet.
|
I changed the behavior per our discussion, and opened an SDK issue to add the property that I think should be available there. |
| 'with status 1')) | ||
| @click.option('--absolute-time', is_flag=True, | ||
| show_default=True, default=False, | ||
| help=('Treat the value of --until as a POSIX timestamp, not a ' |
There was a problem hiding this comment.
Feel free to ignore this, but it might be nice to phrase this as "POSIX timestamp (Unix time)" or "POSIX timestamp (Epoch time)" so that people know what you're talking about and don't get it confused with ISO 8601.
Adds note that the time is num seconds since epoch to the helptext.
The layout of modules, since they don't have to reflect the CLI surface / usage, is largely a matter of taste. However, making them reflect that structure was the only sensible option I could work out.
So, in brief:
globus_cli.commandsglobus_cli.servicesendpoint acltoendpoint permission, per discussionsendpoint autoactivatetoendpoint is-activatedwith a new implementation to match those semanticsThe one bit of weirdness is that I haven't come up with a better name for all of the
commandsmodules for building chunks of the tree, so we end up with ugliness likeglobus_cli.commands.bookmark.commands, which is not great naming-wise.Closes #71, #103, #42