state: update SetEnvironConfig to take new and old

state: update SetEnvironConfig to take new and old

SetEnvironConfig is updated to take both new and old
configurations, and a delta computed from these. The
only functional change for now is that attributes that
exist in old but not in new are now deleted from state.

To do this right, we should also check that the
settings in state are the same as the "old" config,
so the delta computed is what the user expects. I have
not done this part yet, as it is a fairly big change
and I am not confident I understand all of the
repercussions.

https://code.launchpad.net/~axwalk/juju-core/setenvironconfig-delta/+merge/197989

(do not edit description out of merge proposal)

[axw, 2013-12-06 04:56:07]

Please take a look.

[rog, 2013-12-06 08:46:22]

LGTM with one thought below. Thanks!

https://codereview.appspot.com/38180043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/38180043/diff/1/state/state.go#newcode272
state/state.go:272: settings.Update(newattrs)
To do this right, shouldn't we only update
those settings that have changed between old
and new?

Otherwise if two people concurrently do:

 juju set-environment foo=a
 juju set-environment bar=b

then one of them may overwrite the other.

I suspect that the only way to do this correctly is
to have something like:

func (st *State) UpdateEnvironConfig(func (*config.Config) (*config.Config,
error)) error

which would retrieve the configuration from state, call the given
function to determine the new one and verify that it's valid, and apply the new
configuration to the state, checking that the settings have not changed in the
meantime.

I guess that's a bigger change though, and the change in this CL is at least a
significant improvement on what we had before, so I'm OK with it for now.

[axw, 2013-12-06 09:01:26]

Thanks for the review

https://codereview.appspot.com/38180043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/38180043/diff/1/state/state.go#newcode272
state/state.go:272: settings.Update(newattrs)
On 2013/12/06 08:46:23, rog wrote:
> To do this right, shouldn't we only update
> those settings that have changed between old
> and new?
> 
> Otherwise if two people concurrently do:
> 
>  juju set-environment foo=a
>  juju set-environment bar=b
> 
> then one of them may overwrite the other.

I don't think it will overwrite in that example, but I may be mistaken. Updates
and deletions are translated to $set/$unset transaction operations. Deletions
are only relative to the "old" config, which is known to the caller. In both the
cases you give, neither one will cause a deletion.

> I suspect that the only way to do this correctly is
> to have something like:
> 
> func (st *State) UpdateEnvironConfig(func (*config.Config) (*config.Config,
> error)) error
> 
> which would retrieve the configuration from state, call the given
> function to determine the new one and verify that it's valid, and apply the
new
> configuration to the state, checking that the settings have not changed in the
> meantime.
> 
> I guess that's a bigger change though, and the change in this CL is at least a
> significant improvement on what we had before, so I'm OK with it for now.

[axw, 2013-12-06 09:09:28]

On 2013/12/06 09:01:26, axw wrote:
> Thanks for the review
> 
> https://codereview.appspot.com/38180043/diff/1/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/38180043/diff/1/state/state.go#newcode272
> state/state.go:272: settings.Update(newattrs)
> On 2013/12/06 08:46:23, rog wrote:
> > To do this right, shouldn't we only update
> > those settings that have changed between old
> > and new?
> > 
> > Otherwise if two people concurrently do:
> > 
> >  juju set-environment foo=a
> >  juju set-environment bar=b
> > 
> > then one of them may overwrite the other.
> 
> I don't think it will overwrite in that example, but I may be mistaken.
Updates
> and deletions are translated to $set/$unset transaction operations. Deletions
> are only relative to the "old" config, which is known to the caller. In both
the
> cases you give, neither one will cause a deletion.

Responding to myself here: rog meant that we could overwrite foo/bar with an old
value, on account of us updating everything (Update(AllAttrs)).

> > I suspect that the only way to do this correctly is
> > to have something like:
> > 
> > func (st *State) UpdateEnvironConfig(func (*config.Config) (*config.Config,
> > error)) error
> > 
> > which would retrieve the configuration from state, call the given
> > function to determine the new one and verify that it's valid, and apply the
> new
> > configuration to the state, checking that the settings have not changed in
the
> > meantime.
> > 
> > I guess that's a bigger change though, and the change in this CL is at least
a
> > significant improvement on what we had before, so I'm OK with it for now.