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

[Service Bus] Deprecate Internal _Accesstoken #14450

Conversation

egelfand
Copy link
Contributor

Replaced references to internal _Accesstoken with the Accesstoken itself, now that its available. Closes #13638.

@ghost ghost added Service Bus customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Oct 13, 2020
@ghost
Copy link

ghost commented Oct 13, 2020

Thank you for your contribution egelfand! We will review the pull request and get back to you soon.

@KieranBrantnerMagee
Copy link
Member

I don't know if you're able to view the CI output so I've pasted the current exception below, should be a simple fix (just addressing the async import for this same stanza) otherwise this approach looks solid.

ImportError while importing test module '/home/vsts/work/1/s/sdk/servicebus/azure-servicebus/tests/async_tests/test_sessions_async.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: tests/async_tests/test_sessions_async.py:17: in <module> from azure.servicebus.aio import ServiceBusClient, ReceivedMessage, AutoLockRenew azure/servicebus/aio/__init__.py:7: in <module> from ._servicebus_sender_async import ServiceBusSender azure/servicebus/aio/_servicebus_sender_async.py:14: in <module> from ._base_handler_async import BaseHandler azure/servicebus/aio/_base_handler_async.py:17: in <module> from .._base_handler import _generate_sas_token, _AccessToken, BaseHandler as BaseHandlerSync E ImportError: cannot import name '_AccessToken' from 'azure.servicebus._base_handler' (/home/vsts/work/1/s/sdk/servicebus/azure-servicebus/azure/servicebus/_base_handler.py)

@ghost
Copy link

ghost commented Oct 27, 2020

CLA assistant check
All CLA requirements met.

@yunhaoling
Copy link
Contributor

yunhaoling commented Oct 27, 2020

hey @egelfand , thanks for the PR :) we're very close to merging it with just three minor mypy/pylint issue:

mypy is reporting:

azure/servicebus/_base_handler.py:96: error: Name '_AccessToken' is not defined
azure/servicebus/_base_handler.py:150: error: Name '_AccessToken' is not defined

action: _AccessToken -> AccessToken

pylint is reporting:

azure/servicebus/_base_handler.py:5: [W0611(unused-import), ] Unused import collections

action: we could simply remove the import

I'm sure CI would be happy after we apply the changes, then we could approve and merge the PR.

@yunhaoling
Copy link
Contributor

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yunhaoling
Copy link
Contributor

hey @egelfand, your PR looks good to me, could you sign the agreement mentioned by microsoft-cla:
#14450 (comment)

so that we could merge your PR.

@egelfand
Copy link
Contributor Author

Just did so, thanks for all of your help!

Copy link
Contributor

@yunhaoling yunhaoling left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

@yunhaoling yunhaoling merged commit 5407248 into Azure:master Nov 2, 2020
iscai-msft pushed a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Nov 4, 2020
* replaced references to internal _accesstoken with the accesstoken itself, now that its available

* Further replaced references to internal _Accesstoken with Accesstoken itself.

* Corrected mypy/pylint errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. Service Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ServiceBus] Deprecate historical _AccessToken workaround now that AccessToken is imported from core.
3 participants