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

GCS get and update endpoint #933

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Jan 2, 2024

sc-28586

What?

Added 2 new operations to the GCSClient class:

  • gcs_client.get_endpoint()
  • gcs_client.update_endpoint(EndpointDocument(...))

Testing

  • Updated an endpoint in sandbox environment.
  • Unit tested with _testing patched responses.
  • Tested nulling an endpoint subscription on a sandbox gcs endpoint:
    gcs.update_endpoint(EndpointDocument(subscription_id=None))

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

src/globus_sdk/utils.py Outdated Show resolved Hide resolved
@sirosen
Copy link
Member

sirosen commented Jan 29, 2024

I've offered an alternative (PR on this PR) here: derek-globus#1

I'd like us to discuss this more if we don't want to go down that path.

- Get rid of `_MISSING_SENTINEL`, revert the old helpers to
  `None`-only, but don't use them for this class
- Assign values directly, without worrying about applying converters
  in almost any cases
- In the one case in which a converter is needed (`keywords`), apply
  it explicitly inline
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.

I spotted one last, minor thing which I'd like us to improve before approving+merging.

src/globus_sdk/services/gcs/client.py Outdated Show resolved Hide resolved
@derek-globus derek-globus merged commit 3a8a87e into globus:main Feb 5, 2024
16 checks passed
@derek-globus derek-globus deleted the gcs-endpoint-update-sc-28586 branch February 5, 2024 21:02
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.

2 participants