Fail early if brew is not installed.

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)

[bac, 2014-06-13 00:35:51]

Please take a look.

[frankban, 2014-06-13 09:34:56]

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?

[bac, 2014-06-13 13:51:32]

Please take a look.

[frankban, 2014-06-13 14:16:28]

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#newc...
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#newc...
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_suppo...
File quickstart/platform_support.py (right):

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

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_suppo...
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_suppo...
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_suppo...
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_pla...
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_pla...
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)

[bac, 2014-06-13 14:40:47]

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

https://codereview.appspot.com/108930045/diff/20001/quickstart/platform_suppo...
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_suppo...
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_suppo...
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_suppo...
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_pla...
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/108930045/diff/20001/quickstart/tests/test_pla...
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.