Code review comment for lp:~clint-fewbar/pyjuju/namespace-from-env

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for sticking with this. I've included some feedback.

https://codereview.appspot.com/5695056/diff/3001/juju/charm/tests/test_url.py
File juju/charm/tests/test_url.py (left):

https://codereview.appspot.com/5695056/diff/3001/juju/charm/tests/test_url.py#oldcode126
juju/charm/tests/test_url.py:126:
All the tests assume the env is set to a proper value, test the case
where it is not.

https://codereview.appspot.com/5695056/diff/3001/juju/charm/url.py
File juju/charm/url.py (right):

https://codereview.appspot.com/5695056/diff/3001/juju/charm/url.py#newcode131
juju/charm/url.py:131: default_schema, rest =
os.environ.get("JUJU_DEFAULT_NS","cs:").split(":",1)
I think this module is too low level to be interacting with the
environment like this. The juju/control layer should consume environment
information and can supply information. Those (deploy and upgrade-charm)
use the repository.resolve function which can take additional arguments
for this (and handle errors in a place where we can inform the user they
set the variable incorrectly. Resolve can take the parsed output of the
environment and pass it to infer. juju/control can even use a util
method from here to help parse the argument but that parsing has to be
more solid that what is here now.

Currently a bad value, missing a ":" for example, will result in a
ValueError when trying to unpack on the left-hand-side. Need to be more
defensive than that.

https://codereview.appspot.com/5695056/

« Back to merge proposal