uniter: fix config-changed ordering

uniter: fix config-changed ordering

see lp:1091357

https://code.launchpad.net/~fwereade/juju-core/config-changed-ordering/+merge/140266

(do not edit description out of merge proposal)

[fwereade, 2012-12-17 17:57:15]

Please take a look.

[rog, 2012-12-17 18:14:35]

LGTM assuming you've actually tested this live against some charm that expects
this behaviour.

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode147
worker/uniter/modes.go:147: func ModeConfigChanged(u *Uniter) (next Mode, err
error) {
it'd be nice if this stayed in the same place to make the diffs smaller.

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode205
worker/uniter/modes.go:205: return nil, fmt.Errorf("insane uniter state: %#v",
u.s)
i know this is current behaviour, but it might be nice if it printed that it
expects Continue, or at least which mode we're in (i can imaging seeing this
message in a log file and having no idea where it came from)

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode302
worker/uniter/modes.go:302: return nil, fmt.Errorf("insane uniter state: %#v",
u.s)
as above.

https://codereview.appspot.com/6946071/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6946071/diff/1/worker/uniter/uniter.go#newcode157
worker/uniter/uniter.go:157: CharmURL: url,
Started: op == RunHook && hi.Kind == hook.Start || u.s != nil && u.s.Started

and delete 5 lines below?

[fwereade, 2012-12-17 19:53:59]

Yeah, discovered it today with the current wordpress :/.

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode147
worker/uniter/modes.go:147: func ModeConfigChanged(u *Uniter) (next Mode, err
error) {
On 2012/12/17 18:14:36, rog wrote:
> it'd be nice if this stayed in the same place to make the diffs smaller.

But leaving it where it was degrades the sane grouping of modes...

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode205
worker/uniter/modes.go:205: return nil, fmt.Errorf("insane uniter state: %#v",
u.s)
On 2012/12/17 18:14:36, rog wrote:
> i know this is current behaviour, but it might be nice if it printed that it
> expects Continue, or at least which mode we're in (i can imaging seeing this
> message in a log file and having no idea where it came from)

It'll come out as "ModeAbide: insane uniter state": etc, which I think is good
enough?

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode302
worker/uniter/modes.go:302: return nil, fmt.Errorf("insane uniter state: %#v",
u.s)
On 2012/12/17 18:14:36, rog wrote:
> as above.
as above :)

https://codereview.appspot.com/6946071/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6946071/diff/1/worker/uniter/uniter.go#newcode157
worker/uniter/uniter.go:157: CharmURL: url,
On 2012/12/17 18:14:36, rog wrote:
> Started: op == RunHook && hi.Kind == hook.Start || u.s != nil && u.s.Started
> 
> and delete 5 lines below?

The aesthetics of that bother me a little; I hope the replacement finds favour
:).

[fwereade, 2012-12-17 20:28:10]

On 2012/12/17 19:53:59, fwereade wrote:
> > and delete 5 lines below?
> 
> The aesthetics of that bother me a little; I hope the replacement finds favour
> :).

Heh, changed my mind, yours is better.

[fwereade, 2012-12-17 21:11:01]

Please take a look.

[dimitern, 2012-12-18 15:14:44]

On 2012/12/17 21:11:01, fwereade wrote:
> Please take a look.

LGTM

[fwereade, 2012-12-18 15:21:40]

*** Submitted:

uniter: fix config-changed ordering

see lp:1091357

R=rog, dimitern
CC=
https://codereview.appspot.com/6946071

[rog, 2012-12-18 15:41:17]

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode205
worker/uniter/modes.go:205: return nil, fmt.Errorf("insane uniter state: %#v",
u.s)
On 2012/12/17 19:53:59, fwereade wrote:
> On 2012/12/17 18:14:36, rog wrote:
> > i know this is current behaviour, but it might be nice if it printed that it
> > expects Continue, or at least which mode we're in (i can imaging seeing this
> > message in a log file and having no idea where it came from)
> 
> It'll come out as "ModeAbide: insane uniter state": etc, which I think is good
> enough?

yeah, that seems fine.

https://codereview.appspot.com/6946071/diff/1/worker/uniter/modes.go#newcode302
worker/uniter/modes.go:302: return nil, fmt.Errorf("insane uniter state: %#v",
u.s)
On 2012/12/17 19:53:59, fwereade wrote:
> On 2012/12/17 18:14:36, rog wrote:
> > as above.
> as above :)
as above :)

https://codereview.appspot.com/6946071/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6946071/diff/1/worker/uniter/uniter.go#newcode157
worker/uniter/uniter.go:157: CharmURL: url,
On 2012/12/17 19:53:59, fwereade wrote:
> On 2012/12/17 18:14:36, rog wrote:
> > Started: op == RunHook && hi.Kind == hook.Start || u.s != nil && u.s.Started
> > 
> > and delete 5 lines below?
> 
> The aesthetics of that bother me a little; I hope the replacement finds favour
> :).

i'm wondering what the intermediate stage was now!