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#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.
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 app.py: 236: utils.check_ gui_charm_ url(charm_ url)
quickstart/
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 charms. py (right):
File quickstart/
https:/ /codereview. appspot. com/30760043/ diff/1/ quickstart/ charms. py#newcode29 charms. py:29: def parse_url(url):
quickstart/
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 manage. py (right):
File quickstart/
https:/ /codereview. appspot. com/30760043/ diff/1/ quickstart/ manage. py#newcode110 manage. py:110: if charm.series not in JUJU_GUI_ SUPPORTED_ SERIES:
quickstart/
settings.
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 manage. py:112: if (
quickstart/
should this be moved to some _checkIsValidBundle method?
https:/ /codereview. appspot. com/30760043/ diff/1/ quickstart/ tests/test_ app.py tests/test_ app.py (right):
File quickstart/
https:/ /codereview. appspot. com/30760043/ diff/1/ quickstart/ tests/test_ app.py# newcode651 tests/test_ app.py: 651: # An existing but unexpected charm URL
quickstart/
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 utils.py: 66: def check_gui_ charm_url( charm_url) : charm_url
quickstart/
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_
method?
https:/ /codereview. appspot. com/30760043/