Conversation
The new client definition is minimal, providing no methods. It only maps collection ID + base URL to existing SDK methods, and defines the scopes appropriately for the client. Based on the solution for `scope` as a class-attribute of clients used in the `SpecificFlowClient`, a stub is defined for the scopes of this client class which raises appropriate attribute errors on access. It thereby makes itself type-safe in contexts where it is used correctly. (Adjusting such that scope based access at the class level is a type error for this class and `SpecificFlowClient` is considered out of scope for this change.) Unit tests for the scope behaviors + a new (small) doc page are included.
| collection_id: str | uuid.UUID, | ||
| base_url: str, |
There was a problem hiding this comment.
Is the relationship between these two parameters one where base url can be discovered consistently from transfer collection search -> endpoint id -> endpoint info route?
But that logic was too complex for us to put into a client constructor?
There was a problem hiding this comment.
I think so. But there may even be cases (I'm not certain) where a user cannot view the collection in Transfer but can interact with it directly for upload/download.
I think we could build an alternate constructor or factory that uses a TransferClient in the future, but I'd want to discuss it with the Transfer/GCS devs.
There was a problem hiding this comment.
Okay, I suppose I'm fine leaving that undecided since this is going into experimental, but I will want to make sure we revisit before pulling into the main repo.
Separate question: Could we use a different term from base_url (or at least update the documentation on it to be more collection domain specific).
In the GCSClient we use:
:param gcs_address: The FQDN (DNS name) or HTTPS URL for the GCS Manager API.
The person constructing this client will likely a few different urls to choose from when creating this class. Ideally we would answer that question by the parameter name, but if not a clear explanation in the docstring would go a long way.
There was a problem hiding this comment.
Okay, I suppose I'm fine leaving that undecided since this is going into experimental, but I will want to make sure we revisit before pulling into the main repo.
Just to be clear -- I don't think this discussion blocks us so maybe not a big deal -- I don't consider this something "undecided".
This client should not require that you use TransferClient in order to instantiate it. I'm fine with adding a feature (in this class or outside of it) which does precisely that, but the __init__ for this class should not change. Changing this definition in order to support such a behavior would be a strong warning sign that we've accidentally broken the orthogonality of components.
We'll end up revisiting, I'm sure, when it's time to promote this out of experimental, so we can leave the debate for later. 😁
Regarding the parameter name...
In the Collection and Endpoint doucments documentation, the relevant field is named https_server.
To me, that feels just slightly wrong as a name in the SDK context.
The gcs_address field most closely matches gcs_manager_url, so we're already a little mismatched.
I'm going to push a change which calls this collection_address, and documents it as "as might be retrieved from the https_server field in Globus Transfer".
Co-authored-by: derek-globus <113056046+derek-globus@users.noreply.github.com>
Use `collection_address` to match `GCSClient`.
The new client definition is minimal, providing no methods. It only maps
collection ID + base URL to existing SDK methods, and defines the scopes
appropriately for the client.
Based on the solution for
scopeas a class-attribute of clients used inthe
SpecificFlowClient, a stub is defined for the scopes of this clientclass which raises appropriate attribute errors on access.
It thereby makes itself type-safe in contexts where it is used correctly.
(Adjusting such that scope based access at the class level is a type
error for this class and
SpecificFlowClientis considered out of scopefor this change.)
Unit tests for the scope behaviors + a new (small) doc page are included.