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

[Upgrade] Add az upgrade command #14803

Merged
merged 27 commits into from
Aug 21, 2020
Merged

Conversation

fengzhou-msft
Copy link
Member

Description

Add az upgrade to update azure-cli on all platforms and with --all enabled, extensions will also be updated.

Auto-upgrade can be enabled with az config set auto-upgrade.enable=yes(default is no). You can also specify whether to update extensions (auto-upgrade.all, default is yes), show prompt message(auto-upgrade.prompt, default is yes). The check against the latest version is done through the local cached ~/.azure/versionCheck.json file. The data is valid for 10 days (the expected wait period for the 3-week release cycle of Azure CLI). The check happens after the execution of a command.

Add az extension list-versions command to list all available versions of an extension.

Testing Guide

History Notes

[Upgrade] Add az upgrade command to upgrade azure cli and extensions
[Extension] Add az extension list-versions command to list all available versions of an extension


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

@yungezz
Copy link
Member

yungezz commented Aug 18, 2020

hi @arrownj could you pls review this PR? thanks

@Azure Azure deleted a comment from yonzhan Aug 18, 2020
try:
exts = index_data[extension_name]
except Exception:
raise CLIError('Extension {} not found.'.format(extension_name))
Copy link
Member

Choose a reason for hiding this comment

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

should this error out or silently swallow? will this func be used other than az upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is not used in az upgrade now as we always update to the latest extension version. This error happens when users type the wrong extension name.

Copy link
Member

Choose a reason for hiding this comment

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

ok. I saw it used by list-available-version, so error out is ok. My original point is that az upgrade internal failure should not error out which might impact user in silent upgrading scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

In silent upgrading scenario (auto-upgrade with no prompt), the exit code and stdoutof a command will not be affected, but warnings and errors from az upgrade will be shown in stderr.

def get_latest_from_github(package_path='azure-cli'):
try:
import requests
git_url = "https://raw.githubusercontent.com/Azure/azure-cli/master/src/{}/setup.py".format(package_path)
Copy link
Member

Choose a reason for hiding this comment

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

how to accommodate air-gapped cloud?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are going to support az upgrade in air-gapped cloud, we may not be able to use system package managers. We could put all the packages in a storage account and download/install the corresponding package based on the user system.

Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to call it out in az upgrade or cloud endpoint discovery design spec?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, will add to cloud endpoint discovery design spec.

if LooseVersion(VERSIONS['versions']['core']['local']) < LooseVersion(VERSIONS['versions']['core']['pypi']): # pylint: disable=line-too-long
import subprocess
import platform
logger.warning("New Azure CLI version available. Running 'az upgrade' to update automatically.")
Copy link
Member

Choose a reason for hiding this comment

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

A warning should be ok right?

Copy link
Member Author

Choose a reason for hiding this comment

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

A warning will not impact the command result for use in script.

src/azure-cli/azure/cli/__main__.py Outdated Show resolved Hide resolved
@fengzhou-msft fengzhou-msft requested a review from houk-ms August 20, 2020 07:47
try:
exts = index_data[extension_name]
except Exception:
raise CLIError('Extension {} not found.'.format(extension_name))
Copy link
Member

Choose a reason for hiding this comment

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

ok. I saw it used by list-available-version, so error out is ok. My original point is that az upgrade internal failure should not error out which might impact user in silent upgrading scenario.

def get_latest_from_github(package_path='azure-cli'):
try:
import requests
git_url = "https://raw.githubusercontent.com/Azure/azure-cli/master/src/{}/setup.py".format(package_path)
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to call it out in az upgrade or cloud endpoint discovery design spec?

telemetry.set_success("Upgrade stopped by user")
return
installer = os.getenv(_ENV_AZ_INSTALLER)
if installer == 'DEB':
Copy link
Member

Choose a reason for hiding this comment

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

upper() then equal?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +80 to +82
if os.geteuid() != 0: # pylint: disable=no-member
apt_update_cmd.insert(0, 'sudo')
az_update_cmd.insert(0, 'sudo')
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the current user does not have sudo permission?

Copy link
Member Author

Choose a reason for hiding this comment

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

An error will be thrown by the Linux system and shown to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the error message is given by the OS, and the message does not confuse the user, then I think it is fine~
It may be better that let the users input their admin password to obtain sudo permissions.

else:
logger.warning(UPGRADE_MSG)
if exit_code:
telemetry.set_failure("CLI upgrade failed.")
Copy link
Contributor

@zhoxing-ms zhoxing-ms Aug 21, 2020

Choose a reason for hiding this comment

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

Could we consider record the error information when the upgrade fails in the telemetry? In this way, we can make statistics on various problems in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. There is an error improvement PR that revamps telemetry. I will work with @houk-ms to improve error tracking here.

Copy link
Contributor

@zhoxing-ms zhoxing-ms left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants