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.

Sign up for GitHub

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

Jump to bottom

feat: add support for context manager in client #987

Merged
merged 15 commits into from Oct 4, 2021
Merged

feat: add support for context manager in client #987

merged 15 commits into from Oct 4, 2021

Conversation

atulep
Copy link
Contributor

@atulep atulep commented Sep 7, 2021

Addresses feature request filed in #575.

@atulep atulep requested a review from a team as a code owner September 7, 2021 18:04
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 7, 2021
@software-dov software-dov self-requested a review September 7, 2021 18:10
@atulep atulep linked an issue Sep 7, 2021 that may be closed by this pull request
Feature Request: allow clients to act as context manager and add close() method to cleanup resources #575
Closed
software-dov
software-dov reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

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

Looks good, just some nits around testing.

I think it would be good to have some autogenerated unit tests as well so we can keep generated code coverage at 100%.
Maybe something that injects a dummy transport that can tell us that it was told to enter, something like (strawman expressions in the template, use the correct ones :P )

class DummyTransport:
    def __init__(self):
        self.trigger_state = 0
        
    def __enter__(self):
        self.trigger_state = 1
        
    def __exit__(self, *args, **kwargs):
        self.trigger_state = 2
        
client = {{ api.module_name }}.{{ service.client_name }}(transport=DummyTransport())
assert c.transport.trigger_state == 0
with client:
    assert c.transport.trigger_state == 1
    
assert c.transport.trigger_state == 2

And then associated mox tests for the grpc and rest transport enter/exit methods (I always have to steal mox setup from existing code).

gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 Show resolved Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 Outdated Show resolved Hide resolved
@atulep
Copy link
Contributor Author

atulep commented Sep 13, 2021

Looks like golden files need to be updated. I can't get bazel run :update on my Mac due to some weird error related to wrong version of Python. I'm investigating it now.

software-dov
software-dov reviewed Sep 14, 2021
View reviewed changes
Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

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

Looks good, just a few more obnoxious nitpicking comments :)

gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Show resolved Hide resolved
gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved
gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 Outdated Show resolved Hide resolved
busunkim96
busunkim96 approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this!

CC @tswast @nicain I think both of you were interested in this feature, PTAL if you have a moment.

@ncclementi ncclementi mentioned this pull request Sep 16, 2021
Merged
@atulep
Copy link
Contributor Author

atulep commented Sep 23, 2021

@lidizheng PTAL async client

lidizheng
lidizheng reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

The added context manager method looks good, just one minor comment.

gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 Show resolved Hide resolved
@atulep
Copy link
Contributor Author

atulep commented Sep 24, 2021

@software-dov @tseaver @tswast @busunkim96 PTAL. I've addressed all your comments.

tswast
tswast reviewed Sep 27, 2021
View reviewed changes
gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/base.py.j2 Outdated Show resolved Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/transports/base.py.j2 Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/transports/base.py Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

...on/goldens/credentials/google/iam/credentials_v1/services/iam_credentials/transports/base.py Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

...ation/goldens/logging/google/cloud/logging_v2/services/logging_service_v2/transports/base.py Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

...ation/goldens/logging/google/cloud/logging_v2/services/metrics_service_v2/transports/base.py Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

tests/integration/goldens/redis/google/cloud/redis_v1/services/cloud_redis/transports/base.py Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

...ration/goldens/logging/google/cloud/logging_v2/services/config_service_v2/transports/base.py Outdated
def close(self):
"""Closes resources associated with the transport.

:: warning::
Copy link

Choose a reason for hiding this comment

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

Suggested change
:: warning::
.. warning::

@atulep atulep merged commit 4edabcf into googleapis:master Oct 4, 2021
This was referenced Jan 18, 2022
Merged
Closed
Closed
This was referenced Jan 26, 2022
Closed
Closed
Merged
@parthea parthea mentioned this pull request Oct 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver left review comments

@tswast tswast tswast left review comments

@lidizheng lidizheng lidizheng left review comments

@busunkim96 busunkim96 busunkim96 approved these changes

@software-dov software-dov software-dov approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: allow clients to act as context manager and add close() method to cleanup resources
6 participants
@atulep @tseaver @tswast @software-dov @lidizheng @busunkim96