Improve charm URL handling.

Improve charm URL handling.

Validate the user provided GUI charm URL.
Also add a missing test for complete coverage 
(obsessive-compulsive mode on).

Tests: `make check`.

QA:

1)
Provide invalid charm URLs, the program 
should immediately stop the execution
and exit with an error, e.g.:

.venv/bin/python juju-quickstart --gui-charm-url invalid

.venv/bin/python juju-quickstart --gui-charm-url local:precise/juju-gui-80

.venv/bin/python juju-quickstart --gui-charm-url http:~juju-gui/precise/juju-gui-80

.venv/bin/python juju-quickstart --gui-charm-url cs:precise/juju-gui-1 bundle:~jorge/mediawiki-simple/4/mediawiki-simple

.venv/bin/python juju-quickstart --gui-charm-url cs:saucy/juju-gui-80


2)
Run the program passing a customized charm URL, e.g.:

.venv/bin/python juju-quickstart --gui-charm-url cs:~juju-gui/precise/juju-gui-128

You should see the "using a customized juju-gui charm" warning
printed during the service deployment step.

Re-execute the command above: quickstart should reuse the
service in the environment and the warning is printed again.

Destroy the environment.


3)
Now manually deploy an outdated version of the GUI charm:

(sudo) juju bootstrap
juju deploy cs:precise/juju-gui-79

Run quickstart:
.venv/bin/python juju-quickstart

You should see the "charm is outdated" warning, then quickstart
waits for the outdated GUI to be deployed and ready.

Destroy the environment.


4)
Run quickstart normally:
.venv/bin/python juju-quickstart
The last official GUI charm (cs:precise/juju-gui-80)
is installed and no warnings are logged.

Destroy the environment.
Done, thank you!

https://code.launchpad.net/~frankban/juju-quickstart/charm-url-warning/+merge/196244

(do not edit description out of merge proposal)

[frankban, 2013-11-22 09:56:59]

Please take a look.

[rharding, 2013-11-22 13:47:33]

I'm worried about the dupe code. We've got another project doing charm parsing
and such. This is done in the juju store and charmworld. I wonder if we can
cheat and check if the charm can be found via a network connection to either of
those stores rather than dupe the code. 

The hard coded series support just seems like a land mine to forget to update
the deployer and then get bug reports down the road. Maybe we can at least put
in precise/trusty now and file a bug on the charm we need to test and get it
into trusty?

The code looks ok with a couple of notes below, but I'd like to discuss if
there's a lighter maintenance way to go.

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

https://codereview.appspot.com/30760043/diff/1/quickstart/app.py#newcode236
quickstart/app.py:236: utils.check_gui_charm_url(charm_url)
I'm a bit hmmm on the check method just being inline. Other things are doing
logging, raising exeptions, etc. I'd have expected this to check it and, based
on the response, take some logic to exit from here.

https://codereview.appspot.com/30760043/diff/1/quickstart/charms.py
File quickstart/charms.py (right):

https://codereview.appspot.com/30760043/diff/1/quickstart/charms.py#newcode29
quickstart/charms.py:29: def parse_url(url):
yuck, another place of duped logic for urls. Is there any way we can/should push
this up to charmworld?

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

https://codereview.appspot.com/30760043/diff/1/quickstart/manage.py#newcode110
quickstart/manage.py:110: if charm.series not in
settings.JUJU_GUI_SUPPORTED_SERIES:
is settings coming from juju or is there another copy of the list in here?

https://codereview.appspot.com/30760043/diff/1/quickstart/manage.py#newcode112
quickstart/manage.py:112: if (
should this be moved to some _checkIsValidBundle method?

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

https://codereview.appspot.com/30760043/diff/1/quickstart/tests/test_app.py#n...
quickstart/tests/test_app.py:651: # An existing but unexpected charm URL is
correctly found and logged.
Is there a test for a non-precise charm?

https://codereview.appspot.com/30760043/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/30760043/diff/1/quickstart/utils.py#newcode66
quickstart/utils.py:66: def check_gui_charm_url(charm_url):
I see, it's just logging. I still thing it reads strange in app to check and
then do nothing with the check. Maybe it's a warn_invalid_charm_url method?

[rharding, 2013-11-22 14:31:34]

Per irc chat, LGTM on the code. It'll be a nice day when we all share a single
proof library for that stuff. 

Will QA now.

[bac, 2013-11-22 14:31:50]

LGTM modulo small suggestions and Rick's concerns.  QA OK though I did not do
#3.

https://codereview.appspot.com/30760043/diff/1/quickstart/charms.py
File quickstart/charms.py (right):

https://codereview.appspot.com/30760043/diff/1/quickstart/charms.py#newcode29
quickstart/charms.py:29: def parse_url(url):
This function looks very complete.  But as Rick said it duplicates a lot of
functionality used by Charmworld.  Would be nice to consolidate them...but
where?

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

https://codereview.appspot.com/30760043/diff/1/quickstart/manage.py#newcode111
quickstart/manage.py:111: return parser.error('unsupported charm series:
{}'.format(charm))
If you just printed the charm.series it would be more consistent with other
error messages.

https://codereview.appspot.com/30760043/diff/1/quickstart/settings.py
File quickstart/settings.py (right):

https://codereview.appspot.com/30760043/diff/1/quickstart/settings.py#newcode41
quickstart/settings.py:41: # The series supported by the Juju GUI charm.
Since the word 'series' is both singular and plural, just for clarity I'd change
the above to say "The set of series..." to indicate in the future it may be more
than one.  May be overkill since you clearly define a tuple below...

[rharding, 2013-11-22 14:40:14]

QA on #3 is good.

[frankban, 2013-11-22 15:09:25]

*** Submitted:

Improve charm URL handling.

Validate the user provided GUI charm URL.
Also add a missing test for complete coverage 
(obsessive-compulsive mode on).

Tests: `make check`.

QA:

1)
Provide invalid charm URLs, the program 
should immediately stop the execution
and exit with an error, e.g.:

.venv/bin/python juju-quickstart --gui-charm-url invalid

.venv/bin/python juju-quickstart --gui-charm-url local:precise/juju-gui-80

.venv/bin/python juju-quickstart --gui-charm-url
http:~juju-gui/precise/juju-gui-80

.venv/bin/python juju-quickstart --gui-charm-url cs:precise/juju-gui-1
bundle:~jorge/mediawiki-simple/4/mediawiki-simple

.venv/bin/python juju-quickstart --gui-charm-url cs:saucy/juju-gui-80


2)
Run the program passing a customized charm URL, e.g.:

