Merge lp://qastaging/~bac/juju-quickstart/get_os_version into lp://qastaging/juju-quickstart

Reviewers: mp+220737_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/99410054/

Affected files (+78, -0 lines):
   A [revision details]
   M quickstart/manage.py
   M quickstart/tests/test_manage.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: quickstart/manage.py
=== modified file 'quickstart/manage.py'
--- quickstart/manage.py 2014-04-23 11:22:44 +0000
+++ quickstart/manage.py 2014-05-22 20:51:42 +0000
@@ -25,6 +25,7 @@
  import codecs
  import logging
  import os
+import platform
  import shutil
  import sys
  import webbrowser
@@ -54,6 +55,31 @@
          parser.exit()

+class UnsupportedOS(Exception):
+ pass
+
+
+OSX = object()
+LINUX_APT = object()
+LINUX_RPM = object()
+WINDOWS = object()
+
+
+def get_os_version():
+ system = platform.system()
+ if system == 'Darwin':
+ return OSX
+ elif system == 'Windows':
+ return WINDOWS
+ elif system == 'Linux':
+ if os.path.isfile('/usr/bin/apt-get'):
+ return LINUX_APT
+ elif os.path.isfile('/usr/bin/rpm'):
+ return LINUX_RPM
+ raise UnsupportedOS('{} without apt-get nor rpm'.format(system))
+ raise UnsupportedOS(system)
+
+
  def _get_packaging_info(juju_source):
      """Return packaging info based on the given juju source.

Index: quickstart/tests/test_manage.py
=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py 2014-04-23 13:08:12 +0000
+++ quickstart/tests/test_manage.py 2014-05-22 20:51:42 +0000
@@ -919,3 +919,53 @@
              u'admin-secret not found in ~/.juju/environments/local.jenv '
              'or environments.yaml')
          self.assertEqual(expected, context.exception.message)
+
+
+class TestGetOSVersion(unittest.TestCase):
+
+ def patch_platform_system(self, system=None):
+ """Patch the platform.system call to return the given value."""
+ mock_patch_platform = mock.Mock(return_value=system)
+ path = 'quickstart.manage.platform.system'
+ return mock.patch(path, mock_patch_platform)
+
+ def patch_isfile(self, values):
+ mock_patch_isfile = mock.Mock(side_effect=iter(values))
+ path = 'quickstart.manage.os.path.isfile'
+ return mock.patch(path, mock_patch_isfile)
+
+ def test_linux_apt(self):
+ with self.patch_platform_system('Linux'):
+ with self.patch_isfile([True]):
+ result = manage.get_os_version()
+ self.assertEqual(manage.LINUX_APT, result)
+
+ def test_linux_rpm(self):
+ with self.patch_platform_system('Linux'):
+ with self.pat...

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.py#newcode924
quickstart/tests/test_manage.py:924: class
TestGetOSVersion(unittest.TestCase):
Very nice tests.

https://codereview.appspot.com/99410054/diff/1/quickstart/tests/test_manage.py#newcode961
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.py#newcode968
quickstart/tests/test_manage.py:968: with
self.assertRaises(manage.UnsupportedOS) as exc:
Ditto.

https://codereview.appspot.com/99410054/

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.py#newcode924
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.py#newcode961
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.py#newcode968
quickstart/tests/test_manage.py:968: with
self.assertRaises(manage.UnsupportedOS) as exc:
On 2014/05/23 08:16:31, frankban wrote:
> Ditto.

Done.

https://codereview.appspot.com/99410054/

LGTM with minors.
Thank you!

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

https://codereview.appspot.com/99410054/diff/20001/quickstart/platform_support.py#newcode22
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_manage.py
File quickstart/tests/test_manage.py (right):

https://codereview.appspot.com/99410054/diff/20001/quickstart/tests/test_manage.py#newcode920
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?

https://codereview.appspot.com/99410054/

*** 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_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/99410054/diff/20001/quickstart/platform_support.py#newcode22
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.

https://codereview.appspot.com/99410054/