state: validate SetStatus inputs

state: validate SetStatus inputs



https://code.launchpad.net/~fwereade/juju-core/state-validate-status/+merge/177616

(do not edit description out of merge proposal)

[fwereade, 2013-07-30 14:33:06]

Please take a look.

[mue, 2013-07-30 16:28:41]

LGTM

[gz, 2013-07-31 13:16:21]

LGTM

[rog, 2013-07-31 13:25:13]

LGTM with a couple of trivial thoughts.

https://codereview.appspot.com/12105043/diff/1/state/api/params/constants.go
File state/api/params/constants.go (right):

https://codereview.appspot.com/12105043/diff/1/state/api/params/constants.go#...
state/api/params/constants.go:57: case StatusPending, StatusInstalled,
StatusStarted:

I'm mot quite so keen on the irregularity here.

perhaps:


func (status Status) Valid() bool { 
   switch status {
   case
        StatusPending,
        StatusInstalled,
        etc,
        StatusError:
        return true
   }
   return false
}

or even:

var validStatus = map[Status]bool {
    StatusPending: true,
    etc
}

func (status Status) Valid() bool {
     return validStatus[status]
}

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

https://codereview.appspot.com/12105043/diff/1/state/status.go#newcode28
state/status.go:28: return fmt.Errorf("cannot set invalid status %q",
doc.Status)

Who's to say that statusDoc.validate is always to be called in a "set-status"
context?

I'd just return "invalid status %q" (and below)

https://codereview.appspot.com/12105043/diff/1/state/status.go#newcode35
state/status.go:35: return fmt.Errorf("cannot set status %q without info",
doc.Status)
"status %q has no associated information"
?

[fwereade, 2013-07-31 14:23:47]

Please take a look.

https://codereview.appspot.com/12105043/diff/1/state/api/params/constants.go
File state/api/params/constants.go (right):

https://codereview.appspot.com/12105043/diff/1/state/api/params/constants.go#...
state/api/params/constants.go:57: case StatusPending, StatusInstalled,
StatusStarted:
On 2013/07/31 13:25:13, rog wrote:
> 
> I'm mot quite so keen on the irregularity here.
> 
> perhaps:
> 
> 
> func (status Status) Valid() bool { 
>    switch status {
>    case
>         StatusPending,
>         StatusInstalled,
>         etc,
>         StatusError:
>         return true
>    }
>    return false
> }
> 
> or even:
> 
> var validStatus = map[Status]bool {
>     StatusPending: true,
>     etc
> }
> 
> func (status Status) Valid() bool {
>      return validStatus[status]
> }

Done.

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

https://codereview.appspot.com/12105043/diff/1/state/status.go#newcode28
state/status.go:28: return fmt.Errorf("cannot set invalid status %q",
doc.Status)
On 2013/07/31 13:25:13, rog wrote:
> 
> Who's to say that statusDoc.validate is always to be called in a "set-status"
> context?
> 
> I'd just return "invalid status %q" (and below)

changed the name to validateSet to make it clear that it is. Then that reminded
me that StatusDown is also invalid for set, so I fixed that.

https://codereview.appspot.com/12105043/diff/1/state/status.go#newcode35
state/status.go:35: return fmt.Errorf("cannot set status %q without info",
doc.Status)
On 2013/07/31 13:25:13, rog wrote:
> "status %q has no associated information"
> ?

Keeping as is now "set" context is explicit in name.

[rog, 2013-07-31 15:26:16]

still LGTM

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

https://codereview.appspot.com/12105043/diff/1/state/status.go#newcode28
state/status.go:28: return fmt.Errorf("cannot set invalid status %q",
doc.Status)
On 2013/07/31 14:23:47, fwereade wrote:
> On 2013/07/31 13:25:13, rog wrote:
> > 
> > Who's to say that statusDoc.validate is always to be called in a
"set-status"
> > context?
> > 
> > I'd just return "invalid status %q" (and below)
> 
> changed the name to validateSet to make it clear that it is. Then that
reminded
> me that StatusDown is also invalid for set, so I fixed that.

cool.

https://codereview.appspot.com/12105043/diff/9001/state/api/params/constants.go
File state/api/params/constants.go (right):

https://codereview.appspot.com/12105043/diff/9001/state/api/params/constants....
state/api/params/constants.go:64: default:
trivial i know, but if you return true above, you won't need a default clause.