Add a simple command that knows how to download the dcos client. #1166

brendandburns · 2016-10-26T04:32:00Z

@rgardler @ahowe

This is my first PR to the Azure CLI, please advise if I'm doing it wrong...

tjprescott

A couple comments. You also need to address the pylint errors. You can use 'scripts/lintall' locally to see the results before you open your PR since the CI can be slow. I would recommend @derekbekoe take a look at the PR as well, since it deals with installing things.

tjprescott · 2016-10-26T15:51:23Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+        install_location = '/usr/local/bin/dcos'
+        file_url = 'https://downloads.dcos.io/binaries/cli/darwin/x86-64/dcos-1.8/dcos'
+    else:
+        logger.error('unknown system: %s' % system)


This should probably raise a CLIError. Otherwise it will attempt to install to "".

Something like raise CLIError("Unknown system type '{}'".format(system))

tjprescott · 2016-10-26T15:53:06Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+        logger.error('unknown system: %s' % system)
+
+    user_location = input('dcos binary install location [%s]:' % install_location)
+    if len(user_location) == 0:


if not len(user_location): ?

derekbekoe

My main comments are about the use of install_location.
It should be a command line argument (not interactive) and you can use the _params.py file to set the defaults for the user when the command line argument is registered in the CLI.

derekbekoe · 2016-10-26T16:55:17Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+        install_location = '/usr/local/bin/dcos'
+        file_url = 'https://downloads.dcos.io/binaries/cli/darwin/x86-64/dcos-1.8/dcos'
+    else:
+        logger.error('unknown system: %s' % system)


Something like raise CLIError("Unknown system type '{}'".format(system))

derekbekoe · 2016-10-26T17:02:35Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

@@ -59,6 +61,33 @@ def dcos_browse(name, resource_group_name):

    return

+def dcos_install():


install_location should be an optional argument to this function so it can be specified on the command line with acs dcos install --install-location ...

i.e.

def dcos_install(install_location=None): ...

You can the acs/_params.py file to define what install_location should be.
This means that when this method is called, the right install_location will be passed in.

So for example, you can remove the install locations that have been defined here and add the following lines to the acs/_params.py file:

import platform def _get_default_install_location(): system = platform.system() if system == 'Windows': return 'C:\Program Files\dcos.exe' if system == 'Linux': return '/usr/local/bin/dcos' if system == 'Darwin': return '/usr/local/bin/dcos' else: return None register_cli_argument('acs dcos install', 'install_location', options_list=('--install-location',), default=_get_default_install_location())

This means when the user runs az acs dcos install -h, they'll see:

Arguments --install-location : Default: /usr/local/bin/dcos.

with the default being computed automatically.

derekbekoe · 2016-10-26T17:04:07Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+    else:
+        logger.error('unknown system: %s' % system)
+
+    user_location = input('dcos binary install location [%s]:' % install_location)


Our convention is to not take user input interactively like this.
It makes it difficult to set the install_location with a script.
I'd suggest removing this and adding an optional argument install_location like I've described above.

derekbekoe · 2016-10-26T17:09:19Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+        user_location = install_location
+
+    logger.info("Downloading client to %s\n" % user_location)
+    urllib.request.urlretrieve(file_url, user_location)


