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

Reviewers: mp+221412_code.launchpad.net,

Message:
Please take a look.

Description:
Include installation steps for OS X.

A slight re-factoring was done to move platform-specific steps from
app.py to
platform_support.py.

Also ProgramExit was moved to utils.py. Many of the test still refer to
app.ProgramExit. That change can be made post-review so as to not
clutter up
the actual changes, if desired.

https://code.launchpad.net/~bac/juju-quickstart/brew-commands/+merge/221412

(do not edit description out of merge proposal)

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

Affected files (+200, -40 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/platform_support.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_platform_support.py
   M quickstart/utils.py

Hey Brad, this branch looks good and has very nice tests.
I'd like to know your opinion on the only non-minor change I described
below (re ProgramExit) before QAing.
Thank you!

https://codereview.appspot.com/102870043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/102870043/diff/1/quickstart/app.py#newcode49
quickstart/app.py:49: This is usually done by adding the Juju stable PPA
and installing the
This docstring here no longer applies in the case we are running on OSX.

https://codereview.appspot.com/102870043/diff/1/quickstart/app.py#newcode52
quickstart/app.py:52: If distro_only is True, the above PPA is not added
to the apt sources, and
I guess distro_only will be only relevant on Ubuntu. Could you please
add/change this comment?

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode65
quickstart/platform_support.py:65: def _installer_apt(distro_only,
required_packages):
In this context, required packages could be the first argument with a
distro_only=True kwarg. <shrug>

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode71
quickstart/platform_support.py:71: raise utils.ProgramExit(bytes(err))
I'd be inclined to keep ProgramExit in apps, and add to the installers
contract the fact that they can raise an OSError.

So, basically, installers return None or raise an OSError with a message
if something goes wrong. apps code can then catch the exception and
convert it to a ProgramExit.

The goal is to delimit the scope of a hard exit: ProgramExit makes sense
in apps and in some cases also in manage. This module here should behave
like a library (and I remember someone raising some interest in using
quickstart as a lib), and when manipulating an OS (e.g. installing
packages) an OSError seems better to me.

If you are convinced, please document the possible raised exception in
both installers docstrings.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode77
quickstart/platform_support.py:77: return retcode, error
So in this case:
if retcode:
     raise OSError(...)

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode83
quickstart/platform_support.py:83: Note distro_only is meaningless on OS
X as there is no PPA option. It is
Nice.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode100
quickstart/platform_support.py:100: LINUX_APT: ['juju-core',
'juju-local'],
Switch to tuples? <shrug #2>

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode109
quickstart/platform_support.py:109: Returns platform and installer
method. The installer_method is None if no
installer callable?

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode117
quickstart/platform_support.py:117: """Return True if the platform
support local (LXC) deploys."""
s/support/supports/

https://codereview.appspot.com/102870043/diff/1/quickstart/tests/test_app.py
File quickstart/tes...

Thanks for the nice review. All changes made.

https://codereview.appspot.com/102870043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/102870043/diff/1/quickstart/app.py#newcode52
quickstart/app.py:52: If distro_only is True, the above PPA is not added
to the apt sources, and
On 2014/05/29 16:14:05, frankban wrote:
> I guess distro_only will be only relevant on Ubuntu. Could you please
add/change
> this comment?

Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode65
quickstart/platform_support.py:65: def _installer_apt(distro_only,
required_packages):
Could, but since the full signature must be used when calling it doesn't
help.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode71
quickstart/platform_support.py:71: raise utils.ProgramExit(bytes(err))
Good suggestion. Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode77
quickstart/platform_support.py:77: return retcode, error
On 2014/05/29 16:14:06, frankban wrote:
> So in this case:
> if retcode:
> raise OSError(...)

Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode100
quickstart/platform_support.py:100: LINUX_APT: ['juju-core',
'juju-local'],
On 2014/05/29 16:14:06, frankban wrote:
> Switch to tuples? <shrug #2>

Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode109
quickstart/platform_support.py:109: Returns platform and installer
method. The installer_method is None if no
On 2014/05/29 16:14:06, frankban wrote:
> installer callable?

Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode117
quickstart/platform_support.py:117: """Return True if the platform
support local (LXC) deploys."""
On 2014/05/29 16:14:06, frankban wrote:
> s/support/supports/

Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.py#newcode117
quickstart/platform_support.py:117: """Return True if the platform
support local (LXC) deploys."""
On 2014/05/29 16:14:06, frankban wrote:
> s/support/supports/

Done.

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

https://codereview.appspot.com/102870043/diff/1/quickstart/tests/test_app.py#newcode124
quickstart/tests/test_app.py:124: # All the missing packages are
installed from the PPA.
On 2014/05/29 16:14:06, frankban wrote:
> This comment does not seem right.

Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/tests/test_app.py#newcode155
quickstart/tests/test_app.py:155: # get_platform_installer raises
UnsupportedOS, causing ProgramExit.
On 2014/05/29 16:14:06, frankban wrote:
> If no installer is found, a ProgramExit is raised.

Done.

https://codereview.appspot.com/102870043/

Please take a look.

https://codereview.appspot.com/102870043/

Hi Francesco,

The changes include the things you requested plus a later change to set
JUJU_CMD for OS X, since brew installs it in /usr/local/bin.

QA:
brew uninstall juju
.venv/bin/python juju-quickstart -e ec2 # Or some other existing
environment

Ensure you see:
Installing the following packages: juju

Which juju should show /usr/local/bin/juju

Re-running quickstart does not show installation of juju.

Note that quickstart will not actually work. More work to do.

https://codereview.appspot.com/102870043/

Thanks for the changes Brad.
Please take a look at my proposal below for a different API. Given the
juju path changes across platforms, I guess we should handle that
carefully.
If you agree with my comments, feel free to tackle that in a follow up
task, in which case you have my LGTM.

Also I've found another minor: in manage.run the log message in
logging.debug('ensuring juju and lxc are installed') should be changed
e.g. 'ensuring juju is installed'.

QA is ok, juju is installed by brew.
Those are the errors I see in two subsequent runs (and I suppose you
encountered similar errors).

websocket.Dial wss://ec2-54-226-250-148.compute-1.amazonaws.com:17070/:
dial tcp 54.226.250.148:17070: operation timed out
This seems spurious.

unable to connect to the Juju API server on
wss://ec2-107-20-18-130.compute-1.amazonaws.com:17070: [Errno 1]
_ssl.c:504: error:1407742E:SSL routines:SSL23_GET_SERVER_HELLO:tlsv1
alert protocol version
This seems https://bugs.launchpad.net/juju-deployer/+bug/1323803: I
added quickstart to the list of affected projects.

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

https://codereview.appspot.com/102870043/diff/20001/quickstart/platform_support.py#newcode111
quickstart/platform_support.py:111: ALTERNATE_JUJU_LOCATIONS = {
I am not sure this should be here.
AFAICT utils.get_juju_version now only works thanks to the fact that an
external module level name is redefined by get_platform_installer.
It took me some minutes to figure out that, and I think we can make this
simpler. The same applies for the places in apps where juju is invoked
(juju status etc.).
At this point, since the juju cmd paths are different across platforms,
I'd be inclined to move the platform definitions in settings, and then
add something like:

JUJU_CMD_PATHS = {
    OSX: ...
    default: ...
}

A settings.JUJU_PACKAGES sounds also good to me. At that point,
INSTALLERS is the only constant in platform_support, and we can have
something like:

platform_support.get_platform()
platform_support.get_juju_installer(platform) # Renamed, and it receives
a platform, if you agree.
platform_support.supports_local(platform)
platform_support.get_juju_cmd(platform) # New function.

The get_platform() function could also be called once at setup time and
stored as an option, so that quickstart exits with a nice message sooner
later than later.

What do you think?

I know this is quite a change, so, if you agree with this proposal, feel
free to subdivide the work in multiple tasks.

https://codereview.appspot.com/102870043/diff/20001/quickstart/platform_support.py#newcode125
quickstart/platform_support.py:125: settings.JUJU_CMD =
ALTERNATE_JUJU_LOCATIONS[platform]
I'd really like to avoid this.

https://codereview.appspot.com/102870043/

Thanks for the thoughtful review Francesco.

Yes, having to redefine JUJU_CMD showed weaknesses in the original
approach. I like your idea and will pursue it in the next branch.

I have made the change to the log message and will land the branch with
the current implementation.

https://codereview.appspot.com/102870043/

*** Submitted:

Include installation steps for OS X.

A slight re-factoring was done to move platform-specific steps from
app.py to
platform_support.py.

The redefinition of JUJU_CMD is a known problem and a refactoring branch
will be forthcoming.

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

https://codereview.appspot.com/102870043/

Please take a look.

https://codereview.appspot.com/102870043/