environs/agent: add Change function

environs/agent: add Change function

This enables independent workers to make
changes to the config file without clashing.

https://code.launchpad.net/~rogpeppe/juju-core/189-agent-change/+merge/143087

(do not edit description out of merge proposal)

[rog, 2013-01-14 11:41:23]

Please take a look.

[fwereade, 2013-01-14 11:47:54]

Nice, LGTM.

[dimitern, 2013-01-14 12:31:42]

LGTM

[TheMue, 2013-01-14 13:39:31]

LGTM, nice.

[rog, 2013-01-14 13:46:34]

*** Submitted:

environs/agent: add Change function

This enables independent workers to make
changes to the config file without clashing.

R=fwereade, dimitern, TheMue
CC=
https://codereview.appspot.com/7096054

[niemeyer, 2013-01-15 19:09:06]

https://codereview.appspot.com/7096054/diff/7001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7096054/diff/7001/environs/agent/agent.go#newc...
environs/agent/agent.go:153: func (c *Conf) Change(mutate func(conf *Conf)
error) error {
A few late comments on this one:

1) Not sure if a method is the right choice, given that it completely ignores
what c is, and overrides it no matter what. A function next to ReadConf
(ChangeConf?) might be more appropriate. If there's value in seeing the
end-result of the mutation, it can return the final value instead.

2) It must definitely comment that it's racy across processes, since the doc
makes it sound like it's safe.

3) That's more about Conf itself: why isn't Write checking that the
configuration is correct, instead of requiring an explicit call on Check, taking
in account that we wouldn't like a broken configuration.

[rog, 2013-01-16 14:57:55]

https://codereview.appspot.com/7096054/diff/7001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7096054/diff/7001/environs/agent/agent.go#newc...
environs/agent/agent.go:153: func (c *Conf) Change(mutate func(conf *Conf)
error) error {
On 2013/01/15 19:09:06, niemeyer wrote:
> A few late comments on this one:
> 
> 1) Not sure if a method is the right choice, given that it completely ignores
> what c is, and overrides it no matter what. A function next to ReadConf
> (ChangeConf?) might be more appropriate. If there's value in seeing the
> end-result of the mutation, it can return the final value instead.
> 
> 2) It must definitely comment that it's racy across processes, since the doc
> makes it sound like it's safe.
> 
> 3) That's more about Conf itself: why isn't Write checking that the
> configuration is correct, instead of requiring an explicit call on Check,
taking
> in account that we wouldn't like a broken configuration.

i think i'll delete the Change method for now, given the issues with it. i only
need it in one place, and i can do the same thing externally there.

with regard to 3), Write *is* checking that the configuration is correct unless
i'm missing something stupid.