Azure webpubsub service initial client library version #18199
Conversation
Update the sign algo to generate the expected jwt
| ClientType = TypeVar('ClientType', bound='WebPubSubServiceClient') | ||
|
|
||
|
|
||
| def build_authentication_token(endpoint, hub, key, **kwargs): |
There was a problem hiding this comment.
We also need a from_connection_string version of this function.
Do you have any comments on this?
* upgrade rest * read response before returning
|
/azp run prepare-pipelines |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| return self.__class__.ZERO | ||
|
|
||
|
|
||
| UTC = _UTC_TZ() |
There was a problem hiding this comment.
We should probably support the stdlib type where available:
try:
from datetime import timezone
UTC = timezone.utc
except ImportError:
UTC = _UTC_TZ()
|
|
||
| def _format_url(self, url): | ||
| # type: (str) -> str | ||
| assert self.endpoint[-1] != "/", "My endpoint should not have a trailing slash" |
There was a problem hiding this comment.
Would be kind of strange to encounter an AssertError when constructing the client?
There was a problem hiding this comment.
In that scenario, this is truly an assert (we are stripping the trailing slash before calling the method). However, when looking at the code, the endpoint is a read/write attribute (which it probably shouldn't be) and the user could have set it to a value that does not conform to our assumptions. I may want to change self.endpoint to a read-only property...
| if not self._internal_request.headers.get("Content-Length"): | ||
| try: | ||
| # set content length header if possible | ||
| self._internal_request.headers["Content-Length"] = str(len(self._internal_request.data)) # type: ignore |
There was a problem hiding this comment.
Is this going to be reliable?
| class AsyncHttpResponse(_HttpResponseBase): | ||
|
|
||
| @property | ||
| def content(self) -> bytes: |
There was a problem hiding this comment.
Seems like this property could live on _HttpResponseBase?
There was a problem hiding this comment.
yep good point. it used to be different across sync and async, but no longer needed. Not going to update here though (it's fine to keep it like this), but will update in my core pr. thanks @annatisch !
| await asyncio.sleep(0) | ||
|
|
||
| async def __aexit__(self, *args) -> None: | ||
| await self._internal_response.internal_response.__aexit__(*args) |
There was a problem hiding this comment.
Should we also be calling self.close() here?
There was a problem hiding this comment.
that's a good point, let me call close instead to handle stream closing. Not sure if we want to update this in this pr @johanste , but will udpate in my core pr
|
|
||
| - Python 2.7, or 3.6 or later is required to use this package. | ||
| - You need an [Azure subscription][azure_sub], and a [Azure WebPubSub service instance][webpubsubservice_docs] to use this package. | ||
| - An existing Azure Web PubSub service instance. |
There was a problem hiding this comment.
This seems like a duplicate of the line above.
johanste
requested review from
danieljurek,
mitchdenny,
scbedd and
weshaggard
as code owners
| [webpubsubservice_docs]: https://aka.ms/awps/doc | ||
| [azure_cli]: https://docs.microsoft.com/cli/azure | ||
| [azure_sub]: https://azure.microsoft.com/free/ | ||
| [webpubsubservice_client_class]: https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/signalr/azure-messaging-webpubsubservice/azure/messaging/webpubsubservice/__init__.py |
There was a problem hiding this comment.
move the azure-messaging-webpubsubservice into a new webpubsub folder instead of using signalr?
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
| include azure/messaging/__init__.py | ||
| include LICENSE.txt | ||
| recursive-include tests *.py | ||
| recursive-include examples *.py *.md |
There was a problem hiding this comment.
We usually use folder name samples
|
|
||
| token = six.ensure_str(jwt.encode(payload, key, algorithm="HS256")) | ||
| return { | ||
| "baseUrl": client_url, |
There was a problem hiding this comment.
Is the casing here a particular convention for this value?
| 'build_send_to_all_request', | ||
| 'build_send_to_connection_request', | ||
| 'build_send_to_group_request', | ||
| 'build_send_to_user_request' |
There was a problem hiding this comment.
I'm guessing there are no operations groups for this API?
I saw there's been a shift to include operation groups in the namespace rather than the function name - but it seems that change has no impact on this SDK?
There was a problem hiding this comment.
@annatisch @johanste has already done some manual-manipulation of the code, we completely got rid of the operation groups
No description provided.