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

Proposed by Jeff Pihach
Status: Merged
Merged at revision: 52
Proposed branch: lp://staging/~hatch/juju-quickstart/no-more-sudo
Merge into: lp://staging/juju-quickstart
Diff against target: 247 lines (+126/-18)
7 files modified
Makefile (+1/-1)
quickstart/app.py (+2/-4)
quickstart/manage.py (+15/-3)
quickstart/tests/test_app.py (+1/-1)
quickstart/tests/test_manage.py (+28/-9)
quickstart/tests/test_utils.py (+49/-0)
quickstart/utils.py (+30/-0)
To merge this branch: bzr merge lp://staging/~hatch/juju-quickstart/no-more-sudo
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+203846@code.staging.launchpad.net

Description of the change

Starting with Juju 1.17.1 sudo is no longer required when using a local lxc environment. This branch implements the check functionality on the version and then only uses sudo when required.

Please QA

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :
Download full text (3.3 KiB)

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...

Read more...

51. By Jeff Pihach

Refactored the local_requires_sudo check outside of the bootstrap method

Revision history for this message
Brad Crittenden (bac) wrote :

I think local_requires_sudo could just be

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

52. By Jeff Pihach

Fixed local_bootstrap_requires_sudo returning incorrect values and added extra tests

Revision history for this message
Brad Crittenden (bac) wrote :

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.

53. By Jeff Pihach

Merged in trunk

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

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.

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

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

54. By Jeff Pihach

Minor cleanup and fix to sudo version requirement parsing

55. By Jeff Pihach

fixed post quickstart comment string to react to sudo requirements

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: