Skip to content

Conversation

@detienne20
Copy link
Contributor

@detienne20 detienne20 commented Aug 6, 2020

Description

Bump azure-mgmt-resource from 10.1.0 to 10.2.0
For PR: #14448

Cause of the problem

The method provider = client.providers.get(namespace) is called to retrieve the api-version list of all the ResourceTypes in the specified namespace. Whether the return value of the ProviderResourceType in the api-version list contains the default_api_version field will affect the api-version fetch logic:

  1. The default_api_version field is not included in the ProviderResourceType returned by the older version before 10.2.0 of SDK, so the latest api-version will be taken (stable version is preferred).
  2. The SDK with version 10.2.0 returns the default_api_version in ProviderResourceType, so the logic to fetch api-version has changed, the value of default_api_version will be used directly.

Fix getting resource API version in ARM cmdlt: @eladperets

Latest swagger updates added the 'defaultApiVersion' property to the GET resource providers response.
The old code was trying to use the default API version and only than fall back to the API versions specified in the manifest.
This was wrong because:

  1. default API version was not part of the response (until the latest SDK update), so this code was not doing anything
  2. default API versions can be null
  3. default API version is mostly used for internal purposes and must be included in the list of available API versions.

This change addresses this issue.

Testing Guide

History Notes

dkmiller and others added 30 commits April 27, 2020 16:13
…e stamp added to deployment_name, excess file removed in commit, launch,json reverted
@detienne20 detienne20 changed the title Daetienn/10.2.0 sdk release {ARM} Bump azure-mgmt-resource from 10.1.0 to 10.2.0 Aug 6, 2020
@detienne20
Copy link
Contributor Author

detienne20 commented Aug 6, 2020

ping @eladperets @zhoxing-ms

@yonzhan yonzhan requested a review from zhoxing-ms August 6, 2020 04:24
@yonzhan yonzhan added this to the S174 milestone Aug 6, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Aug 6, 2020

ARM

@zhoxing-ms zhoxing-ms changed the title {ARM} Bump azure-mgmt-resource from 10.1.0 to 10.2.0 {ARM} Bump azure-mgmt-resource SDK from 10.1.0 to 10.2.0 Aug 6, 2020
from azure.cli.core.parser import IncorrectUsageError
raise IncorrectUsageError('Resource type {} not found.'.format(resource_type_str))
try:
# if the service specifies, use the default API version
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be reviewed by more folks.

# single API version. API versions are returned by the service in a sorted list
# Use the most recent non-preview API version unless there is only a
# single API version. API versions are returned by the service in a sorted list.
api_version = next((x for x in rt.api_versions if not x.endswith('preview')), rt.api_versions[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

With such change, if the rp only has preview api versions, it will get an exception because you only try to get latest stable api version. But in previous design if the default version is preview version, it still can get an preview api version.
I will prefer preview api version could be an backup in such scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but in fact this is how the current logic is implemented. When (x for x in rt.api_versions if not x.endswith('preview')) cannot get a value, next() will take rt.api_versions[0], and rt.api_versions[0] is the latest preview api-version.

Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

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

Does this change has impacts to extensions which import resource SDK ?

@zhoxing-ms
Copy link
Contributor

Does this change has impacts to extensions which import resource SDK ?

@arrownj There should be no impact, because this version of SDK only returns more fields, but does not affect the original logic.
The reason for this breaking this time is that the logic of method get_arm_resource_by_id itself is not correct, so it only affects where it is called.

Copy link
Member

@jsntcy jsntcy left a comment

Choose a reason for hiding this comment

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

:shipit:

@zhoxing-ms zhoxing-ms merged commit 459da3d into Azure:dev Aug 19, 2020
@detienne20 detienne20 deleted the daetienn/10.2.0_SDK_Release branch September 1, 2020 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants