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