-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Added Response Headers to Control Plane Operations #41742
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
Added Response Headers to Control Plane Operations #41742
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds support for retrieving response headers for control plane operations (databases and containers) on the Cosmos service, for both synchronous and asynchronous clients.
- Introduces new test cases to verify that response headers are returned for various operations.
- Adds a new get_response_headers method and initializes _response_headers in the Database and Container classes (including their async counterparts).
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
sdk/cosmos/azure-cosmos/tests/test_cosmos_responses_async.py | Added async tests for response headers on database and container operations. |
sdk/cosmos/azure-cosmos/tests/test_cosmos_responses.py | Added sync tests for response headers on database and container operations. |
sdk/cosmos/azure-cosmos/azure/cosmos/database.py | Added _response_headers initialization and get_response_headers method. |
sdk/cosmos/azure-cosmos/azure/cosmos/container.py | Added _response_headers initialization and get_response_headers method. |
sdk/cosmos/azure-cosmos/azure/cosmos/aio/_database.py | Added _response_headers initialization and get_response_headers method for async client. |
sdk/cosmos/azure-cosmos/azure/cosmos/aio/_container.py | Added _response_headers initialization and get_response_headers method for async client. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some optimizations and missing functions. I only commented on async codes, but the same comment applies to sync codes. Please address them in both async/sync codes.
I believe if you follow my suggeestions, you won't need @overload
or return_headers
anymore.
Based on the changes, the tests should be updated accordingly.
Also, you can run pylint
and mypy
tests locally. Please let me know if you need help with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides some small comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @andrewmathew1 can we please add samples as well for them if not present already, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides few small comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small things here and there but looking good - please ensure Anna signs off on the overloading logic
Co-authored-by: Tomas Varon <[email protected]>
Co-authored-by: Tomas Varon <[email protected]>
sdk/cosmos/azure-cosmos/azure/cosmos/_cosmos_client_connection.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Anna Tisch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you!
/azp run python - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
This PR introduces a way to get response headers for given control plane operations on containers and databases. This is availble for both the async and sync client. Similar to #35791 for response headers at the item level.