Merge lp://qastaging/~hatch/juju-quickstart/no-more-sudo into lp://qastaging/juju-quickstart

Very nice first Python branch Jeff, thank you!
Some changes/comments follow.
Please let me know what do you think about the strategy I propose below.

9 +SYSDEPS = build-essential python-pip python-virtualenv python-dev
Nice! Could you please keep those dependencies in alphabetical order?

27 + raise ProgramExit('Unable to retrieve juju version: {}'.format(err))
28 +
raise ProgramExit('unable to retrieve Juju version: {}'.format(err))
Also the empty line can be removed.

42 + """Easy quickstart.utils.get_juju_version stub"""
Please use a full sentence, and complete the sentence with a dot.
Also, I am not sure this is strictly required, see below.

I am considering a change in how app.bootstrap works.
It could just take a requires_sudo argument: this way it can be nicely tested and the arguments are more understandable. Basically, I'd like something like the following in manage:

requires_sudo = False
if is_local:
   version = ...
   requires_sudo = utils.local_bootstrap_requires_sudo(version)
already_bootstrapped, bsn_series = app.bootstrap(
    options.env_name, requires_sudo=requires_sudo, debug=options.debug)

This way we also call "juju version" only when required.

64 + helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase,
65 + helpers.GetJujuVersionMixin):
Please move the GetJujuVersionMixin after CallTestsMixin. Anyway, if you decide to make the change I suggested above, this should be no longer required.

186 + # Should return a numeric string from the juju version
Please complete the sentence.

192 + # If Juju version returns an error this will throw
Ditto.

202 + # If Juju version is lower than 1.17.1
Ditto.

204 + self.assertEqual(True, value)
This can also be written as: self.assertTrue(value).

207 + # If juju version is higher than 1.16.X
Please complete the sentence.

209 + self.assertEqual(False, value)
Same as above (assertFalse).

212 + # If deploying to a cloud env
214 + self.assertEqual(False, value)
...

223 +def get_juju_version():
What do you think about making this function return a tuple like the following:
(major:int, minor:int, patch:str)?
This way, future code which needs to perform version checks can just call this function avoiding further parsing. Something like:

def get_juju_version():
    """Return the current juju-core version.

    Return a (major:int, minor:int, patch:bytes) tuple, including
    major, minor and patch version numbers.

    Raise a ValueError if the "juju version" call exits with an error
    or the returned version is not well formed.
    """
    retcode, output, error = call('juju', 'version')
    if retcode:
        raise ValueError(error)
    version_string = output.split('-')[0]
    try:
        major, minor, patch = version_string.split('.', 2)
        return int(major), int(minor), patch
    except ValueError:
        msg = 'invalid version string: {}'.format(version_string)
        raise ValueError(msg.encode('utf-8'))

What do you think?

234 +def bootstrap_requires_sudo(is_local, version):
235 + """After Juju version 1.17.0 sudo is no longer required for
236 + bootstrapping local dep...

I think local_requires_sudo could just be

return major <= 1 and minor <= 17 and patch < 2

Hey Jeff have a look at distutils.version.StrictVersion. It seems quite nice and handles parsing and comparison of sane versioning schemes. You could replace get_juju_version and simplify local_bootstrap_requires_sudo.

Thanks for your changes, I think we are almost there!
Please see below my other comments, I'd like to re-review and QA later.

9 +SYSDEPS = python-dev build-essential python-pip python-virtualenv
Let's try alphabetical sorting again ;-)

34 print('sudo privileges required to bootstrap the environment')
I'd be inclined to move this print from here to manage.py:run#394, right after "if is_local".
Basically, if version > 1.17.2 juju-core itself asks for sudo privileges, so this notification still applies.
<shrug>

49 + major, minor, patch = utils.get_juju_version()
get_juju_version can raise a ValueError and we don't catch it here. We should do that and re-raise an app.app.ProgramExit so that quickstart fails gracefully, e.g.:

try:
    major, minor, patch = utils.get_juju_version()
except ValueError as err:
    raise app.ProgramExit(bytes(err))

66 +class GetJujuVersionMixin(object):
It seems this mixin is used only in one test case. What do you think about adding the patch_get_juju_version method directly to that test case (test_manage.TestRun) and avoiding the definition of a new base class?

131 + # The application correctly handles working with local providers.
... with newer Juju versions not requiring "sudo" to bootsrap an environment?

140 + # The application correctly handles working with local providers.
... when Juju requires an external "sudo" call to bootsrap an environment?

214 + Return a (major:int, minor:int, patch:bytes) tuple, including
We are actually returning a tuple of three ints. So s/patch:bytes/patch:int/.
Relatedly, are we sure juju-core stable will never use, e.g. "1.19.2wtf"?
If so, I suppose this is ok, but I'd be more comfortable if we are sure the above will never happen, and it would be ideal if we can support that case too, since it is supported in semantic versioning (IIRC). That's why I think Brad suggested distutils. This can also be another card if you are convinced of this change.

222 + raise ValueError(error)
I know I suggested this, but, looking at the code, utils.call returns the error as a unicode string, so this should really be: raise ValueError(error.encode('utf-8'))

238 + patch_ok = True
239 + if minor < 18:
240 + patch_ok = patch <= 1
241 + return major >= 1 and minor <= 17 and patch_ok
This seems broken, e.g. for version 2.16.1 or 1.16.5.
As Brad suggested, all of this can be rewritten in a more readable way:
return major <= 1 and minor <= 17 and patch < 2
Please also add tests for 2.16.1/1.16.5 or similar.

Actually, the comparison can be more correctly written as:
return (major, minor, patch) < (1, 17, 2)