Fixes to upgrade infrastructure

Fixes to upgrade infrastructure

The initial upgrade framework was including
steps for the current version by mistake.
Additional tests also added.

https://code.launchpad.net/~wallyworld/juju-core/upgrade-framework-fixes/+merge/205897

(do not edit description out of merge proposal)

[wallyworld, 2014-02-12 05:03:46]

Please take a look.

[wallyworld, 2014-02-12 07:02:31]

Please take a look.

[wallyworld, 2014-02-12 07:25:44]

Please take a look.

[dimitern, 2014-02-12 08:55:05]

Please repropose the diff is screwed.

[wallyworld, 2014-02-12 09:07:29]

Please take a look.

[dimitern, 2014-02-12 09:30:01]

LGTM

https://codereview.appspot.com/61510049/diff/60001/upgrades/upgrade.go
File upgrades/upgrade.go (right):

https://codereview.appspot.com/61510049/diff/60001/upgrades/upgrade.go#newcode16
upgrades/upgrade.go:16: // Description is a human readable description of what
the upgrade step does.
Put some blank lines between the doc comment/method def pairs please.

https://codereview.appspot.com/61510049/diff/60001/upgrades/upgrade.go#newcode42
upgrades/upgrade.go:42: // StateServer is a machine participating in a Juju
state server cluster.
Here as well - blank lines between.

[rog, 2014-02-13 09:26:29]

A nice direction, but could be a tad simpler, I think.

https://codereview.appspot.com/61510049/diff/60001/upgrades/upgrade.go
File upgrades/upgrade.go (right):

https://codereview.appspot.com/61510049/diff/60001/upgrades/upgrade.go#newcode15
upgrades/upgrade.go:15: type UpgradeStep interface {
Given that this is the upgrades package, perhaps
"Step" would be an appropriate name and saves
stuttering ("upgrades.Step"
reads quite well). Similarly, Operation instead 
of UpgradeOperation and Target instead
of UpgradeTarget, below.

https://codereview.appspot.com/61510049/diff/60001/upgrades/upgrade.go#newcode65
upgrades/upgrade.go:65: type Context interface {
I don't really see the point of defining both this interface and
the UpgradeContext struct together

I think that the struct would be sufficient for both
use cases - it's just a bag of stuff - we're not attaching behaviour to it.

Alternatively just define the Context interface here and
rely on clients to implement it.

Similarly for UpgradeStep and UpgradeOperation - they're essentially data
descriptions of stuff to do, and could easily be:

type UpgradeStep struct {
    Description string
    Targets []UpgradeTarget
    Run func(context Context) error
}

type UpgradeOperation struct {
    TargetVersion version.Number
    Steps []UpgradeStep
}

This would quite a bit of boilerplate code without
any particular loss of generality that I can see.

https://codereview.appspot.com/61510049/diff/60001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/61510049/diff/60001/version/version.go#newcode208
version/version.go:208: func (v Number) lessMaybeEqual(w Number, wantEqual bool)
bool {
Rather than this, how about a slightly more
general function?

// Compare returns -1, 0 or 1 depending
// on whether v is less than, equal to or greater
// than w.
func (v Number) Compare(w Number) int {
    if v == w {
        return 0
    }
    switch {
    case v.Major != w.Major:
        less = v.Major < w.Major
    case v.Minor != w.Minor:
        less = v.Minor < w.Minor
    case v.Patch != w.Patch:
        less = v.Patch < w.Patch
    case v.Build != w.Build:
        less = v.Build < w.Build
    }
    if less {
        return -1
    }
    return 1
}

func (v Number) Less(w Number) bool {
    return v.Compare(w) < 0
}

?

Then we can use v.Compare(w) <= 0 instead
of LessEqual, etc.

bytes.Compare is precedent for this.

https://codereview.appspot.com/61510049/diff/60001/version/version.go#newcode208
version/version.go:208: func (v Number) lessMaybeEqual(w Number, wantEqual bool)
bool {
Rather than this, how about:

// Compare returns -1, 0 or 1 depending
// on whether v is less than, equal to or greater
// than w.
func (v Number) Compare(w Number) int {

    switch {
    case v.Major != w.Major:
        return v.Major < w.Major
    case v.Minor != w.Minor:
        return v.Minor < w.Minor
    case v.Patch != w.Patch:
        return v.Patch < w.Patch
    case v.Build != w.Build:
        return v.Build < w.Build
    }
    return wantEqual
}