Code review comment for lp:~frankban/juju-quickstart/charm-url-warning

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

https://codereview.appspot.com/30760043/