.venv/bin/python juju-quickstart --gui-charm-url
cs:~juju-gui/precise/juju-gui-128

You should see the "using a customized juju-gui charm" warning
printed during the service deployment step.

Re-execute the command above: quickstart should reuse the
service in the environment and the warning is printed again.

Destroy the environment.


3)
Now manually deploy an outdated version of the GUI charm:

(sudo) juju bootstrap
juju deploy cs:precise/juju-gui-79

Run quickstart:
.venv/bin/python juju-quickstart

You should see the "charm is outdated" warning, then quickstart
waits for the outdated GUI to be deployed and ready.

Destroy the environment.


4)
Run quickstart normally:
.venv/bin/python juju-quickstart
The last official GUI charm (cs:precise/juju-gui-80)
is installed and no warnings are logged.

Destroy the environment.
Done, thank you!

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

https://codereview.appspot.com/30760043/diff/1/quickstart/charms.py
File quickstart/charms.py (right):

https://codereview.appspot.com/30760043/diff/1/quickstart/charms.py#newcode29
quickstart/charms.py:29: def parse_url(url):
On 2013/11/22 14:31:51, bac wrote:
> This function looks very complete.  But as Rick said it duplicates a lot of
> functionality used by Charmworld.  Would be nice to consolidate them...but
> where?

Agreed. We discussed on IRC about the need for an external charms/bundles
validation library where to refactor this kind of code. Many projects can then
take advantage of such a library, e.g. quickstart, proof, deployer, guiserver,
charmworld...

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

https://codereview.appspot.com/30760043/diff/1/quickstart/manage.py#newcode110
quickstart/manage.py:110: if charm.series not in
settings.JUJU_GUI_SUPPORTED_SERIES:
On 2013/11/22 13:47:33, rharding wrote:
> is settings coming from juju or is there another copy of the list in here?

The list is defined in quickstart.

https://codereview.appspot.com/30760043/diff/1/quickstart/manage.py#newcode111
quickstart/manage.py:111: return parser.error('unsupported charm series:
{}'.format(charm))
On 2013/11/22 14:31:51, bac wrote:
> If you just printed the charm.series it would be more consistent with other
> error messages.

Done.

https://codereview.appspot.com/30760043/diff/1/quickstart/manage.py#newcode112
quickstart/manage.py:112: if (
On 2013/11/22 13:47:33, rharding wrote:
> should this be moved to some _checkIsValidBundle method?

+1 on creating another method when this condition is reused elsewhere.

https://codereview.appspot.com/30760043/diff/1/quickstart/settings.py
File quickstart/settings.py (right):

https://codereview.appspot.com/30760043/diff/1/quickstart/settings.py#newcode41
quickstart/settings.py:41: # The series supported by the Juju GUI charm.
On 2013/11/22 14:31:51, bac wrote:
> Since the word 'series' is both singular and plural, just for clarity I'd
change
> the above to say "The set of series..." to indicate in the future it may be
more
> than one.  May be overkill since you clearly define a tuple below...

Done.

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

https://codereview.appspot.com/30760043/diff/1/quickstart/tests/test_app.py#n...
quickstart/tests/test_app.py:651: # An existing but unexpected charm URL is
correctly found and logged.
On 2013/11/22 13:47:33, rharding wrote:
> Is there a test for a non-precise charm? 

An existing non-precise charm is allowed in this context. Allowed series
validation is done elsewhere and it's tested.

[frankban, 2013-11-22 15:14:02]

Hey Rick and Brad,
thank you for the great suggestions in you reviews!