Merge lp:~rogpeppe/juju-core/502-upgrades-simplify into lp:~go-bot/juju-core/trunk

[Roger Peppe]

upgrades: remove unnecessary indirection layer

The upgrade description is just data - it doesn't
need any behaviour associated with it, so it's
sufficient, simpler and more direct to just use
structs where we can, making the code easier
to understand and shorter.

https://codereview.appspot.com/63530048/

[Roger Peppe (rogpeppe)]

Reviewers: mp+206361_code.launchpad.net,

Message:
Please take a look.

Description:
upgrades: remove unnecessary indirection layer

The upgrade description is just data - it doesn't
need any behaviour associated with it, so it's
sufficient, simpler and more direct to just use
structs where we can, making the code easier
to understand and shorter.

https://code.launchpad.net/~rogpeppe/juju-core/502-upgrades-simplify/+merge/206361

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/63530048/

Affected files (+137, -196 lines):
   A [revision details]
   M upgrades/lockdirectory.go
   M upgrades/lockdirectory_test.go
   M upgrades/operations.go
   M upgrades/rsyslogconf.go
   M upgrades/rsyslogconf_test.go
   M upgrades/steps118.go
   M upgrades/upgrade.go
   M upgrades/upgrade_test.go

[William Reade (fwereade)]

AFAICS, this just makes the code harder to change. Opinions may
reasonably differ re likelihood of changes that'd cause the interface
implementation to pay off, but IME the aggregated cost of designing for
change is trivial compared to the cost of failing to do so, and so a CL
that just removes flexibility does not lgtm.

https://codereview.appspot.com/63530048/

[Roger Peppe (rogpeppe)]

On 17 February 2014 10:34, <email address hidden> wrote:
> AFAICS, this just makes the code harder to change. Opinions may
> reasonably differ re likelihood of changes that'd cause the interface
> implementation to pay off, but IME the aggregated cost of designing for
> change is trivial compared to the cost of failing to do so, and so a CL
> that just removes flexibility does not lgtm.

For the record, my view is precisely the opposite in this case.
For methods that simply return some data, it is easier just
to have a field containing the data rather than an extra method
to call that returns the data and an interface type that encapsulates
that.

Adding an extra layer of indirection does *not* make things easier
to change, particularly when we are talking about a package-internal
detail here.

Any change is likely to require changes to the signature of the interface
type, and therefore to the user of that type. Given that, we may as well
just store the data in fields and avoid the need to change all the
interface implementations as *well* as the users of the type.

Unmerged revisions

2328. By Roger Peppe on 2014-02-14

upgrades: simplify

=== modified file 'upgrades/lockdirectory.go'
--- upgrades/lockdirectory.go	2014-02-13 05:54:35 +0000
+++ upgrades/lockdirectory.go	2014-02-14 09:17:52 +0000
@@ -21,8 +21,8 @@
 // we need to change the ownership of the lock directory to ubuntu:ubuntu.
 // Also we need to make sure that this directory exists on machines with no
 // units.