Should wrap this in a try/except as it can raise an IOError if the connection cannot be made (https://docs.python.org/2/library/urllib.html#urllib.urlopen).

e.g.

try: urllib.request.urlretrieve(file_url, user_location) except IOError: raise CLIError('Connection error attempting to download client')

derekbekoe · 2016-10-26T17:09:58Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+    if len(user_location) == 0:
+        user_location = install_location
+
+    logger.info("Downloading client to %s\n" % user_location)


Shouldn't need the '\n' in the log message

derekbekoe · 2016-10-26T17:10:24Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/generated.py

 cli_command('acs dcos browse', dcos_browse)
+cli_command('acs dcos install', dcos_install)


Missing new line that pylint was complaining about.

derekbekoe · 2016-10-26T17:27:46Z

@brendandburns As an FYI, to prevent the 'cla-required' label from being applied, visit aka.ms/azuregithub to link your GitHub account.

derekbekoe · 2016-10-26T18:07:36Z

Also, why is the command acs dcos install when it actually downloads the client only?

brendandburns · 2016-10-26T20:35:30Z

@derekbekoe thanks for the fast review. addressed all coments, pylint is clean now.

Changed to acs dcos install-cli

Please take another look.

Thanks!
--brendan

derekbekoe

Thanks for making those changes.
I've added a few more comments with a suggestion for how to fix the CI error.

derekbekoe · 2016-10-26T20:58:07Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_params.py

 from azure.cli.core.commands import register_cli_argument
 from azure.cli.core.commands.parameters import (
    name_type,
    resource_group_name_type)
+from azure.cli.core._util import CLIError
+
+def _get_default_install_location(exeName):


python convention is snake casing so exe_name

derekbekoe · 2016-10-26T21:40:00Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_params.py

+    elif system == 'Linux' or system == 'Darwin':
+        install_location = '/usr/local/bin/{}'.format(exeName)
+    else:
+        raise CLIError('Proxy server ({}) does not exist on the cluster.'.format(system))


Here, should just set install_location=None.
It would be too soon to raise a CLIError at this stage as it'll cause the 'acs' module to not load.

Then, in the dcos_install_cli() method, you'd have a check for:

def dcos_install_cli(install_location=None): if not install_location: raise CLIError("No install location specified and it could not be determined from the current platform '{}'".format(system))

derekbekoe · 2016-10-26T21:40:32Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_params.py

 from azure.cli.core.commands import register_cli_argument
 from azure.cli.core.commands.parameters import (
    name_type,
    resource_group_name_type)
+from azure.cli.core._util import CLIError


After making my other suggested change, this import would be unused so can be removed.

derekbekoe · 2016-10-26T21:47:08Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

 import random
 import string
+import urllib.request


The CI build is failing because of this.
urllib.request is defined in Python 3 but not Python 2.

This should fix this:
from six.moves.urllib.request import urlretrieve
from: https://pythonhosted.org/six/#module-six.moves.urllib.request

yugangw-msft · 2016-10-27T20:09:36Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_params.py

+def _get_default_install_location(exeName):
+    system = platform.system()
+    if system == 'Windows':
+        install_location = 'C:\\Program Files\\{}.exe'.format(exeName)


please use os.environ['ProgramFiles']

x64 vs x86? And please verify behavior with both 64bit/32bit python on windows...

Also, os.environ.get('ProgramFiles') instead as then you don't get a KeyError from the dict and can check for None on the result and not set install location if you get none back from the call to os.environ.

yugangw-msft · 2016-10-27T20:13:02Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+    file_url = ''
+    system = platform.system()
+    if system == 'Windows':
+        file_url = 'https://downloads.dcos.io/binaries/cli/windows/x86-64/dcos-1.8/dcos.exe'


Suggest to add a command help to call out the version of 1.8, or expose the version as parameter people can pick; otherwise people will be surprised that we don't install newer stable one after some period.

brendandburns · 2016-10-28T04:58:55Z

Comments addressed, please re-check.

Thanks!
--brendan

…into download

yugangw-msft · 2016-10-28T20:50:30Z

@brendandburns, to merge the PR, you will need to suppress the pylint error of "E: 9, 0: Unable to import 'six.moves.urllib.request' (import-error)". I assume you favor this lib over the requests package for the simplicity reason

brendandburns · 2016-10-28T21:10:59Z

@yugangw-msft do I need to worry about adding that to a dependency list somewhere?

THanks
--brendan

yugangw-msft · 2016-10-28T21:18:52Z

@brendandburns, six is already brought in through the common dependency of azure-cli-core. Still, I think you should update acs command package's setup.py with this dependency. The pylint error might still surface for you to suppress
//cc:@derekbekoe

derekbekoe · 2016-10-28T21:37:56Z

@brendandburns: As @yugangw-msft mentioned, add #pylint: disable=import-error after the six import that's failing the pylint check.
Also, add six to the DEPENDENCIES section of the setup.py file that Yugang referenced.

brendandburns · 2016-10-28T23:25:45Z

Done, please take another look.

Thanks!
--brendan

yugangw-msft · 2016-10-28T23:32:56Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

 import random
 import string
+#pylint: disable=import-error
+from six.moves.urllib.request import urlretrieve


You probably should add the suppression at the same line ; otherwise the suppression will apply on all the imports in the rest of the file

brendandburns · 2016-10-29T03:06:30Z

Comments addressed, please take another look.

Thanks!
--brendan

azurecla added the cla-required label Oct 26, 2016

tjprescott reviewed Oct 26, 2016

View reviewed changes

derekbekoe suggested changes Oct 26, 2016

View reviewed changes

brendandburns added 2 commits October 26, 2016 13:09

Add a simple command that knows how to download the dcos client.

d27fc5d

comments addressed.

39973d4

brendandburns force-pushed the download branch from dacae41 to 39973d4 Compare October 26, 2016 20:33

derekbekoe suggested changes Oct 26, 2016

View reviewed changes

derekbekoe added cla-not-required and removed cla-required labels Oct 26, 2016

Fix a bug in the file_url setup

44c6ea0

yugangw-msft reviewed Oct 27, 2016

View reviewed changes

address comments.

ed3d116

derekbekoe approved these changes Oct 28, 2016

View reviewed changes

Merge branch 'download' of https://github.com/brendandburns/azure-cli …

600d9c0

…into download

brendandburns force-pushed the download branch from 20efcbe to 06ed02b Compare October 28, 2016 23:25

yugangw-msft reviewed Oct 28, 2016

View reviewed changes

Fix lint errors.

3d30ae0

brendandburns force-pushed the download branch from 06ed02b to 3d30ae0 Compare October 29, 2016 03:06

yugangw-msft merged commit 3ff036b into Azure:master Oct 29, 2016

haroldrandom added the cla-not-required label Oct 25, 2019

		@@ -59,6 +61,33 @@ def dcos_browse(name, resource_group_name):

		return

		def dcos_install():

		cli_command('acs dcos browse', dcos_browse)
		cli_command('acs dcos install', dcos_install)

Add a simple command that knows how to download the dcos client. #1166

Add a simple command that knows how to download the dcos client. #1166

Conversation

brendandburns commented Oct 26, 2016

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe commented Oct 26, 2016

derekbekoe commented Oct 26, 2016

brendandburns commented Oct 26, 2016

derekbekoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Oct 28, 2016

yugangw-msft commented Oct 28, 2016

brendandburns commented Oct 28, 2016

yugangw-msft commented Oct 28, 2016 • edited Loading

derekbekoe commented Oct 28, 2016

brendandburns commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Oct 29, 2016

yugangw-msft commented Oct 28, 2016 •

edited

Loading