-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{Upgrade} Download MSI to a local file first #19192
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
Conversation
heaths
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple nits, but overall LGTM. Thanks!
| Directly installing from URL may be blocked by policy: https://github.com/Azure/azure-cli/issues/19171 | ||
| This also gives the user a chance to manually install the MSI in case of msiexec.exe failure. | ||
| """ | ||
| logger.warning("Updating Azure CLI with MSI from https://aka.ms/installazurecliwindows") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was logger.debug before. Intentional change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I want the command to be more explicit and give the user more information about what it is doing internally. Otherwise, in case of failure, all that the user can see is CLI upgrade failed. (No different from Something occurred.)
To view the details, the user must specify --debug which is definitely not user-friendly.
| import tempfile | ||
| tmp_dir = tempfile.mkdtemp() | ||
| msi_path = os.path.join(tmp_dir, file_name) | ||
| logger.warning("Downloading MSI to %s", msi_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, isn't this more of a logger.debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
| "or run 'pip install --upgrade azure-cli' in this container") | ||
| elif installer == 'MSI': | ||
| logger.debug("Update azure cli with MSI from https://aka.ms/installazurecliwindows") | ||
| exit_code = subprocess.call(['powershell.exe', '-NoProfile', "Start-Process msiexec.exe -Wait -ArgumentList '/i https://aka.ms/installazurecliwindows'"]) # pylint: disable=line-too-long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to launch PowerShell and let it launch msiexec.exe. Directly launching msiexec.exe is sufficient.
Symptom
Fix #19171, #18566
Directly installing from URL may be blocked by policy: #19171. The exact reason for the failure is unclear, and installing MSI from a URL is always problematic.
Change
Download MSI to a temp folder and install it with
msiexec.exe. This also gives the user a chance to manually install the MSI in case ofmsiexec.exefailure.Testing Guide
In
upgrade_version, after retrieving theinstaller:azure-cli/src/azure-cli/azure/cli/command_modules/util/custom.py
Line 62 in c100b80
force
so that
_upgrade_on_windowscan be triggered.