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.
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 bootstrap_ requires_ sudo(version) bootstrapped, bsn_series = app.bootstrap( env_name, requires_ sudo=requires_ sudo, debug=options. debug)
if is_local:
version = ...
requires_sudo = utils.local_
already_
options.
This way we also call "juju version" only when required.
64 + helpers. CallTestsMixin, ProgramExitTest sMixin, unittest.TestCase, GetJujuVersionM ixin):
65 + helpers.
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.assertEqua l(True, value) (value) .
This can also be written as: self.assertTrue
207 + # If juju version is higher than 1.16.X
Please complete the sentence.
209 + self.assertEqua l(False, value)
Same as above (assertFalse).
212 + # If deploying to a cloud env l(False, value)
214 + self.assertEqua
...
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 split(' -')[0] string. split(' .', 2) version_ string) msg.encode( 'utf-8' ))
or the returned version is not well formed.
"""
retcode, output, error = call('juju', 'version')
if retcode:
raise ValueError(error)
version_string = output.
try:
major, minor, patch = version_
return int(major), int(minor), patch
except ValueError:
msg = 'invalid version string: {}'.format(
raise ValueError(
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.