Azure-cli: Windows MSI (2.0.62) broken

[tjprescott]

Latest MSI is broken. A few things should happen:

• [x] Find root cause and fix prior to upcoming release
• [ ] Fix release pipeline to automatically check the MSI release like other channels or add a manual check to install and run az self-test with the MSI.

Workaround

For customers impacted by this bug, you should be able to install the 2.0.61 release with the following link: https://bit.ly/2v9W3id

To Reproduce:

• Install latest MSI
• Run: az self-test
• Result:

D:\github>az self-test
This command has been deprecated and will be removed in a future release.
Running CLI self-test.

Loading all commands and arguments...
Error occurred loading commands!

Retrieving all help...
Help loaded OK.

{'load_commands': ImportError("cannot import name 'StorageAccountTypes'",)}

D:\github>echo %ERRORLEVEL%
1

Environment Summary

Windows-10-10.0.17763-SP0
Python 3.6.6
Shell: cmd.exe

azure-cli                         2.0.62

Extensions:
alias                              0.5.2

Python location 'C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\python.exe'
Extensions directory 'C:\Users\trpresco\.azure\cliextensions'
Development extension sources:
    D:\github\azure-cli-extensions

Python (Windows) 3.6.6 (v3.6.6:4cf1f54eb7, Jun 27 2018, 02:47:15) [MSC v.1900 32 bit (Intel)]

[marstr]

I got this figured out, and it's an interesting bug.

The problem we're having is because StorageAccountTypes is declared as a copy of DiskStorageAccountTypes here. There's nothing wrong with doing that from the Python SDK's perspective. However, this is tricking some logic that we use while building the Windows Installer.

Here we scrape all of the symbols that AutoRest exports:
https://github.com/Azure/azure-cli/blob/f45a62c5721314d4e2278a9432fe0540f70e9071/build_scripts/windows/scripts/patch_models_v2.py#L87-L136

My first thought when reading through the above was that inspect wasn't finding StorageAccountTypes, due to it being a global variable and not a proper class declaration. However, I debugged locally and discovered that it was in-fact finding StorageAccountTypes. So I had to keep digging, and I found that in code below assumes all enums that were declared were done so in the enums file, not the __init__ file:
https://github.com/Azure/azure-cli/blob/f45a62c5721314d4e2278a9432fe0540f70e9071/build_scripts/windows/scripts/patch_models_v2.py#L215

Hence, we end up trying to import a symbol from a file where it doesn't exist. There are a few solutions, but if possible, I'd really like to not create any special cases in our code. For that reason, I think the best solution is for me to submit a PR to the azure-sdk-for-python to make the original fix from @lmazuel just resemble the code generated by Python a little more.

/cc @lmazuel, @yugangw-msft, @tjprescott, @adewaleo

[lmazuel]

StorageAccountTypes has been renamed to DiskStorageAccountTypes by the Compute team. To anticipate the future, I would strongly suggest you adapt the CLI code anyway, since the next major version I will be able to do will kill StorageAccountTypes compat import

That being said, I'm looking at your PR right now.

[marstr]

As far as I can tell, we do not actually use StorageAccountTypes. It's only that we end up creating an __init__ that's destined to fail.

[adewaleo]

StorageAccountTypes has been renamed to DiskStorageAccountTypes by the Compute team. To anticipate the future, I would strongly suggest you adapt the CLI code anyway, since the next major version I will be able to do will kill StorageAccountTypes compat import

That being said, I'm looking at your PR right now.

Please let us know in the future when you release the compute SDK that breaks StorageAccountTypes.

[lmazuel]

@adewaleo I would not think like that :). Since you know TODAY, that it will break soon, put a task to do it in the next sprint ;). That would be my advice at least, especially since there is 4 characters to change :)

[adewaleo]

@lmazuel, sounds good. I was thinking of creating the issue. However, I guess I was unsure because it seems like StorageAccountType is still in use by services such as shared image gallery see this swagger pr and SDK. So it seemed like the compute change isn't complete.

[adewaleo]

@lmazuel @marstr

In 2018_09_30: disk.json defines DiskStorageAccountType.
However, in 2019_03_01: compute.json defines StorageAccountType.

If I was to attempt to make a complete switch to DiskStorageAccountType, then parts of the CLI relying on the latest (2019_03_01) compute API version might be affected.

[marstr]

I've tested it, and this issue will not reproduce in 2.0.63 which will be released early next week.

[tjprescott]

I will keep the issue pinned until the release happens. Also, the issue shouldn't really be closed until the release pipeline has been fixed to either require a manual test or add an automated test.

[tjprescott]

Reopening and re-pinning this issue. The release has not yet happened, and the release pipeline hasn't been updated to detect future breakage.

[tjprescott]

The new MSI has been released. Unpinning this issue.

[lmazuel]

@adewaleo This is two differents enums, treats then as such:

• disk.json provides RT "disks" and "snapshots". Use enums from this file only in those scenarios and calls
• compute.json provides the rest