Adopted needs-upgrade change of Python code

Adopted needs-upgrade change of Python code

The behavior of the current Python version of the needs-upgrade
flag has been modified. It now maintains an internal force flag
that can by set from the command line. The Go Unit methods as
well as the NeedsUpgradeWatcher now handles this flag too.

https://code.launchpad.net/~themue/juju/go-state-unit-needs-upgrade-with-force/+merge/103486

(do not edit description out of merge proposal)

[TheMue, 2012-04-25 14:29:06]

Please take a look.

[niemeyer, 2012-04-25 15:04:40]

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

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode248
state/unit.go:248: return &NeedsUpgrade{false, false}, nil
return &NeedsUpgrade{}, nil

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode251
state/unit.go:251: return &NeedsUpgrade{false, false}, err
return nil, err

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode257
state/unit.go:257: return &NeedsUpgrade{false, false}, err
return nil, err

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode281
state/unit.go:281: return "", fmt.Errorf("unit upgrade already enabled: %q",
u.Name())
"upgrade already enabled for unit %q"

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode562
state/unit.go:562: var setting struct {
This is the third time this type is defined. It would be better handled as a
proper type, defined above the two relevant functions above:

type needsUpgradeNode struct {
    Force bool
}

and used in all three places.

[TheMue, 2012-04-25 15:30:08]

Please take a look.

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

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode248
state/unit.go:248: return &NeedsUpgrade{false, false}, nil
On 2012/04/25 15:04:40, niemeyer wrote:
> return &NeedsUpgrade{}, nil

Done.

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode251
state/unit.go:251: return &NeedsUpgrade{false, false}, err
On 2012/04/25 15:04:40, niemeyer wrote:
> return nil, err

Done.

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode257
state/unit.go:257: return &NeedsUpgrade{false, false}, err
On 2012/04/25 15:04:40, niemeyer wrote:
> return nil, err

Done.

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode281
state/unit.go:281: return "", fmt.Errorf("unit upgrade already enabled: %q",
u.Name())
On 2012/04/25 15:04:40, niemeyer wrote:
> "upgrade already enabled for unit %q"

Done.

https://codereview.appspot.com/6111053/diff/1/state/unit.go#newcode562
state/unit.go:562: var setting struct {
On 2012/04/25 15:04:40, niemeyer wrote:
> This is the third time this type is defined. It would be better handled as a
> proper type, defined above the two relevant functions above:
> 
> type needsUpgradeNode struct {
>     Force bool
> }
> 
> and used in all three places.

Done.

[fwereade, 2012-04-26 14:16:05]

https://codereview.appspot.com/6111053/diff/4001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6111053/diff/4001/state/state_test.go#newcode836
state/state_test.go:836: // Can't be set multipe with different force flag.
(TODO)
Why not?

https://codereview.appspot.com/6111053/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6111053/diff/4001/state/unit.go#newcode271
state/unit.go:271: if oldYaml == "" {
I don't get why we're even checking oldYaml. Shouldn't we always just blat the
desired state over whatever was there before?

https://codereview.appspot.com/6111053/diff/4001/state/watch_test.go
File state/watch_test.go (left):

https://codereview.appspot.com/6111053/diff/4001/state/watch_test.go#oldcode94
state/watch_test.go:94: time.Sleep(50 * time.Millisecond)
This style of testing feels very opaque to me. Maybe consider unrolling the loop
and interspersing the sets/clears with the desired checks?

[fwereade, 2012-04-26 14:28:25]

Agreed on IRC current behaviour matches python and is fine.

[niemeyer, 2012-04-26 14:28:53]

LGTM, thanks.

[fwereade, 2012-04-26 14:54:32]

LGTM

https://codereview.appspot.com/6111053/diff/4001/state/watch_test.go
File state/watch_test.go (left):

https://codereview.appspot.com/6111053/diff/4001/state/watch_test.go#oldcode94
state/watch_test.go:94: time.Sleep(50 * time.Millisecond)
On 2012/04/26 14:16:06, fwereade wrote:
> This style of testing feels very opaque to me. Maybe consider unrolling the
loop
> and interspersing the sets/clears with the desired checks?

Agreed in IRC more suitable for a followup branch.

[TheMue, 2012-04-26 15:12:53]

*** Submitted:

Adopted needs-upgrade change of Python code

The behavior of the current Python version of the needs-upgrade
flag has been modified. It now maintains an internal force flag
that can by set from the command line. The Go Unit methods as
well as the NeedsUpgradeWatcher now handles this flag too.

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/6111053

https://codereview.appspot.com/6111053/diff/4001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6111053/diff/4001/state/state_test.go#newcode836
state/state_test.go:836: // Can't be set multipe with different force flag.
(TODO)
On 2012/04/26 14:16:06, fwereade wrote:
> Why not?

Discussed in IRC, stays as implemented in Py.

https://codereview.appspot.com/6111053/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6111053/diff/4001/state/unit.go#newcode271
state/unit.go:271: if oldYaml == "" {
On 2012/04/26 14:16:06, fwereade wrote:
> I don't get why we're even checking oldYaml. Shouldn't we always just blat the
> desired state over whatever was there before?

It has to be empty or in a valid state. This way we ensure it and not just
ignore it like a simple writing would do.