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

transfer client local_user support #736

Merged
merged 5 commits into from
Jun 5, 2023
Merged

transfer client local_user support #736

merged 5 commits into from
Jun 5, 2023

Conversation

aaschaer
Copy link
Contributor

@aaschaer aaschaer commented May 30, 2023

Adds local user selection support to transfer, delete, ls, mkdir, and rename, and _testing support for mkdir and rename

SDK/CLI story: https://app.shortcut.com/globus/story/20504
Corresponding Transfer docs PR: https://github.com/globusonline/docs.globus.org/pull/1322

Opening as draft for a few reasons:

  1. Transfer support is not live yet
  2. Due to some bad session error behavior in some cases its not clear how loudly the Transfer support will be announced yet, and I'm not sure if/how that will effect the SDK/CLI
  3. I have not yet written a changelog fragment due to reason 2 so this should fail checks

📚 Documentation preview 📚: https://globus-sdk-python--736.org.readthedocs.build/en/736/

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

The feature looks pretty straightforward from the client side, the Transfer side of it and complexity around it notwithstanding.

If we expect this to take a while, it might be nice to get your new mkdir and rename tests into main with local_user removed. I'd fast-track review for that if you want to do the work that way.

I think we could have a very minimal changelog for the SDK which would be suitable no matter what happens. Something like

Several TransferClient methods, TransferData, and DeleteData now support the local_user, source_local_user, and destination_local_user parameters

But if this is still being discussed, we don't need to rush it.

src/globus_sdk/services/transfer/client.py Outdated Show resolved Hide resolved
@aaschaer aaschaer marked this pull request as draft May 31, 2023 00:55
Co-authored-by: Stephen Rosen <sirosen@globus.org>
@kurtmckee kurtmckee added no-news-is-good-news This change does not require a news file and removed no-news-is-good-news This change does not require a news file labels May 31, 2023
@aaschaer aaschaer marked this pull request as ready for review June 2, 2023 20:17
@aaschaer
Copy link
Contributor Author

aaschaer commented Jun 2, 2023

This should be good to go now.

We still aren't loudly advertising this feature while better Transfer/GCS errors are in progress, but SDK support with a minimal changelog should be fine. We might want to make the CLI options hidden since that's aimed at a broader audience that might need better errors more, but that's a future PR.

I also added a change to TransferRequestsTransport to not retry on Transfer EndpointErrors that came up while testing this. I don't know of any transient EndpointErrors and I can report its not a great experience to try a local user option in the CLI and then wait several seconds to learn your GCS version isn't high enough

@sirosen
Copy link
Member

sirosen commented Jun 5, 2023

I also added a change to TransferRequestsTransport to not retry on Transfer EndpointErrors that came up while testing this. I don't know of any transient EndpointErrors and I can report its not a great experience to try a local user option in the CLI and then wait several seconds to learn your GCS version isn't high enough

This makes sense to me, and 👍 for including the test and changelog for it.

I'm going ahead with the merge. I want to do a quick roundup of other outstanding issues and we can maybe get this out in short order.

@sirosen sirosen merged commit 11205d0 into main Jun 5, 2023
@sirosen sirosen deleted the aaron/local_user branch June 5, 2023 14:56
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.

3 participants