Merge lp://staging/~frankban/juju-quickstart/new-bootstrap-strategy into lp://staging/juju-quickstart

Reviewers: mp+241553_code.launchpad.net,

Message:
Please take a look.

Description:
New bootstrap strategy.

This is a massive branch: my apologies.
But:
     - a new module has been created (netutils), so there
       are license headers and moreover some existing
       functions (with their tests) have been just moved
       from utils.py and must not be re-reviewed.
       The only new function there is check_listening.
     - most of the code are tests: we reached 700 unit
       tests yay!

This branch changes the way quickstart is run on an
existing environment: instead of always trying to
bootstrap, it looks for the jenv file for the current
environment, and, if present, it retrieves the API URL
from there. As a consequence, quickstart uses Juju
in a less expensive way, and it's also faster when
invoked on a bootstrapped environment.

Split the app.bootstrap function to two new
functions: app.bootstrap and app.status. The intent
is to make them more reusable and more easy to test.

Fix a subtle bug never reported and not easy to hit:
the environment type of an existing environment is
now retrieved from the jenv, rather than relying on
what's stored in the environments.yaml file.

Also reorganized the tests for manage.run: over
time they ended up failing to achieve their goal
of describing how the application is run, and
become not really easy to update and change.
Now this situation should be improved.

The little change to the HACKING file is to make
the rst to render correctly on sublime text.

Tests: `make check`.

QA: run quickstart as usual on local and ec2.
Run quickstart again on an already bootstrapped
environment (local and ec2). You should no longer
see the "bootstrapping environment" message.
Instead, a more correct "reusing the already
bootstrapped..." message is displayed.
Also this should feel quicker, especially on ec2.

Thank you!

https://code.launchpad.net/~frankban/juju-quickstart/new-bootstrap-strategy/+merge/241553

(do not edit description out of merge proposal)

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

Affected files (+761, -511 lines):
   M HACKING.rst
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   A quickstart/netutils.py
   M quickstart/tests/helpers.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py
   A quickstart/tests/test_netutils.py
   M quickstart/tests/test_utils.py
   M quickstart/utils.py
   M quickstart/watchers.py

Code LGTM with minor comments. Will QA now.

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

https://codereview.appspot.com/172380043/diff/1/quickstart/app.py#newcode245
quickstart/app.py:245: # For this reason, a call to status() is usually
required at this point.
This is no different from the calling perspective than the 'not already
bootstrapped' situation, right?

If so, perhaps that should be noted in the docstring for this function.

https://codereview.appspot.com/172380043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/manage.py#newcode520
quickstart/manage.py:520: already_bootstrapped = app.bootstrap(
What is the scenario for api_url being None but it is already
bootstrapped? Someone deleted the jenv file out from under juju?

https://codereview.appspot.com/172380043/diff/1/quickstart/netutils.py
File quickstart/netutils.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/netutils.py#newcode50
quickstart/netutils.py:50: timeout = 3
Make timeout an arg with default = 3?

https://codereview.appspot.com/172380043/

LGTM thank you for the updates.

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

https://codereview.appspot.com/172380043/diff/1/quickstart/app.py#newcode190
quickstart/app.py:190: candidates = jenv.get_value(env_name,
'state-servers')
do you know how this list gets updated from Juju? In an HA situation
does juju rewrite the jenv file?

https://codereview.appspot.com/172380043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/manage.py#newcode520
quickstart/manage.py:520: already_bootstrapped = app.bootstrap(
On 2014/11/12 14:48:25, bac wrote:
> What is the scenario for api_url being None but it is already
bootstrapped?
> Someone deleted the jenv file out from under juju?

That has happened and there's been bugs/work around the scenario. It's a
good one to watch for.

https://codereview.appspot.com/172380043/

QA ok

https://codereview.appspot.com/172380043/

Thanks for the reviews, really good stuff!

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

https://codereview.appspot.com/172380043/diff/1/quickstart/app.py#newcode190
quickstart/app.py:190: candidates = jenv.get_value(env_name,
'state-servers')
On 2014/11/12 15:30:44, rharding wrote:
> do you know how this list gets updated from Juju? In an HA situation
does juju
> rewrite the jenv file?

I don't know that for sure: it could be the case when you set HA from
the CLI. I would be surprised if the jenv is updated when you will set
HA using the API. In case of stale addresses, we just fall back to the
old bootstrap strategy.

https://codereview.appspot.com/172380043/diff/1/quickstart/app.py#newcode245
quickstart/app.py:245: # For this reason, a call to status() is usually
required at this point.
On 2014/11/12 14:48:25, bac wrote:
> This is no different from the calling perspective than the 'not
already
> bootstrapped' situation, right?

> If so, perhaps that should be noted in the docstring for this
function.

Done.

https://codereview.appspot.com/172380043/diff/1/quickstart/netutils.py
File quickstart/netutils.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/netutils.py#newcode50
quickstart/netutils.py:50: timeout = 3
On 2014/11/12 14:48:25, bac wrote:
> Make timeout an arg with default = 3?

Done.

https://codereview.appspot.com/172380043/

*** Submitted:

New bootstrap strategy.

This is a massive branch: my apologies.
But:
     - a new module has been created (netutils), so there
       are license headers and moreover some existing
       functions (with their tests) have been just moved
       from utils.py and must not be re-reviewed.
       The only new function there is check_listening.
     - most of the code are tests: we reached 700 unit
       tests yay!

This branch changes the way quickstart is run on an
existing environment: instead of always trying to
bootstrap, it looks for the jenv file for the current
environment, and, if present, it retrieves the API URL
from there. As a consequence, quickstart uses Juju
in a less expensive way, and it's also faster when
invoked on a bootstrapped environment.

Split the app.bootstrap function to two new
functions: app.bootstrap and app.status. The intent
is to make them more reusable and more easy to test.

Fix a subtle bug never reported and not easy to hit:
the environment type of an existing environment is
now retrieved from the jenv, rather than relying on
what's stored in the environments.yaml file.

Also reorganized the tests for manage.run: over
time they ended up failing to achieve their goal
of describing how the application is run, and
become not really easy to update and change.
Now this situation should be improved.

The little change to the HACKING file is to make
the rst to render correctly on sublime text.

Tests: `make check`.

QA: run quickstart as usual on local and ec2.
Run quickstart again on an already bootstrapped
environment (local and ec2). You should no longer
see the "bootstrapping environment" message.
Instead, a more correct "reusing the already
bootstrapped..." message is displayed.
Also this should feel quicker, especially on ec2.

Thank you!

R=bac, rharding
CC=
https://codereview.appspot.com/172380043

https://codereview.appspot.com/172380043/