Reorganize platform settings.

Reorganize platform settings.

The first cut of the platform work was a bit unclean with respect to the
dividing lines between the quickstart app code and the parts that can be
re-used as a library.  This branch moves things around to re-attain that
separation.  

Francesco's original concerns are raised in the previous code review at:

https://code.launchpad.net/~bac/juju-quickstart/platform-settings/+merge/221735

(do not edit description out of merge proposal)

[bac, 2014-05-29 15:13:32]

Please take a look.

[frankban, 2014-05-29 16:14:06]

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.p...
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.p...
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.p...
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.p...
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.p...
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.p...
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.p...
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/tests/test_app.py (right):

https://codereview.appspot.com/102870043/diff/1/quickstart/tests/test_app.py#...
quickstart/tests/test_app.py:124: # All the missing packages are installed from
the PPA.
This comment does not seem right.

https://codereview.appspot.com/102870043/diff/1/quickstart/tests/test_app.py#...
quickstart/tests/test_app.py:147: err = platform_support.UnsupportedOS('Unknown
TRS-80')
Yeah, 4 KB of RAM are not enough for quickstart ;-)

https://codereview.appspot.com/102870043/diff/1/quickstart/tests/test_app.py#...
quickstart/tests/test_app.py:155: # get_platform_installer raises UnsupportedOS,
causing ProgramExit.
If no installer is found, a ProgramExit is raised.

[bac, 2014-05-29 17:54:47]

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.p...
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.p...
quickstart/platform_support.py:71: raise utils.ProgramExit(bytes(err))
Good suggestion.  Done.

https://codereview.appspot.com/102870043/diff/1/quickstart/platform_support.p...
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.p...
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.p...
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.p...
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.p...
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#...
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#...
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.

[bac, 2014-05-29 21:01:35]

Please take a look.

[bac, 2014-05-29 21:08:01]

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.

[frankban, 2014-05-30 11:05:04]

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

https://codereview.appspot.com/102870043/diff/20001/quickstart/platform_suppo...
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_suppo...
quickstart/platform_support.py:125: settings.JUJU_CMD =
ALTERNATE_JUJU_LOCATIONS[platform]
I'd really like to avoid this.

[bac, 2014-05-30 12:25:50]

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.

[bac, 2014-05-30 12:30:31]

*** 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

[bac, 2014-06-02 13:34:17]

Please take a look.