Code review comment for lp:~hatch/juju-quickstart/no-more-sudo

Revision history for this message
Francesco Banconi (frankban) wrote :

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 deployments.
237 + """

At this point this could be called local_bootstrap_requires_sudo and just receive the version tuple.
Also, when writing docstrings, please keep the first sentence in a single line.

« Back to merge proposal