-
Notifications
You must be signed in to change notification settings - Fork 926
Add missing endpoints (/compatibility, /mode) to SchemaRegistryClient #2024
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
Conversation
🎉 All Contributor License Agreements have been signed. Ready to merge. |
ddf1826
to
dc8d820
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/confluent_kafka/schema_registry/_async/schema_registry_client.py
Outdated
Show resolved
Hide resolved
) | ||
return response['is_compatible'] # TODO: should it return entire response (including error messages)? |
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.
Maybe return a tuple [bool, Response] so it's easy to have a simple response vs a parsing the complex object? But I think this should probably return the full response by itself with a reference in the docstring to the is_compatible
as a likely attribute for consumption.
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.
My concern is this would be a breaking change, although in this case I think it might be necessary to just make the change (or add a separate method and mark this one as @deprecated): not sure how customers usually interact with test_compatibility, but intuitively I think the "messages" field for test_compatibility is important to include
cc @rayokota if you have any thoughts on this :)
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.
We shouldn't make any breaking changes, let's keep this as-is for now
`GET Global Mode API Reference <https://docs.confluent.io/current/schema-registry/develop/api.html#get--mode>`_ | ||
""" # noqa: E501 | ||
result = self._rest_client.get('mode') | ||
return result['mode'] |
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.
Consider a debug log here with the result if you're not returning the full payload so the contents aren't lost (or maybe generally have a logging debug level option for all rest requests made to SR service)
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.
In this case, mode
is the only field in the JSON returned from this API
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
7a24c92
to
9b468a7
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 PR adds missing endpoints for /compatibility
and /mode
operations to the Schema Registry Client, expanding the client's API coverage to match the complete Schema Registry REST API.
Key changes include:
- Added new mode management endpoints for global and subject-level mode operations
- Enhanced compatibility testing with new all-versions endpoint and query parameters
- Added configuration deletion endpoint and contexts listing endpoint
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
File | Description |
---|---|
tests/schema_registry/conftest.py | Updated mock server to support new endpoints with proper regex patterns and callback handlers |
tests/schema_registry/_sync/test_api_client.py | Added comprehensive test coverage for all new sync client methods |
tests/schema_registry/_async/test_api_client.py | Added comprehensive test coverage for all new async client methods |
src/confluent_kafka/schema_registry/_sync/schema_registry_client.py | Implemented new endpoint methods and enhanced existing compatibility testing |
src/confluent_kafka/schema_registry/_async/schema_registry_client.py | Implemented new endpoint methods and enhanced existing compatibility testing |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/confluent_kafka/schema_registry/_sync/schema_registry_client.py
Outdated
Show resolved
Hide resolved
src/confluent_kafka/schema_registry/_async/schema_registry_client.py
Outdated
Show resolved
Hide resolved
src/confluent_kafka/schema_registry/_async/schema_registry_client.py
Outdated
Show resolved
Hide resolved
src/confluent_kafka/schema_registry/_async/schema_registry_client.py
Outdated
Show resolved
Hide resolved
src/confluent_kafka/schema_registry/_async/schema_registry_client.py
Outdated
Show resolved
Hide resolved
src/confluent_kafka/schema_registry/_async/schema_registry_client.py
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
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 @fangnx , LGTM
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.
Might need a merge/rebase from master
What
Add the missing endpoints and update the existing ones to SR client, for /compatibility and /mode endpoints.
Checklist
References
JIRA: https://confluentinc.atlassian.net/browse/DGS-21591
Test & Review
Open questions / Follow-ups