Skip to content

Conversation

@fengzhou-msft
Copy link
Member

Description

The bundled Python in MSI is bumped to 3.8.9. The source code of python is moved from ·site-packages· to python38.zip (which is the original format in the embeddable python). lib2to3 is removed from it as it's not needed.

This PR removes more network SDK API versions with a script to check all used versions in profile and hard-coded ones plus the default versions in models.py, and only keep them in MSI. All hard-coded network APIs used in azure-cli is refactored into one place in _shared.py. Note that all network related extensions are using vendored SDK, so removing a version that is higher than current used versions should not affect this azure-cli version working with future extensions that may use the higher version network SDK.

Testing Guide

History Notes

[Packaging] Bump bundled python to 3.8.9 in MSI.


This checklist is used to make sure that common guidelines for a pull request are followed.

@jsntcy
Copy link
Member

jsntcy commented Apr 22, 2021

Looks like not all Network related extensions are using vendored_sdk, although there is vendored_sdk folder, but some places still use Network defined in CLI core. I remember some extensions are broken when Network SDK version is bumped in CLI core.
Please double check.

@fengzhou-msft
Copy link
Member Author

fengzhou-msft commented Apr 22, 2021

Looks like not all Network related extensions are using vendored_sdk, although there is vendored_sdk folder, but some places still use Network defined in CLI core. I remember some extensions are broken when Network SDK version is bumped in CLI core.
Please double check.

In azure-cli-extensions repo:

  • A search for def network_client_factory showed the code all uses customized resource type with vendored SDK.

  • A search for ResourceType.MGMT_NETWORK showed spring-cloud and ssh calls get_mgmt_service_client to create a network client from the version defined in the profile of azure cli.

  • A search for NetworkManagementClient excluding vendored_sdks folders showed that they are only used in customized resource type with version from vendored SDK.

So no sign of extensions using a hard-coded API version from azure-cli installed network SDK.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

Have you test the new build with all test scenarios?

@fengzhou-msft
Copy link
Member Author

fengzhou-msft commented Apr 28, 2021

Have you test the new build with all test scenarios?

Yes. Tested locally with setup_msi_test.ps1 which is also updated in this PR. The tests for impacted modules all passed. A few tests not related to this PR failed because of some generated test code causing errors like ValueError: attempted relative import beyond top-level package. No plan to fix that in this PR.

# use the version in a profile as much as possible.
AD_HOC_API_VERSIONS = {
ResourceType.MGMT_NETWORK: {
'vm_default_target_network': '2018-01-01',
Copy link
Member

@qwordy qwordy Apr 28, 2021

Choose a reason for hiding this comment

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

Define a const variable? VM_DEFAULT_TARGET_NETWORK = 'vm_default_target_network'

version = get_api_version(cli_ctx, ResourceType.MGMT_NETWORK)
if cli_ctx.cloud.profile == 'latest':
version = '2018-01-01'
version = AD_HOC_API_VERSIONS[ResourceType.MGMT_NETWORK]['vm_default_target_network']
Copy link
Member

Choose a reason for hiding this comment

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

Define a const variable? VM_DEFAULT_TARGET_NETWORK = 'vm_default_target_network'

Copy link
Member Author

@fengzhou-msft fengzhou-msft Apr 29, 2021

Choose a reason for hiding this comment

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

Good suggestion, but as this is temp work, eventually this will be removed, I'll check in as it is now to catch the release. Using hardcoded api version should not be allowed or we should support more dimensions in profile definition so it's possible to use different api versions in different modules. Leave this work to profile owner.

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.

5 participants