Addressed review commends by @axw

Horacio Duran · Horacio Duran · commit 48696678e028 · 2015-12-21T14:53:06.000+10:00
diff --git a/provider/interfaces.go b/provider/interfaces.go
@@ -5,8 +5,8 @@ package provider
 
 import "github.com/juju/juju/version"
 
-// Upgradable represents a provider that supports upgrade steps
+// Upgradeable represents a provider that supports upgrade steps
 // if present, these steps will get called upon upgrading.
-type Upgradable interface {
+type Upgradeable interface {
 	RunUpgradeStepsFor(version.Number) error
 }
diff --git a/provider/openstack/upgrade.go b/provider/openstack/upgrade.go
@@ -26,17 +26,18 @@ func (e *Environ) RunUpgradeStepsFor(ver version.Number) error {
 	return nil
 }
 
-func replaceNameWithID(oldName, envName, eUUID string) (string, error, bool) {
-	nameStart := strings.LastIndex(oldName, envName)
-	if nameStart <= -1 {
-		uuidPresent := strings.LastIndex(oldName, eUUID)
-		if uuidPresent < 0 {
-			return "", nil, false
+func replaceNameWithID(oldName, envName, eUUID string) (string, bool, error) {
+	prefix := "juju-" + envName
+	if !strings.HasPrefix(oldName, prefix) {
+		return "", false, nil
+	}
+	if len(prefix) < len(oldName) {
+		if oldName[len(prefix)] != '-' {
+			return "", false, nil
 		}
-		return oldName, nil, false
 	}
-	partial := oldName[nameStart:]
-	return fmt.Sprintf("%s%s", oldName[:nameStart], strings.Replace(partial, envName, eUUID, -1)), nil, true
+	newPrefix := "juju-" + eUUID
+	return strings.Replace(oldName, prefix, newPrefix, -1), true, nil
 }
 
 func addUUIDToSecurityGroupNames(e *Environ) error {
@@ -52,7 +53,7 @@ func addUUIDToSecurityGroupNames(e *Environ) error {
 		return errors.NotFoundf("environment uuid for environment %q", eName)
 	}
 	for _, group := range groups {
-		newName, err, ok := replaceNameWithID(group.Name, eName, eUUID)
+		newName, ok, err := replaceNameWithID(group.Name, eName, eUUID)
 		if err != nil {
 			return errors.Annotate(err, "generating the new security group name")
 		}
@@ -90,7 +91,7 @@ func addUUIDToMachineNames(e *Environ) error {
 		return errors.NotFoundf("environment uuid for environment %q", eName)
 	}
 	for _, server := range servers {
-		newName, err, ok := replaceNameWithID(server.Name, eName, eUUID)
+		newName, ok, err := replaceNameWithID(server.Name, eName, eUUID)
 		if err != nil {
 			return errors.Annotate(err, "generating the new server name")
 		}
diff --git a/upgrades/providerchanges.go b/upgrades/providerchanges.go
@@ -16,7 +16,7 @@ func upgradeProviderChanges(env environs.Environ, reader environConfigReader, ve
 		return errors.Annotate(err, "reading environment config")
 	}
 
-	upgrader, ok := env.(provider.Upgradable)
+	upgrader, ok := env.(provider.Upgradeable)
 	if !ok {
 		logger.Debugf("provider %q has no upgrades", cfg.Type())
 		return nil
diff --git a/upgrades/steps126.go b/upgrades/steps126.go
@@ -5,6 +5,7 @@ package upgrades
 
 import (
 	"github.com/juju/errors"
+
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/state"
 	"github.com/juju/juju/state/utils"
@@ -44,6 +45,8 @@ func stateStepsFor126() []Step {
 				return upgradeEnvironConfig(st, st, environs.GlobalProviderRegistry())
 			},
 		},
+		//TODO(perrito666) make this an unconditional upgrade step.
+		// it would be ideal not to have to modify this package whenever we add provider upgrade steps.
 		&upgradeStep{
 			description: "provider side upgrades",
 			targets:     []Target{DatabaseMaster},

Original file line number	Diff line number	Diff line change
`@@ -5,8 +5,8 @@ package provider`
`5`	`5`
`6`	`6`	`import "github.com/juju/juju/version"`
`7`	`7`
`8`		`-// Upgradable represents a provider that supports upgrade steps`
	`8`	`+// Upgradeable represents a provider that supports upgrade steps`
`9`	`9`	`// if present, these steps will get called upon upgrading.`
`10`		`-type Upgradable interface {`
	`10`	`+type Upgradeable interface {`
`11`	`11`	`RunUpgradeStepsFor(version.Number) error`
`12`	`12`	`}`
Original file line number	Diff line number	Diff line change
`@@ -26,17 +26,18 @@ func (e *Environ) RunUpgradeStepsFor(ver version.Number) error {`
`26`	`26`	`return nil`
`27`	`27`	`}`
`28`	`28`
`29`		`-func replaceNameWithID(oldName, envName, eUUID string) (string, error, bool) {`
`30`		`- nameStart := strings.LastIndex(oldName, envName)`
`31`		`- if nameStart <= -1 {`
`32`		`- uuidPresent := strings.LastIndex(oldName, eUUID)`
`33`		`- if uuidPresent < 0 {`
`34`		`- return "", nil, false`
	`29`	`+func replaceNameWithID(oldName, envName, eUUID string) (string, bool, error) {`
	`30`	`+ prefix := "juju-" + envName`
	`31`	`+ if !strings.HasPrefix(oldName, prefix) {`
	`32`	`+ return "", false, nil`
	`33`	`+ }`
	`34`	`+ if len(prefix) < len(oldName) {`
	`35`	`+ if oldName[len(prefix)] != '-' {`
	`36`	`+ return "", false, nil`
`35`	`37`	`}`
`36`		`- return oldName, nil, false`
`37`	`38`	`}`
`38`		`- partial := oldName[nameStart:]`
`39`		`- return fmt.Sprintf("%s%s", oldName[:nameStart], strings.Replace(partial, envName, eUUID, -1)), nil, true`
	`39`	`+ newPrefix := "juju-" + eUUID`
	`40`	`+ return strings.Replace(oldName, prefix, newPrefix, -1), true, nil`
`40`	`41`	`}`
`41`	`42`
`42`	`43`	`func addUUIDToSecurityGroupNames(e *Environ) error {`
`@@ -52,7 +53,7 @@ func addUUIDToSecurityGroupNames(e *Environ) error {`
`52`	`53`	`return errors.NotFoundf("environment uuid for environment %q", eName)`
`53`	`54`	`}`
`54`	`55`	`for _, group := range groups {`
`55`		`- newName, err, ok := replaceNameWithID(group.Name, eName, eUUID)`
	`56`	`+ newName, ok, err := replaceNameWithID(group.Name, eName, eUUID)`
`56`	`57`	`if err != nil {`
`57`	`58`	`return errors.Annotate(err, "generating the new security group name")`
`58`	`59`	`}`
`@@ -90,7 +91,7 @@ func addUUIDToMachineNames(e *Environ) error {`
`90`	`91`	`return errors.NotFoundf("environment uuid for environment %q", eName)`
`91`	`92`	`}`
`92`	`93`	`for _, server := range servers {`
`93`		`- newName, err, ok := replaceNameWithID(server.Name, eName, eUUID)`
	`94`	`+ newName, ok, err := replaceNameWithID(server.Name, eName, eUUID)`
`94`	`95`	`if err != nil {`
`95`	`96`	`return errors.Annotate(err, "generating the new server name")`
`96`	`97`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ func upgradeProviderChanges(env environs.Environ, reader environConfigReader, ve`
`16`	`16`	`return errors.Annotate(err, "reading environment config")`
`17`	`17`	`}`
`18`	`18`
`19`		`- upgrader, ok := env.(provider.Upgradable)`
	`19`	`+ upgrader, ok := env.(provider.Upgradeable)`
`20`	`20`	`if !ok {`
`21`	`21`	`logger.Debugf("provider %q has no upgrades", cfg.Type())`
`22`	`22`	`return nil`