Make quickstart idempotent.

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.

https://code.launchpad.net/~frankban/juju-quickstart/idempotent-feature/+merge/195592

(do not edit description out of merge proposal)

[frankban, 2013-11-18 12:22:24]

Please take a look.

[gary.poster, 2013-11-18 15:02:19]

LGTM with a few trivials and a small.  QA good.  Very nice functionality.  Thank
you!

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

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
typo: ...rather than...

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode82
quickstart/app.py:82: already_bootstrapped = True
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.

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.
After reading docstring, I am not clear on three things.

(1) why is the provider type important here?
(2) why is the already_bootstrapped value important here?
(3) what happens if the GUI charm exists with a different name?  what happens if
the given name is not a GUI charm?

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:
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.

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode187
quickstart/app.py:187: print('charm URL: {}'.format(charm_url))
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+$/ ?

https://codereview.appspot.com/28250044/diff/1/quickstart/app.py#newcode199
quickstart/app.py:199: # bootstrap node if we are using the local provider.
Ah-ha!

[gary.poster, 2013-11-18 15:04:26]

One QA note.

The end of your instructions said this:

--------------------------
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").
--------------------------

I saw it on machine 0 (ec2), which I expected given the immediately previous
instruction to destroy the service.  If this is in fact a surprise and
broken...I reported it. :-)

[rharding, 2013-11-18 15:42:55]

LGTM and qa-ok. 

I'm with Gary on the is_bootstrapped being a bit confusing sometimes in the
inner workings. 

I also get a bit confused with the 'charm_url' as I forget that in this context
that's ALWAYS the Gui charm. As features grow and things like deploying bundles
come into the picture, I wonder if a s/charm_url/gui_charm_url might be in
order.

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#newcode74
quickstart/app.py:74: # jens files seems to be an internal juju-core detail.
Definitely we
jenv

[frankban, 2013-11-18 17:17:35]

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

[frankban, 2013-11-18 18:03:37]

Thank you both for the great reviews!