Azure / azure-powershell

Microsoft Azure PowerShell
Other
4.27k stars 3.87k forks source link

Remove dependency of "Microsoft.Azure.Management.Storage" from 4 module test. #11891

Open blueww opened 4 years ago

blueww commented 4 years ago

Description

Currently there are 4 modules test reference "Microsoft.Azure.Management.Storage" besides Az.Storage.

Other module (except Az.Storage) should not reference "Microsoft.Azure.Management.Storage", or we will have unnecessary dependency, and it will take trouble when Az.Storage upgrade "Microsoft.Azure.Management.Storage": other module test need upgrade or even re-record.

Please follow Az.Compute Test, to handle storage account management with \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1, and remove "Microsoft.Azure.Management.Storage" reference.

In the future, all new added module should not reference "Microsoft.Azure.Management.Storage".

Cost

NA

dingmeng-xue commented 4 years ago

We will look into and breakdown them

idear1203 commented 4 years ago

@blueww , I am the owner of Synapse module. Unfortunately I am not able to use \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1.

The purpose to add dependency to Microsoft.Azure.Management.Storage is that I need to use the latest Az.Storage module. I need to create a ADLS Gen 2 storage account (Storage v2 account with hierarchical namespace enabled), which is not supported by \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1.

On the other hand, I don't think it is a big trouble for you to upgrade dependencies for those projects as well. All of these projects are just test projects so there won't be any customer facing issues. Besides, I would like to know if you have to upgrade those project files as well? Is it possible to just let those services that have dependency of Microsoft.Azure.Management.Storage take care of upgrading the assembly by themselves?

blueww commented 4 years ago

@dingmeng-xue

As \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1 is originally created by Mark, not sure if your team could help to update it by support creating storage account with ADLS Gen2 (isHnsEnabled = true, would suggest to upgrade to latest API version 2019-06-01)?

@idear1203 As Az.Storage will upgrade the SDK frequently, if Synapse test project still refer the SDK, storage change can't be merge modify the Synapse test project., since the PR test won't pass. Especially, when the SDK has API version upgrade, will need re-record the test case, this will take a lot of trouble. To avoid the trouble, the origin module test project (compute, sql...) all use \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1 to handle storage account. The new module should follow that.

idear1203 commented 4 years ago

@blueww I did some experiments. If I only upgrade Microsoft.Azure.Management.Storage to 16.0.0 for Az.Storage but not modify Synapse references, I will get the following error:

    System.Management.Automation.CommandNotFoundException : The 'New-AzStorageAccount' command was found in the module 'Az.Storage', but the module could not be loaded. For more information, run 'Import-Module Az.Storage'.
    ---- System.Management.Automation.CmdletInvocationException : Assembly with same name is already loaded
    -------- System.IO.FileLoadException : Assembly with same name is already loaded

If you also update the reference in Synapse test project to 16.0.0, the error will disappear. I think it is not a big deal when you upgrade the assembly for Storage, you also upgrade for other projects. Alternatively, can we define a common variable for Microsoft.Azure.Management.Storage version somewhere in the azure-powershell project? And let all projects reference that common variable. Then you only need to upgrade the common variable rather than upgrading test projects for other services one by one.

I think re-record is not required as long as Az.Storage module doesn't bring in breaking changes in some fundamental cmdlets such as New-AzStorageAccount. After all, this might only happen at most twice a year.

@dingmeng-xue I think keeping \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1 updated (with new Storage features shipped) is a unnecessary burden for Azure PowerShell team. But I will be happy to upgrade Synapse test projects to use \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1 as long as it supports ADLS Gen2 storage account creation.

blueww commented 4 years ago

@idear1203 I don't see the error in your comment. Anyway, I raise the issue because if only upgrade "Microsoft.Azure.Management.Storage" in Az.Storage, there will be error and cause the change of storage can't be merge.

idear1203 commented 4 years ago

@blueww sorry, I sent the comment by mistake...Updated the comment just now...Please let me know your opinions. Let's work together to find a good solution for this issue:blush:

blueww commented 4 years ago

@idear1203 Re-record Unit test is a must, when API version upgrade for "Microsoft.Azure.Management.Storage" . This is because the record session file contains the API version in URI, if API version upgrade and not re-record, Unit test will fail as the Uri not found.

Although currently we can simply upgrade the SDK in Synapse test, when API version upgrade, it will take a lot of trouble. So might make storage feature can't release in time. So we need resolve it.

@dingmeng-xue As we need support datalake storage account, please help to upgrade \tools\ScenarioTest.ResourceManager\AzureRM.Storage.ps1, let me know if you need any support from me. Anyway, Mark should be the person who most familiar with it.

idear1203 commented 4 years ago

@blueww Thanks for your explanation. This makes sense to me.