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

Revision history for this message
Richard Harding (rharding) wrote :

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

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

« Back to merge proposal