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

Add compatible logic for the track 2 migration of resource #3355

Merged
merged 1 commit into from
May 20, 2021

Conversation

zhoxing-ms
Copy link
Contributor

@zhoxing-ms zhoxing-ms commented May 10, 2021

Because Python SDK used by resource module needs to be migrated to track 2 recently, there are a lot of breaking changes in track 2, while the Python SDK of azure-mgmt-resource used by azure-cli-extension is the same as azure-cli. Therefore, the modules that use Python SDK of azure-mgmt-resource in azure-cli-extension should be added with the compatible logic of track 2.

Please refer to the PR description Azure/azure-cli#17783 for specific compatibility logic to be added


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

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@zhoxing-ms
Copy link
Contributor Author

zhoxing-ms commented May 10, 2021

Since part of the azure-cli-extension modules are not owned by CLI team, so I only added compatible code.
Please help review the new compatible code in your responsible modules and test the affected functions~

@yonzhan yonzhan added this to the S187 milestone May 10, 2021
@@ -368,5 +368,10 @@
"Command": "az webpubsub",
"AzureServiceName": "Azure Web PubSub",
"URL": ""
},
{
"Command": "az sql",
Copy link
Contributor

Choose a reason for hiding this comment

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

follow alphabetical order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! OK

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.

It seems that there is no test run in live mode?

try:
resourceClient.resource_groups.create_or_update(resource_group_name, resource_group_params)
resourceClient.resource_groups.create_or_update(resource_group_name, parameters)
Copy link
Contributor

@akashkeshari akashkeshari May 12, 2021

Choose a reason for hiding this comment

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

We are also using the resourceClient here:

providerDetails = resourceClient.providers.get('Microsoft.Kubernetes')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reminder~ In fact, I know it. However, there is no breaking change for this operation (code link), so no additional compatibility logic is required.

@@ -201,9 +201,11 @@ def create_connectedk8s(cmd, client, resource_group_name, cluster_name, https_pr

# Resource group Creation
if resource_group_exists(cmd.cli_ctx, resource_group_name, subscription_id) is False:
resource_group_params = {'location': location}
from azure.cli.core.profiles import ResourceType
ResourceGroup = cmd.get_models('ResourceGroup', resource_type=ResourceType.MGMT_RESOURCE_RESOURCES)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is another place where we use ResourceType.MGMT_RESOURCE_RESOURCES.

groups = cf_resource_groups(ctx, subscription_id=subscription_id)

imported from:
return get_mgmt_service_client(cli_ctx, ResourceType.MGMT_RESOURCE_RESOURCES,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know it. However, there is no breaking change in this logic, so no additional compatibility logic is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the breaking changes in the new azure-mgmt-resource SDK, please refer to the description of PR Azure/azure-cli#17783

@zhoxing-ms
Copy link
Contributor Author

zhoxing-ms commented May 12, 2021

It seems that there is no test run in live mode?

Yes, because most of the modified modules are not owned by us. Some live tests will fail due to non resource SDK reasons (such as some fixed resources cannot be found or do not have resource permissions), and some of the cases are not covered.
Therefore, I think these tests should be run by the corresponding module owner.

@@ -32,7 +32,7 @@
# TODO: Add any additional SDK dependencies here
DEPENDENCIES = []

VERSION = "0.3.1"
VERSION = "0.3.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

We have another version coming out 0.4.0 so depending on when this goes in, this version might need to be changed

Copy link
Contributor Author

@zhoxing-ms zhoxing-ms May 14, 2021

Choose a reason for hiding this comment

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

OK, may I ask what is your expected version here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should now be 0.4.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it has been updated

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Small comment on versioning

Copy link
Contributor

@JunSun17 JunSun17 left a comment

Choose a reason for hiding this comment

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

For the aks -preview part, looks good to me.

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Please hold this as a call is failing

if validate:
return smc.validate(resource_group_name, deployment_name, properties)
return sdk_no_wait(no_wait, smc.create_or_update, resource_group_name, deployment_name, properties)
return sdk_no_wait(no_wait, smc.begin_create_or_update, resource_group_name, deployment_name, deployment)
Copy link
Contributor

Choose a reason for hiding this comment

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

This call in begin_create_or_update is failing. It is coming with ERROR: 'DeploymentsOperations' object has no attribute 'begin_create_or_update'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the upgrade of azure-mgmt-resource in CLI main repo has not been released, it can only be tested through the dev environment. So may I ask if you are using CLI in dev environment and pulled the latest code from remote dev branch?
The expected version of azure-mgmt-resource is 16.1.0, please execute pip list to confirm whether the version is correct~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathan-innis May I ask can you pass the test now?

@zhoxing-ms
Copy link
Contributor Author

As I have communicated with @jeffj6123, module mesh will be deleted, so this module also be passed

@zhoxing-ms zhoxing-ms merged commit f19ddbe into Azure:master May 20, 2021
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.

6 participants