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.
Thanks for sticking with this. I've included some feedback.
https:/ /codereview. appspot. com/5695056/ diff/3001/ juju/charm/ tests/test_ url.py tests/test_ url.py (left):
File juju/charm/
https:/ /codereview. appspot. com/5695056/ diff/3001/ juju/charm/ tests/test_ url.py# oldcode126 tests/test_ url.py: 126:
juju/charm/
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 url.py: 131: default_schema, rest = get("JUJU_ DEFAULT_ NS","cs: ").split( ":",1)
juju/charm/
os.environ.
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/