-func ensureLockDirExistsAndUbuntuWritable(context Context) error {
-	lockDir := path.Join(context.AgentConfig().DataDir(), "locks")
+func ensureLockDirExistsAndUbuntuWritable(context *Context) error {
+	lockDir := path.Join(context.AgentConfig.DataDir(), "locks")
 	// We only try to change ownership if there is an ubuntu user
 	// defined, and we determine this by the existance of the home dir.
 	command := fmt.Sprintf(""+

=== modified file 'upgrades/lockdirectory_test.go'
--- upgrades/lockdirectory_test.go	2014-02-13 05:54:35 +0000
+++ upgrades/lockdirectory_test.go	2014-02-14 09:17:52 +0000
@@ -23,7 +23,7 @@
 	home    string
 	datadir string
 	lockdir string
-	ctx     upgrades.Context
+	ctx     *upgrades.Context
 }
 
 var _ = gc.Suite(&ensureLockDirSuite{})
@@ -52,7 +52,9 @@
 
 	s.datadir = c.MkDir()
 	s.lockdir = filepath.Join(s.datadir, "locks")
-	s.ctx = &mockContext{agentConfig: &mockAgentConfig{dataDir: s.datadir}}
+	s.ctx = &upgrades.Context{
+		AgentConfig: &mockAgentConfig{dataDir: s.datadir},
+	}
 }
 
 func (s *ensureLockDirSuite) assertChownCalled(c *gc.C) {

=== modified file 'upgrades/operations.go'
--- upgrades/operations.go	2014-02-13 05:27:59 +0000
+++ upgrades/operations.go	2014-02-14 09:17:52 +0000
@@ -5,16 +5,11 @@
 
 import "launchpad.net/juju-core/version"
 
-// upgradeOperations returns an ordered slice of sets of operations needed
+// upgradeOperations holds an ordered slice of sets of operations needed
 // to upgrade Juju to particular version. The slice is ordered by target
 // version, so that the sets of operations are executed in order from oldest
 // version to most recent.
-var upgradeOperations = func() []UpgradeOperation {
-	steps := []UpgradeOperation{
-		upgradeToVersion{
-			version.MustParse("1.18.0"),
-			stepsFor118(),
-		},
-	}
-	return steps
-}
+var upgradeOperations = []UpgradeOperation{{
+	TargetVersion: version.MustParse("1.18.0"),
+	Steps:         stepsFor118(),
+}}

=== modified file 'upgrades/rsyslogconf.go'
--- upgrades/rsyslogconf.go	2014-02-14 00:48:23 +0000
+++ upgrades/rsyslogconf.go	2014-02-14 09:17:52 +0000
@@ -9,11 +9,11 @@
 	"launchpad.net/juju-core/log/syslog"
 )
 
-func confParams(context Context) (machineTag, namespace string, port int, err error) {
-	namespace = context.AgentConfig().Value(agent.Namespace)
-	machineTag = context.AgentConfig().Tag()
+func confParams(context *Context) (machineTag, namespace string, port int, err error) {
+	namespace = context.AgentConfig.Value(agent.Namespace)
+	machineTag = context.AgentConfig.Tag()
 
-	environment := context.APIState().Environment()
+	environment := context.APIState.Environment()
 	config, err := environment.EnvironConfig()
 	if err != nil {
 		return "", "", 0, err
@@ -23,7 +23,7 @@
 }
 
 // upgradeStateServerRsyslogConfig upgrades a rsuslog config file on a state server.
-func upgradeStateServerRsyslogConfig(context Context) (err error) {
+func upgradeStateServerRsyslogConfig(context *Context) (err error) {
 	machineTag, namespace, syslogPort, err := confParams(context)
 	configRenderer := syslog.NewAccumulateConfig(machineTag, syslogPort, namespace)
 	data, err := configRenderer.Render()
@@ -34,9 +34,9 @@
 }
 
 // upgradeHostMachineRsyslogConfig upgrades a rsuslog config file on a host machine.
-func upgradeHostMachineRsyslogConfig(context Context) (err error) {
+func upgradeHostMachineRsyslogConfig(context *Context) (err error) {
 	machineTag, namespace, syslogPort, err := confParams(context)
-	addr, err := context.AgentConfig().APIAddresses()
+	addr, err := context.AgentConfig.APIAddresses()
 	if err != nil {
 		return err
 	}

=== modified file 'upgrades/rsyslogconf_test.go'
--- upgrades/rsyslogconf_test.go	2014-02-13 05:54:35 +0000
+++ upgrades/rsyslogconf_test.go	2014-02-14 09:17:52 +0000
@@ -20,7 +20,7 @@
 	jujutesting.JujuConnSuite
 
 	syslogPath string
-	ctx        upgrades.Context
+	ctx        *upgrades.Context
 }
 
 var _ = gc.Suite(&rsyslogSuite{})
@@ -33,13 +33,13 @@
 	s.PatchValue(&environs.RsyslogConfPath, s.syslogPath)
 
 	apiState, _ := s.OpenAPIAsNewMachine(c, state.JobManageState, state.JobManageEnviron)
-	s.ctx = &mockContext{
-		agentConfig: &mockAgentConfig{
+	s.ctx = &upgrades.Context{
+		AgentConfig: &mockAgentConfig{
 			tag:          "machine-tag",
 			namespace:    "namespace",
 			apiAddresses: []string{"server:1234"},
 		},
-		apiState: apiState,
+		APIState: apiState,
 	}
 }
 

=== modified file 'upgrades/steps118.go'
--- upgrades/steps118.go	2014-02-13 05:22:01 +0000
+++ upgrades/steps118.go	2014-02-14 09:17:52 +0000
@@ -5,21 +5,18 @@
 
 // stepsFor118 returns upgrade steps to upgrade to a Juju 1.18 deployment.
 func stepsFor118() []UpgradeStep {
-	return []UpgradeStep{
-		&upgradeStep{
-			description: "make $DATADIR/locks owned by ubuntu:ubuntu",
-			targets:     []UpgradeTarget{HostMachine},
-			run:         ensureLockDirExistsAndUbuntuWritable,
-		},
-		&upgradeStep{
-			description: "upgrade rsyslog config file on state server",
-			targets:     []UpgradeTarget{StateServer},
-			run:         upgradeStateServerRsyslogConfig,
-		},
-		&upgradeStep{
-			description: "upgrade rsyslog config file on host machine",
-			targets:     []UpgradeTarget{HostMachine},
-			run:         upgradeHostMachineRsyslogConfig,
-		},
+	return []UpgradeStep{{
+		Description: "make $DATADIR/locks owned by ubuntu:ubuntu",
+		Targets:     []UpgradeTarget{HostMachine},
+		Run:         ensureLockDirExistsAndUbuntuWritable,
+	}, {
+		Description: "upgrade rsyslog config file on state server",
+		Targets:     []UpgradeTarget{StateServer},
+		Run:         upgradeStateServerRsyslogConfig,
+	}, {
+		Description: "upgrade rsyslog config file on host machine",
+		Targets:     []UpgradeTarget{HostMachine},
+		Run:         upgradeHostMachineRsyslogConfig,
+	},
 	}
 }

=== modified file 'upgrades/upgrade.go'
--- upgrades/upgrade.go	2014-02-13 05:27:59 +0000
+++ upgrades/upgrade.go	2014-02-14 09:17:52 +0000
@@ -17,27 +17,27 @@
 
 // UpgradeStep defines an idempotent operation that is run to perform
 // a specific upgrade step.
-type UpgradeStep interface {
+type UpgradeStep struct {
 	// Description is a human readable description of what the upgrade step does.
-	Description() string
+	Description string
 
 	// Targets returns the target machine types for which the upgrade step is applicable.
-	Targets() []UpgradeTarget
+	Targets []UpgradeTarget
 
 	// Run executes the upgrade business logic.
-	Run(context Context) error
+	Run func(context *Context) error
 }
 
 // UpgradeOperation defines what steps to perform to upgrade to a target version.
-type UpgradeOperation interface {
+type UpgradeOperation struct {
 	// The Juju version for which this operation is applicable.
 	// Upgrade operations designed for versions of Juju earlier
 	// than we are upgrading from are not run since such steps would
 	// already have been used to get to the version we are running now.
-	TargetVersion() version.Number
+	TargetVersion version.Number
 
 	// Steps to perform during an upgrade.
-	Steps() []UpgradeStep
+	Steps []UpgradeStep
 }
 
 // UpgradeTarget defines the type of machine for which a particular upgrade
@@ -55,50 +55,17 @@
 	StateServer = UpgradeTarget("stateServer")
 )
 
-// upgradeToVersion encapsulates the steps which need to be run to
-// upgrade any prior version of Juju to targetVersion.
-type upgradeToVersion struct {
-	targetVersion version.Number
-	steps         []UpgradeStep
-}
-
-// Steps is defined on the UpgradeOperation interface.
-func (u upgradeToVersion) Steps() []UpgradeStep {
-	return u.steps
-}
-
-// TargetVersion is defined on the UpgradeOperation interface.
-func (u upgradeToVersion) TargetVersion() version.Number {
-	return u.targetVersion
-}
-
 // Context is used give the upgrade steps attributes needed
 // to do their job.
-type Context interface {
+// Work in progress........
+// Exactly what a context needs is to be determined as the
+// implementation evolves.
+type Context struct {
 	// APIState returns an API connection to state.
-	APIState() *api.State
+	APIState *api.State
 
 	// AgentConfig returns the agent config for the machine that is being upgraded.
-	AgentConfig() agent.Config
-}
-
-// UpgradeContext is a default Context implementation.
-type UpgradeContext struct {
-	// Work in progress........
-	// Exactly what a context needs is to be determined as the
-	// implementation evolves.
-	st          *api.State
-	agentConfig agent.Config
-}
-
-// APIState is defined on the Context interface.
-func (c *UpgradeContext) APIState() *api.State {
-	return c.st
-}
-
-// AgentConfig is defined on the Context interface.
-func (c *UpgradeContext) AgentConfig() agent.Config {
-	return c.agentConfig
+	AgentConfig agent.Config
 }
 
 // upgradeError records a description of the step being performed and the error.
@@ -113,14 +80,14 @@
 
 // PerformUpgrade runs the business logic needed to upgrade the current "from" version to this
 // version of Juju on the "target" type of machine.
-func PerformUpgrade(from version.Number, target UpgradeTarget, context Context) *upgradeError {
+func PerformUpgrade(from version.Number, target UpgradeTarget, context *Context) *upgradeError {
 	// If from is not known, it is 1.16.
 	if from == version.Zero {
 		from = version.MustParse("1.16.0")
 	}
-	for _, upgradeOps := range upgradeOperations() {
+	for _, upgradeOps := range upgradeOperations {
 		// Do not run steps for versions of Juju earlier or same as we are upgrading from.
-		if upgradeOps.TargetVersion().LessEqual(from) {
+		if upgradeOps.TargetVersion.LessEqual(from) {
 			continue
 		}
 		if err := runUpgradeSteps(context, target, upgradeOps); err != nil {
@@ -132,12 +99,12 @@
 
 // validTarget returns true if target is in step.Targets().
 func validTarget(target UpgradeTarget, step UpgradeStep) bool {
-	for _, opTarget := range step.Targets() {
+	for _, opTarget := range step.Targets {
 		if target == AllMachines || target == opTarget {
 			return true
 		}
 	}
-	return len(step.Targets()) == 0
+	return len(step.Targets) == 0
 }
 
 // runUpgradeSteps runs all the upgrade steps relevant to target.
@@ -145,38 +112,17 @@
 // subsequent steps may required successful completion of earlier ones.
 // The steps must be idempotent so that the entire upgrade operation can
 // be retried.
-func runUpgradeSteps(context Context, target UpgradeTarget, upgradeOp UpgradeOperation) *upgradeError {
-	for _, step := range upgradeOp.Steps() {
+func runUpgradeSteps(context *Context, target UpgradeTarget, upgradeOp UpgradeOperation) *upgradeError {
+	for _, step := range upgradeOp.Steps {
 		if !validTarget(target, step) {
 			continue
 		}
 		if err := step.Run(context); err != nil {
 			return &upgradeError{
-				description: step.Description(),
+				description: step.Description,
 				err:         err,
 			}
 		}
 	}
 	return nil
 }
-
-type upgradeStep struct {
-	description string
-	targets     []UpgradeTarget
-	run         func(Context) error
-}
-
-// Description is defined on the UpgradeStep interface.
-func (step *upgradeStep) Description() string {
-	return step.description
-}
-
-// Targets is defined on the UpgradeStep interface.
-func (step *upgradeStep) Targets() []UpgradeTarget {
-	return step.targets
-}
-
-// Run is defined on the UpgradeStep interface.
-func (step *upgradeStep) Run(context Context) error {
-	return step.run(context)
-}

=== modified file 'upgrades/upgrade_test.go'
--- upgrades/upgrade_test.go	2014-02-13 05:27:59 +0000
+++ upgrades/upgrade_test.go	2014-02-14 09:17:52 +0000
@@ -28,7 +28,7 @@
 func assertExpectedSteps(c *gc.C, steps []upgrades.UpgradeStep, expectedSteps []string) {
 	var stepNames = make([]string, len(steps))
 	for i, step := range steps {
-		stepNames[i] = step.Description()
+		stepNames[i] = step.Description
 	}
 	c.Assert(stepNames, gc.DeepEquals, expectedSteps)
 }
@@ -39,38 +39,11 @@
 
 var _ = gc.Suite(&upgradeSuite{})
 
-type mockUpgradeOperation struct {
-	targetVersion version.Number
-	steps         []upgrades.UpgradeStep
-}
-
-func (m *mockUpgradeOperation) TargetVersion() version.Number {
-	return m.targetVersion
-}
-
-func (m *mockUpgradeOperation) Steps() []upgrades.UpgradeStep {
-	return m.steps
-}
-
-type mockUpgradeStep struct {
-	msg     string
-	targets []upgrades.UpgradeTarget
-}
-
-func (u *mockUpgradeStep) Description() string {
-	return u.msg
-}
-
-func (u *mockUpgradeStep) Targets() []upgrades.UpgradeTarget {
-	return u.targets
-}
-
-func (u *mockUpgradeStep) Run(context upgrades.Context) error {
-	if strings.HasSuffix(u.msg, "error") {
+func runStep(ctx *testContext, u upgrades.UpgradeStep, context *upgrades.Context) error {
+	if strings.HasSuffix(u.Description, "error") {
 		return errors.New("upgrade error occurred")
 	}
-	ctx := context.(*mockContext)
-	ctx.messages = append(ctx.messages, u.msg)
+	ctx.messages = append(ctx.messages, u.Description)
 	return nil
 }
 
@@ -122,44 +95,72 @@
 	return upgradeTargets
 }
 
-func upgradeOperations() []upgrades.UpgradeOperation {
-	steps := []upgrades.UpgradeOperation{
-		&mockUpgradeOperation{
-			targetVersion: version.MustParse("1.12.0"),
-			steps: []upgrades.UpgradeStep{
-				&mockUpgradeStep{"step 1 - 1.12.0", nil},
-				&mockUpgradeStep{"step 2 error", targets(upgrades.HostMachine)},
-				&mockUpgradeStep{"step 3", targets(upgrades.HostMachine)},
-			},
-		},
-		&mockUpgradeOperation{
-			targetVersion: version.MustParse("1.16.0"),
-			steps: []upgrades.UpgradeStep{
-				&mockUpgradeStep{"step 1 - 1.16.0", targets(upgrades.HostMachine)},
-				&mockUpgradeStep{"step 2 - 1.16.0", targets(upgrades.HostMachine)},
-				&mockUpgradeStep{"step 3 - 1.16.0", targets(upgrades.StateServer)},
-			},
-		},
-		&mockUpgradeOperation{
-			targetVersion: version.MustParse("1.17.0"),
-			steps: []upgrades.UpgradeStep{
-				&mockUpgradeStep{"step 1 - 1.17.0", targets(upgrades.HostMachine)},
-			},
-		},
-		&mockUpgradeOperation{
-			targetVersion: version.MustParse("1.17.1"),
-			steps: []upgrades.UpgradeStep{
-				&mockUpgradeStep{"step 1 - 1.17.1", targets(upgrades.HostMachine)},
-				&mockUpgradeStep{"step 2 - 1.17.1", targets(upgrades.StateServer)},
-			},
-		},
-		&mockUpgradeOperation{
-			targetVersion: version.MustParse("1.18.0"),
-			steps: []upgrades.UpgradeStep{
-				&mockUpgradeStep{"step 1 - 1.18.0", targets(upgrades.HostMachine)},
-				&mockUpgradeStep{"step 2 - 1.18.0", targets(upgrades.StateServer)},
-			},
-		},
+type testContext struct {
+	messages []string
+}
+
+func upgradeOperations(testCtxt *testContext) []upgrades.UpgradeOperation {
+	steps := []upgrades.UpgradeOperation{{
+		TargetVersion: version.MustParse("1.12.0"),
+		Steps: []upgrades.UpgradeStep{{
+			Description: "step 1 - 1.12.0",
+		}, {
+			Description: "step 2 error",
+			Targets:     targets(upgrades.HostMachine),
+		}, {
+			Description: "step 3",
+			Targets:     targets(upgrades.HostMachine),
+		},
+		},
+	}, {
+		TargetVersion: version.MustParse("1.16.0"),
+		Steps: []upgrades.UpgradeStep{{
+			Description: "step 1 - 1.16.0",
+			Targets:     targets(upgrades.HostMachine),
+		}, {
+			Description: "step 2 - 1.16.0",
+			Targets:     targets(upgrades.HostMachine),
+		}, {
+			Description: "step 3 - 1.16.0",
+			Targets:     targets(upgrades.StateServer),
+		},
+		},
+	}, {
+		TargetVersion: version.MustParse("1.17.0"),
+		Steps: []upgrades.UpgradeStep{{
+			Description: "step 1 - 1.17.0",
+			Targets:     targets(upgrades.HostMachine),
+		},
+		},
+	}, {
+		TargetVersion: version.MustParse("1.17.1"),
+		Steps: []upgrades.UpgradeStep{{
+			Description: "step 1 - 1.17.1",
+			Targets:     targets(upgrades.HostMachine),
+		}, {
+			Description: "step 2 - 1.17.1",
+			Targets:     targets(upgrades.StateServer),
+		},
+		},
+	}, {
+		TargetVersion: version.MustParse("1.18.0"),
+		Steps: []upgrades.UpgradeStep{{
+			Description: "step 1 - 1.18.0",
+			Targets:     targets(upgrades.HostMachine),
+		}, {
+			Description: "step 2 - 1.18.0",
+			Targets:     targets(upgrades.StateServer),
+		},
+		},
+	},
+	}
+	for _, op := range steps {
+		for i, step := range op.Steps {
+			step := step
+			op.Steps[i].Run = func(ctxt *upgrades.Context) error {
+				return runStep(testCtxt, step, ctxt)
+			}
+		}
 	}
 	return steps
 }
@@ -213,31 +214,29 @@
 }
 
 func (s *upgradeSuite) TestPerformUpgrade(c *gc.C) {
-	s.PatchValue(upgrades.UpgradeOperations, upgradeOperations)
+	testCtxt := &testContext{}
+	s.PatchValue(upgrades.UpgradeOperations, upgradeOperations(testCtxt))
 	for i, test := range upgradeTests {
 		c.Logf("%d: %s", i, test.about)
-		var messages []string
-		ctx := &mockContext{
-			messages: messages,
-		}
+		testCtxt.messages = nil
 		fromVersion := version.Zero
 		if test.fromVersion != "" {
 			fromVersion = version.MustParse(test.fromVersion)
 		}
-		err := upgrades.PerformUpgrade(fromVersion, test.target, ctx)
+		err := upgrades.PerformUpgrade(fromVersion, test.target, &upgrades.Context{})
 		if test.err == "" {
 			c.Check(err, gc.IsNil)
 		} else {
 			c.Check(err, gc.ErrorMatches, test.err)
 		}
-		c.Check(ctx.messages, jc.DeepEquals, test.expectedSteps)
+		c.Check(testCtxt.messages, jc.DeepEquals, test.expectedSteps)
 	}
 }
 
 func (s *upgradeSuite) TestUpgradeOperationsOrdered(c *gc.C) {
 	var previous version.Number
-	for i, utv := range (*upgrades.UpgradeOperations)() {
-		vers := utv.TargetVersion()
+	for i, utv := range *upgrades.UpgradeOperations {
+		vers := utv.TargetVersion
 		if i > 0 {
 			c.Check(previous.Less(vers), jc.IsTrue)
 		}
@@ -249,8 +248,8 @@
 
 func (s *upgradeSuite) TestUpgradeOperationsVersions(c *gc.C) {
 	var versions []string
-	for _, utv := range (*upgrades.UpgradeOperations)() {
-		versions = append(versions, utv.TargetVersion().String())
+	for _, utv := range *upgrades.UpgradeOperations {
+		versions = append(versions, utv.TargetVersion.String())
 
 	}
 	c.Assert(versions, gc.DeepEquals, expectedVersions)