Conversation
aaschaer
changed the title
|
Marking this ready for review as it now has tests and I think its enough to start adding Transfer v2 support to going forwards. |
aaschaer
requested review from
MaxTueckeGlobus,
ada-globus,
derek-globus,
kurtmckee,
m1yag1 and
sirosen
as code owners
|
|
||
| # CreateTunnelData has been moved to experimental but is provided here for | ||
| # backwards compatibility | ||
| from globus_sdk.experimental.transfer_v2.data import CreateTunnelData |
There was a problem hiding this comment.
I don't like having this dependency arrow, from non-experimental SDK into globus_sdk.experimental.
It makes it harder to know which changes are safe and which are not at review time -- even though the class is located in experimental, any change to it would be subject to the usual level of rigor we apply for our backwards compatibility policy. Even if we decided to make an exemption, it's confusing in terms of "what is allowed?"
I'm trying to decide on a solution to propose. I see two options so far, listed in my current order of preference:
- Depend from
experimentalonto this non-experimental class (since it's not really changing) - Copy the class in two locations, one experimental and one not
I'm sure we could come up with others, but I expect the deprecation cycle for TransferClient to be extremely long, and I'm therefore wary of creating a "temporary situation" which might last much longer than expected. I'd like to be careful here.
|
|
||
| Tunnels functionality is currently in Beta and may experience | ||
| changes that will break this interface | ||
|
|
There was a problem hiding this comment.
This note makes me think that perhaps I misunderstood -- and it has bearing on the comment above. Is the API not actually stabilized? I'd like to clarify that, separately from figuring out what to do about it.
There was a problem hiding this comment.
Responded in slack since that seems like an easier place to have a wider discussion if needed, but yes anything under v2 is not stable
| """This response class is for iterating on JSON:API responses that have an | ||
| array of data in their top level `data` key.""" | ||
|
|
||
| default_iter_key = "data" |
There was a problem hiding this comment.
How would you feel about breaking this response and the paginator off of this PR, into a separate one?
I think we could fairly rapidly review+merge these two parts, with their tests, and that they're unlikely to need changes. That would slim down the PR overall and make it easier to review -- especially since I realize now that I have some high-level questions about how we move away from potentially problematic TransferClient methods.
There was a problem hiding this comment.
Makes sense, will do
2 participants
Adds
TransferClientV2to experimental, adds its supporting classes, and labels existing v2 support in theTransferClientas Beta as proposed here.Most of the code in this PR is things for the existing
TransferClientthat were copied and then slightly modified to work with Transfer v2. Things that are completely new:TransferClientV2.list_stream_access_pointsSome things that aren't included here but would be nice to have at some point:
dict.GlobusHTTPResponsethat are available if the response looks like a JSON:API responseI also found some issues with the existing tunnels support in the
TransferClientwhile copying things over toTransferClientV2, most notably the iteration onlist_tunnelswas looking for a v1 "DATA" key. I've included fixes for now, but I'm a bit worried about supporting those going forwards. We likely will make more changes and fixesTransferClientV2side that might not make it toTransferClient, so maybe we should deprecate those completely rather than leaving them there with a "careful this is Beta" comment?