state: get/set environ constraints

state: get/set environ constraints



https://code.launchpad.net/~fwereade/juju-core/state-environ-constraints/+merge/148942

(do not edit description out of merge proposal)

[fwereade, 2013-02-17 14:35:54]

Please take a look.

[dfc, 2013-02-18 02:16:05]

LGTM.

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

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode93
state/open.go:93: defer func() {
nice

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode101
state/open.go:101: if _, err = st.EnvironConfig(); err == nil {
if _, err := ...

[jameinel, 2013-02-18 09:32:32]

LGTM

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

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode101
state/open.go:101: if _, err = st.EnvironConfig(); err == nil {
On 2013/02/18 02:16:05, dfc wrote:
> if _, err := ...

Given the outer closure, don't you want to re-use the function-scoped 'err'
variable, rather than creating a new if-scoped one?

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode114
state/open.go:114: if err := st.runner.Run(ops, "", nil); err == txn.ErrAborted
{
Though here you *are* creating a locally defined 'err' object. So probably we
want to be consistent, but I'm not entirely sure of which William was thinking
it would be.

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

https://codereview.appspot.com/7307128/diff/1/state/state_test.go#newcode477
state/state_test.go:477: cons1, err := s.State.EnvironConstraints()
I'm a little confused as to why state.Initialize() is having a side effect on
s.State 
Is this because the Environment in the database is being updated?
Maybe a comment on the 'Initialize' line indicating we are depending on the side
effect for later parts of the test.
// Initialize a new state object to push the environment constraints into the
database

https://codereview.appspot.com/7307128/diff/1/state/state_test.go#newcode488
state/state_test.go:488: 
So SetEnvironConstraints completely replaces all constraints, correct? (There
isn't a function for 'add this constraint' yet)

Would we want a negative assertion that cons3 != cons?

[dimitern, 2013-02-18 10:23:29]

LGTM. I agree with some of jameinel's comments, and I have some questions as
well.

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

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode101
state/open.go:101: if _, err = st.EnvironConfig(); err == nil {
On 2013/02/18 09:32:32, jameinel wrote:
> On 2013/02/18 02:16:05, dfc wrote:
> > if _, err := ...
> 
> Given the outer closure, don't you want to re-use the function-scoped 'err'
> variable, rather than creating a new if-scoped one?

+1

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode114
state/open.go:114: if err := st.runner.Run(ops, "", nil); err == txn.ErrAborted
{
On 2013/02/18 09:32:32, jameinel wrote:
> Though here you *are* creating a locally defined 'err' object. So probably we
> want to be consistent, but I'm not entirely sure of which William was thinking
> it would be.

+1

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

https://codereview.appspot.com/7307128/diff/1/state/state_test.go#newcode488
state/state_test.go:488: 
On 2013/02/18 09:32:32, jameinel wrote:
> So SetEnvironConstraints completely replaces all constraints, correct? (There
> isn't a function for 'add this constraint' yet)
> 
> Would we want a negative assertion that cons3 != cons?

+1

[rog, 2013-02-18 14:03:43]

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

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode101
state/open.go:101: if _, err = st.EnvironConfig(); err == nil {
On 2013/02/18 09:32:32, jameinel wrote:
> On 2013/02/18 02:16:05, dfc wrote:
> > if _, err := ...
> 
> Given the outer closure, don't you want to re-use the function-scoped 'err'
> variable, rather than creating a new if-scoped one?

1) he _is_ using the function-scoped err variable.
2) it wouldn't matter anyway, because the closure is referring to the return
parameter, which is always assigned to before return, regardless of what's
returned.

personally i prefer to see local variables when possible, and in this case it is
possible . i'd use := (consistent with the Run call below) but it's really a
matter of taste.

[fwereade, 2013-02-20 13:00:52]

*** Submitted:

state: get/set environ constraints

R=dfc, jameinel, dimitern, rog
CC=
https://codereview.appspot.com/7307128

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

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode93
state/open.go:93: defer func() {
On 2013/02/18 02:16:05, dfc wrote:
> nice
cheers :)

https://codereview.appspot.com/7307128/diff/1/state/open.go#newcode114
state/open.go:114: if err := st.runner.Run(ops, "", nil); err == txn.ErrAborted
{
On 2013/02/18 10:23:29, dimitern wrote:
> On 2013/02/18 09:32:32, jameinel wrote:
> > Though here you *are* creating a locally defined 'err' object. So probably
we
> > want to be consistent, but I'm not entirely sure of which William was
thinking
> > it would be.
> 
> +1

Wrt the above, it doesn't really matter which I do; I'll go with locally scoped
errs, because they're really not used by the rest of the func (the err seen in
the defer is always the named "err" return, to which any returned error will
have been assigned before the code runs).

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

https://codereview.appspot.com/7307128/diff/1/state/state_test.go#newcode477
state/state_test.go:477: cons1, err := s.State.EnvironConstraints()
On 2013/02/18 09:32:32, jameinel wrote:
> I'm a little confused as to why state.Initialize() is having a side effect on
> s.State 
> Is this because the Environment in the database is being updated?
> Maybe a comment on the 'Initialize' line indicating we are depending on the
side
> effect for later parts of the test.
> // Initialize a new state object to push the environment constraints into the
> database

I thought that was maybe covered by the "Environ constraints are not available
before initialization" comment above?

https://codereview.appspot.com/7307128/diff/1/state/state_test.go#newcode488
state/state_test.go:488: 
On 2013/02/18 09:32:32, jameinel wrote:
> So SetEnvironConstraints completely replaces all constraints, correct? (There
> isn't a function for 'add this constraint' yet)

Yeah, that was how it was in python... maybe set should work more like add, or
maybe we should offer both ops, but for now, and in the absence of anyone having
directly complained about the python way, this is deliberate.

> Would we want a negative assertion that cons3 != cons?

Good idea, thanks.