-
Notifications
You must be signed in to change notification settings - Fork 3k
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
{Core} Vendor track 2 subscription SDK in azure-cli-core #15711
Conversation
83f9c82
to
748e1ff
Compare
Core |
# Conflicts: # src/azure-cli-core/azure/cli/core/_profile.py # src/azure-cli-core/azure/cli/core/adal_authentication.py # src/azure-cli-core/azure/cli/core/commands/client_factory.py # src/azure-cli-core/setup.py
s = SubscriptionType() | ||
s.state = StateType.enabled | ||
s.state = 'Enabled' |
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.
Track 2 uses str
instead of Enum
SubscriptionState
as the value when deserializing Subscription
objects, so we do the same.
Track 1:
'state': {'key': 'state', 'type': 'SubscriptionState'},
Track 2:
'state': {'key': 'state', 'type': 'str'},
See email: A breaking change in Enum property's deserialization
@@ -449,8 +458,7 @@ def _match_account(account, subscription_id, secondary_key_name, secondary_key_v | |||
|
|||
@staticmethod | |||
def _pick_working_subscription(subscriptions): | |||
from azure.mgmt.resource.subscriptions.models import SubscriptionState | |||
s = next((x for x in subscriptions if x.get(_STATE) == SubscriptionState.enabled.value), None) | |||
s = next((x for x in subscriptions if x.get(_STATE) == 'Enabled'), None) |
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.
'azure.cli.core.extension', | ||
'azure.cli.core.profiles', | ||
], | ||
packages=find_packages(exclude=["*.tests", "*.tests.*", "tests.*", "tests", "azure", "azure.cli"]), |
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.
Change the package finding logic so that vendored_sdks/
are included. Thank @arrownj for the help.
@@ -77,6 +77,8 @@ | |||
|
|||
_AZ_LOGIN_MESSAGE = "Please run 'az login' to setup account." | |||
|
|||
_USE_VENDORED_SUBSCRIPTION_SDK = True |
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.
Do we need to change this to False
in the future ?
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.
Probably not. azure-cli-core
uses only very limited functionalities from this SDK, so we can always use the fixed version.
api_version = get_api_version(cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS) | ||
client_kwargs = _prepare_client_kwargs_track2(cli_ctx) | ||
# We don't need to change credential_scopes as 'scopes' is ignored by BasicTokenCredential anyway | ||
client = client_type(credentials, api_version=api_version, | ||
base_url=self.cli_ctx.cloud.endpoints.resource_manager) |
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.
What api-version of Subscription is used by azure-cli, the corresponding version of vendor SDK is required for azure-cli-core, right?
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.
Only 2019-11-01
and 2016-06-01
are used and they are included under vendored_sdks
.
ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2019-11-01', |
ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2016-06-01', |
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.
Yes, I know it~ I mean if a new api-version
of Subscription is added to azure-cli
in the feture, does the vendor SDK in azure-cli-core
also need to change to this new api-version
, right?
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.
MGMT_RESOURCE_SUBSCRIPTIONS
is now only used by azure-cli-core
. We can use it for now, assuming the corresponding api-version
is vendored correctly.
self.access_token = access_token | ||
|
||
def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument | ||
return AccessToken(self.access_token, int(time.time() + 3600)) |
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.
Can we use a meaningful constant instead of a magic number (3600) so that reviewers can understand it easier?@jiasli@microsoft.com
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.
As self.access_token
is a fixed value, get_token
cannot refresh the token, thus defeating the purpose of returning a valid expires_on
. Using time.time() + 3600
will force SDK to use the provided token, regardless of whether the token is expired or not.
Also, Subscriptions - List and Tenants - List APIs are not long-running operations, so the time of expires_on
will not be reached anyway.
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.
Description
Fix #15496:
azure-cli-core
has hard dependency on previous version ofazure-mgmt-resource
Workaround #15530: Bump
azure-mgmt-resource
to Track 2 15.0.0Replace PR #15669: {Core} Migrate
azure-cli-core
to useazure-mgmt-resource
Track 2 SDKVendor track 2 subscription SDK in
azure-cli-core
so that it doesn't have dependency onazure-mgmt-resource
, which is required byresource
command module.In this way,
azure-cli-core
can start using Track 2azure-mgmt-resource
SDK beforeresource
finishes the migration.To switch back to using public SDK, simply change
_USE_VENDEROED_SUBSCRIPTION_SDK
toFalse
.