Add a function for determing OS flavor.

Add a function for determing OS flavor.

Determines OS and then for the case of Linuxes, determines whether the
system uses apt-get or RPM.

https://code.launchpad.net/~bac/juju-quickstart/get_os_version/+merge/220737

(do not edit description out of merge proposal)

[bac, 2014-05-22 23:53:29]

Please take a look.

[frankban, 2014-05-23 08:16:31]

This branch is nive Brad, thank you!
I just have some minor comments below, with a request to move the code outside
manage.py.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode58
quickstart/manage.py:58: class UnsupportedOS(Exception):
I think the OS version related code should live in another module (utils.py or
even better a new separate module like platforms.py or similar). 
The manage and apps modules host non-library code, mostly related to the
juju-quickstart program execution.
Moreover the get_os_version function is likely to be imported from the views and
the models, and this would introduce circular imports.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode59
quickstart/manage.py:59: pass
Could you please add a docstring for this exception?

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode68
quickstart/manage.py:68: def get_os_version():
Could you please add a docstring here as well, also describing the exception
raised by the function?

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode79
quickstart/manage.py:79: raise UnsupportedOS('{} without apt-get nor
rpm'.format(system))
raise UnsupportedOS(b'{} without apt-get nor rpm'.format(system))

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode80
quickstart/manage.py:80: raise UnsupportedOS(system)
platform.system returns an empty string if the value cannot be determined. In
this case, we raise an UnsupportedOS(b''). It would be nice to add a check and
raise something like UnsupportedOS(b'unable to determine the OS version').

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.py
File quickstart/tests/test_manage.py (right):

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.p...
quickstart/tests/test_manage.py:924: class TestGetOSVersion(unittest.TestCase):
Very nice tests.

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.p...
quickstart/tests/test_manage.py:961: with
self.assertRaises(manage.UnsupportedOS) as exc:
exc is not really an exception here, it's more a cm (context manager) <shrug>.

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.p...
quickstart/tests/test_manage.py:968: with
self.assertRaises(manage.UnsupportedOS) as exc:
Ditto.

[bac, 2014-05-23 13:42:27]

Please take a look.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode58
quickstart/manage.py:58: class UnsupportedOS(Exception):
On 2014/05/23 08:16:31, frankban wrote:
> I think the OS version related code should live in another module (utils.py or
> even better a new separate module like platforms.py or similar). 
> The manage and apps modules host non-library code, mostly related to the
> juju-quickstart program execution.
> Moreover the get_os_version function is likely to be imported from the views
and
> the models, and this would introduce circular imports.

Done.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode59
quickstart/manage.py:59: pass
On 2014/05/23 08:16:31, frankban wrote:
> Could you please add a docstring for this exception?

Done.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode68
quickstart/manage.py:68: def get_os_version():
On 2014/05/23 08:16:31, frankban wrote:
> Could you please add a docstring here as well, also describing the exception
> raised by the function?

Done.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode79
quickstart/manage.py:79: raise UnsupportedOS('{} without apt-get nor
rpm'.format(system))
On 2014/05/23 08:16:31, frankban wrote:
> raise UnsupportedOS(b'{} without apt-get nor rpm'.format(system))

Done.

https://codereview.appspot.com/99410054/diff/1/quickstart/manage.py#newcode80
quickstart/manage.py:80: raise UnsupportedOS(system)
On 2014/05/23 08:16:31, frankban wrote:
> platform.system returns an empty string if the value cannot be determined. In
> this case, we raise an UnsupportedOS(b''). It would be nice to add a check and
> raise something like UnsupportedOS(b'unable to determine the OS version').

Done.

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.py
File quickstart/tests/test_manage.py (right):

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.p...
quickstart/tests/test_manage.py:924: class TestGetOSVersion(unittest.TestCase):
On 2014/05/23 08:16:31, frankban wrote:
> Very nice tests.

Thanks

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.p...
quickstart/tests/test_manage.py:961: with
self.assertRaises(manage.UnsupportedOS) as exc:
On 2014/05/23 08:16:31, frankban wrote:
> exc is not really an exception here, it's more a cm (context manager) <shrug>.

Done.

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.p...
quickstart/tests/test_manage.py:968: with
self.assertRaises(manage.UnsupportedOS) as exc:
On 2014/05/23 08:16:31, frankban wrote:
> Ditto.

Done.

[frankban, 2014-05-23 13:56:35]

LGTM with minors.
Thank you!

https://codereview.appspot.com/99410054/diff/20001/quickstart/platform_suppor...
File quickstart/platform_support.py (right):

https://codereview.appspot.com/99410054/diff/20001/quickstart/platform_suppor...
quickstart/platform_support.py:22: )
In the other quickstart modules we separate the __future__ imports from the
other ones, considering them a special case. This is not required by PEP8, so
change it only if you like the idea.

https://codereview.appspot.com/99410054/diff/20001/quickstart/tests/test_mana...
File quickstart/tests/test_manage.py (right):

https://codereview.appspot.com/99410054/diff/20001/quickstart/tests/test_mana...
quickstart/tests/test_manage.py:920: 'or
environments.yaml'.format(settings.JUJU_HOME))
Thank you!

https://codereview.appspot.com/99410054/diff/20001/requirements.pip
File requirements.pip (right):

https://codereview.appspot.com/99410054/diff/20001/requirements.pip#newcode26
requirements.pip:26: # here when moving to a newer jujuclient that fixes that
does correctly
s/that fixes //

https://codereview.appspot.com/99410054/diff/20001/requirements.pip#newcode32
requirements.pip:32: 
Blank line?

[bac, 2014-05-23 14:02:42]

*** Submitted:

Add a function for determing OS flavor.

Determines OS and then for the case of Linuxes, determines whether the
system uses apt-get or RPM.

R=frankban
CC=
https://codereview.appspot.com/99410054

https://codereview.appspot.com/99410054/diff/20001/quickstart/platform_suppor...
File quickstart/platform_support.py (right):

https://codereview.appspot.com/99410054/diff/20001/quickstart/platform_suppor...
quickstart/platform_support.py:22: )
On 2014/05/23 13:56:35, frankban wrote:
> In the other quickstart modules we separate the __future__ imports from the
> other ones, considering them a special case. This is not required by PEP8, so
> change it only if you like the idea.

Done.

https://codereview.appspot.com/99410054/diff/20001/requirements.pip
File requirements.pip (right):

https://codereview.appspot.com/99410054/diff/20001/requirements.pip#newcode26
requirements.pip:26: # here when moving to a newer jujuclient that fixes that
does correctly
On 2014/05/23 13:56:35, frankban wrote:
> s/that fixes //

Done.

https://codereview.appspot.com/99410054/diff/20001/requirements.pip#newcode32
requirements.pip:32: 
On 2014/05/23 13:56:35, frankban wrote:
> Blank line?

Done.