Code review comment for lp:~frankban/juju-quickstart/idempotent-feature

*** Submitted:

Make quickstart idempotent.

- do not bootstrap an environment if already bootstrapped;
- do not deploy the GUI if already there.

Sorry, the diff is long, but there are a lot of tests.

Tests: make check

QA:
- .venv/bin/python juju-quickstart -e ec2
   and ensure the GUI is correctly deployed;

- .venv/bin/python juju-quickstart -e ec2
   again, to check it recognizes that env is
   already bootstrapped and the
   GUI unit is already there;

In the following steps, sometimes the browser
can get confused about certs, wss conections
etc. If the GUI is not loading correctly,
try harder, use incognito mode, change the
browser.

- juju destroy-service -e ec2 juju-gui;
- .venv/bin/python juju-quickstart -e ec2
   again, to check the service and the unit
   are correctly re-deployed;

Incidentally the step above, in the case it
succeeds, also demonstrates that the GUI can
safely be redeployed in the same machine: I
wasn't sure about this and this means we are
cleaning up things correctly in our charm, yay!

- juju unexpose -e ec2 juju-gui;
- .venv/bin/python juju-quickstart -e ec2
   again, to check that the service is
   properly re-exposed;

- juju destroy-unit -e ec2 juju-gui/0;
- .venv/bin/python juju-quickstart -e ec2
   again, to check that the unit is
   re-added on the existing service
   (this time it should be named juju-gui/1);

- juju destroy-service -e ec2 juju-gui;
- juju deploy -e ec2 juju-gui (if juju exits with a
   "service already exists" error, retry after a while);
- .venv/bin/python juju-quickstart -e ec2 \
     bundle:~jorge/mediawiki-simple/4/mediawiki-simple;
The last command, executed right after juju-deploy should
also demonstrates that incidentally quickstart
can also be used to watch an already running
deployment, and that a bundle can still be deployed;

Final check:
- .venv/bin/python juju-quickstart -e ec2;
just to ensure quickstart is not surprised
that the unit is not in the bootstrap node
(i.e. you should see "juju-gui/0 is ready on machine 1").

Thanks a lot for testing all of this.
I added a card to automate the QA above with
a collection of functional tests.

Remember to destroy your ec2 environment.

R=gary.poster, rharding
CC=
https://codereview.appspot.com/28250044

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

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode66
quickstart/app.py:66: # XXX frankban 2013-11-13: the check below is
weak. We are relying on
On 2013/11/18 15:02:19, gary.poster wrote:
> Could you file a Juju Core bug, add juju-quickstart as an affected
project, and
> add the bug number to this comment, please?

> It looks like Wm prefers --format to get machine readable output, so
maybe
> that's a way forward?

Done. Filed bug 1252322.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode74
quickstart/app.py:74: # jens files seems to be an internal juju-core
detail. Definitely we
On 2013/11/18 15:42:56, rharding wrote:
> jenv

Done.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode77
quickstart/app.py:77: # rather thank comparing the expected error with
the obtained one, we
On 2013/11/18 15:02:19, gary.poster wrote:
> typo: ...rather than...

Done.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode82
quickstart/app.py:82: already_bootstrapped = True
On 2013/11/18 15:02:19, gary.poster wrote:
> It took me a few seconds of thinking to realize why you didn't just
return True
> here. Low priority, but might be nice to say something explanatory.
Example:

> # Juju is bootstapped, but we don't know if it is ready yet. Fall
> # through to the next block for that check.

Done.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode155
quickstart/app.py:155: Raise a ProgramExit if the API server returns an
error response.
On 2013/11/18 15:02:19, gary.poster wrote:
> After reading docstring, I am not clear on three things.

> (1) why is the provider type important here?
Fixed, now a machine is directly passed, and the check is done outside.

> (2) why is the already_bootstrapped value important here?
Renamed to check_preexisting.

> (3) what happens if the GUI charm exists with a different name? what
happens if
> the given name is not a GUI charm?

As agreed, I will work on this in a future branch.

> Maybe we don't need to answer all of these in the docstring, but
seemed like a
> hole as I read it.

> The code below answers #1 and #2; I have a comment below about #3.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode158
quickstart/app.py:158: if already_bootstrapped:
On 2013/11/18 15:02:19, gary.poster wrote:
> Ah! It is an optimization.

> I thought about suggesting some different names for the argument,
focused on the
> local behavior of enabling checking for services and units rather than
the
> contextual idea that we are already bootstrapped. E.g.,
> s/already_bootstrapped/check_for_preexisting/ or something. From the
local
> function perspective, you could even argue that you should only
control
> disabling the behavior, since this is an optimization--e.g.,
> s/already_bootstrapped/not prevent_preexisting_check/--but that's
effectively a
> double negative. In the end, I'm not suggesting this because I don't
like any
> of the other concrete options I considered. At best, this would have
been
> bikeshedding, anyway.

Done.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode187
quickstart/app.py:187: print('charm URL: {}'.format(charm_url))
On 2013/11/18 15:02:19, gary.poster wrote:
> Maybe we should warn the user if the actual charm_url != the given
charm_url,
> and warn louder if the charm id doesn't match /\/juju-gui-\d+$/ ?

We may not have the actual charm URL here, unless the user explicitly
passed it as an option. The charm URL is retrieved from charmworld only
if an existing service is not found. The regexp check is really a good
suggestion. I'll work on this on another branch.

https://codereview.appspot.com/28250044/