Code review comment for lp:~fwereade/pyjuju/env-constraints

LGTM, some minors on docs.

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_get.py
File juju/control/constraints_get.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_get.py#newcode51
juju/control/constraints_get.py:51: yield
esm.set_config_state(env_config, environment.name)
so why are we syncing env on get-constraints.. the constraints are
already actualized on the object being inspected. if its a legacy
environment we should just log warning and exit.

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_set.py
File juju/control/constraints_set.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_set.py#newcode90
juju/control/constraints_set.py:90: yield
esm.set_config_state(env_config, environment.name)
Some comments on the reasoning for this here and in get-constraints
would be good.

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstrap.py
File juju/control/tests/test_bootstrap.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstrap.py#newcode7
juju/control/tests/test_bootstrap.py:7: from juju.lib.mocker import ANY
not needed.

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initialize.py
File juju/control/tests/test_initialize.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initialize.py#newcode41
juju/control/tests/test_initialize.py:41: def test_bad_data(self):
general comment, its really nice to have doc strings on tests for other
devs, even if its just a single line to express intent, and assuming the
test name itself isn't obvious. ie test_bad_constraints_data or
this is a test that the admin initialize exits 1 on bad constraints
data.

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provision.py
File juju/agents/tests/test_provision.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provision.py#newcode56
juju/agents/tests/test_provision.py:56: reactor.callLater(0.3,
self.push_default_config, False)
nitpick, please use the with_constraints=False invocation, its a bit
easier to grok what the value is doing without hunting for the method
def.

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_get.py
File juju/control/constraints_get.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_get.py#newcode57
juju/control/constraints_get.py:57: entity = yield
msm.get_machine_state(name)
what do you think an info log message describing the entity for which
the constraints are being retrieved.

ie. log.info("Fetching constraints for service %s", entity)
log.info("Fetching constriants for environment")

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_set.py
File juju/control/constraints_set.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_set.py#newcode90
juju/control/constraints_set.py:90: yield
esm.set_config_state(env_config, environment.name)
i'd appreciate if you could document the rationale for syncing as
applying to constraints, perhaps in an internals doc, referenced by the
relevant code points. This will make it much easier to keep the code
concerns in mind when the sync goes away.

https://codereview.appspot.com/5957043/diff/3001/juju/state/environment.py
File juju/state/environment.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/state/environment.py#newcode49
juju/state/environment.py:49: provider =
config.get_default().get_machine_provider()
hmm.. this is a red flag, ie the only callers here should be privileged
things (cli or provisioning agent), a doc string on the method would be
good.

https://codereview.appspot.com/5957043/