Merge lp://qastaging/~bac/juju-quickstart/donde-brew into lp://qastaging/juju-quickstart

Reviewers: mp+223012_code.launchpad.net,

Message:
Please take a look.

Description:
Fail early if brew is not installed.

Did some refactoring to list required files for each supported platform.
If required files are not found, fail early with an appropriate parser
error message.

https://code.launchpad.net/~bac/juju-quickstart/donde-brew/+merge/223012

(do not edit description out of merge proposal)

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

Affected files (+105, -4 lines):
   A [revision details]
   M quickstart/manage.py
   M quickstart/platform_support.py
   M quickstart/tests/test_manage.py
   M quickstart/tests/test_platform_support.py

Hi Brad, thanks for this.
The code looks good in general, but I'm not sure at this point about the
way we separated layers.
It seems we are starting to mix too much the platform retrieving code
(infrastructural) and the quickstart validation of that platform. Also
my current card would take advantage of some refactoring.
Please consider the following proposal.

In settings we add a new UNKNOWN platform (and possibly a LINUX_UNKNOWN
one).

platform_support.py:
- We get rid of UnsupportedPlatform exception.
- get_platform does not raise exceptions, it just returns the platform
or UNKNOWN. It should not require REQUIRED_FILES, it just checks them
individually like it does currently.
- validate_platform returns None or raises a ValueError with a
descriptive message, e.g.:

def validate_platform(platform):
     if platform == settings.UNKNOWN:
         raise ValueError(b'unable to determine the OS platform')
     if platform == settings.LINUX_UNKNOWN:
         raise ValueError(b'unsupported Linux without apt-get nor rpm')
     if platform == settings.OSX and not
os.path.isfile('/usr/local/bin/brew'):
         raise ValueError(
             b'quickstart requires brew to be installed: '
             b'to install brew, see ...'
         )

manage.py:
in setup we call _validate_platform(options, parser), which is something
like:

dev _validate_platform(options, parser):
     platform = platform_support.get_platform()
     try:
         platform_support.validate_platform(platform)
     except ValueError as err:
         parser.error(bytes(err))
         return
     options.platform = platform

This way:
1) we don't need to use app.ProgramExit at setup time;
2) we support code logic which, e.g. is interested in knowing if local
envs are supported by the current platform and doesn't care about
whether quickstart supports the current platform;
3) we are able to output a more informative error (better than "Required
file is missing: /path/to/brew").

In my current task, I'd like to be able to do the following in the
views:
   platform = platform_support.get_platform()
   supports_local = platform_support.supports_local(platform)
without having to deal with exceptions that make sense if I were asking
"is this platform supported by quickstart>" but are basically
meaningless in the context above.

What do you think?

https://codereview.appspot.com/108930045/

Please take a look.

https://codereview.appspot.com/108930045/

This branch is nice, thank you!
LGTM with some changes.

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode332
quickstart/manage.py:332: """Validate the platform.
"""Validate the platform.

Exit with an error if platform is not supported by quickstart
or is missing files.
"""

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode338
quickstart/manage.py:338: return
parser.error(err.message.encode('utf-8'))
err.message should already be a byte string.
Perhaps return parser.error(bytes(err))?

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

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode30
quickstart/platform_support.py:30: def validate_platform(pf):
s/pf/platform/? <shrug>

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode31
quickstart/platform_support.py:31: """Validate the platform has the
required files installed.
Validate the platform is supported and has the required files?

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode46
quickstart/platform_support.py:46: raise ValueError(bytes(error_msg))
An explicit conversion seems slightly better here:
raise ValueError(error_msg.encode('utf-8'))

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode49
quickstart/platform_support.py:49: b'juju-quickstart requires brew to be
installed on OS X.'
Missing trailing space to separate the two sentences.

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_platform_support.py
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_platform_support.py#newcode128
quickstart/tests/test_platform_support.py:128: class
TestValidatePlatform(unittest.TestCase):
Nice tests, thank you.
This TestCase could take advantage of helpers.ValueErrorTestsMixin, so
that you can avoid some boilerplate, e.g.:

with self.assert_value_error(expected_error):
    platform_support.validate_platform(settings.OSX)

https://codereview.appspot.com/108930045/

*** Submitted:

Fail early if brew is not installed.

Did some refactoring to list required files for each supported platform.
If required files are not found, fail early with an appropriate parser
error message.

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

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode332
quickstart/manage.py:332: """Validate the platform.
On 2014/06/13 14:16:27, frankban wrote:
> """Validate the platform.

> Exit with an error if platform is not supported by quickstart
> or is missing files.
> """

Done.

https://codereview.appspot.com/108930045/diff/20001/quickstart/manage.py#newcode338
quickstart/manage.py:338: return
parser.error(err.message.encode('utf-8'))
On 2014/06/13 14:16:27, frankban wrote:
> err.message should already be a byte string.
> Perhaps return parser.error(bytes(err))?

Done.

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

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode30
quickstart/platform_support.py:30: def validate_platform(pf):
platform clashes with the module name. I started to do platform_ but I
found it annoying.

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode31
quickstart/platform_support.py:31: """Validate the platform has the
required files installed.
On 2014/06/13 14:16:27, frankban wrote:
> Validate the platform is supported and has the required files?

Done.

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode46
quickstart/platform_support.py:46: raise ValueError(bytes(error_msg))
On 2014/06/13 14:16:28, frankban wrote:
> An explicit conversion seems slightly better here:
> raise ValueError(error_msg.encode('utf-8'))

Done.

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_support.py#newcode49
quickstart/platform_support.py:49: b'juju-quickstart requires brew to be
installed on OS X.'
On 2014/06/13 14:16:27, frankban wrote:
> Missing trailing space to separate the two sentences.

Done.

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_platform_support.py
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_platform_support.py#newcode128
quickstart/tests/test_platform_support.py:128: class
TestValidatePlatform(unittest.TestCase):
On 2014/06/13 14:16:28, frankban wrote:
> Nice tests, thank you.
> This TestCase could take advantage of helpers.ValueErrorTestsMixin, so
that you
> can avoid some boilerplate, e.g.:

> with self.assert_value_error(expected_error):
> platform_support.validate_platform(settings.OSX)

Done.

https://codereview.appspot.com/108930045/