Merge pull request #7302 from rogpeppe/152-migrate-modelcommand

cmd/juju/commands: make migrateCommand use ModelCommandBase

This means that it can re-use the standard model-finding
logic rather than duplicating it slightly differently,
so that the user will have consistent behaviour
across all commands (less code too)

QA check that the migrate command still works.

cmd/juju/commands/migrate.go

@@ -4,37 +4,32 @@
 package commands
 
 import (
-	"strings"
-
 	"github.com/juju/cmd"
 	"github.com/juju/errors"
 	"gopkg.in/macaroon-bakery.v1/httpbakery"
 	"gopkg.in/macaroon.v1"
 
 	"github.com/juju/juju/api"
-	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/api/controller"
 	"github.com/juju/juju/cmd/modelcmd"
 	"github.com/juju/juju/jujuclient"
 )
 
-func newMigrateCommand() cmd.Command {
+func newMigrateCommand() modelcmd.ModelCommand {
 	var cmd migrateCommand
 	cmd.newAPIRoot = cmd.CommandBase.NewAPIRoot
-	return modelcmd.WrapController(&cmd)
+	return modelcmd.Wrap(&cmd, modelcmd.WrapSkipModelFlags)
 }
 
 // migrateCommand initiates a model migration.
 type migrateCommand struct {
-	modelcmd.ControllerCommandBase
+	modelcmd.ModelCommandBase
 	newAPIRoot       func(jujuclient.ClientStore, string, string) (api.Connection, error)
 	api              migrateAPI
-	model            string
 	targetController string
 }
 
 type migrateAPI interface {
-	AllModels() ([]base.UserModel, error)
 	InitiateMigration(spec controller.MigrationSpec) (string, error)
 }
 
@@ -89,7 +84,7 @@ func (c *migrateCommand) Init(args []string) error {
 		return errors.New("too many arguments specified")
 	}
 
-	c.model = args[0]
+	c.SetModelName(args[0], false)
 	c.targetController = args[1]
 	return nil
 }
@@ -132,11 +127,16 @@ func (c *migrateCommand) Run(ctx *cmd.Context) error {
 	if err != nil {
 		return err
 	}
-	api, err := c.getAPI()
+	modelName, err := c.ModelName()
 	if err != nil {
-		return err
+		return errors.Trace(err)
+	}
+	uuids, err := c.ModelUUIDs([]string{modelName})
+	if err != nil {
+		return errors.Trace(err)
 	}
-	spec.ModelUUID, err = c.findModelUUID(ctx, api)
+	spec.ModelUUID = uuids[0]
+	api, err := c.getAPI()
 	if err != nil {
 		return err
 	}
@@ -148,46 +148,15 @@ func (c *migrateCommand) Run(ctx *cmd.Context) error {
 	return nil
 }
 
-func (c *migrateCommand) findModelUUID(ctx *cmd.Context, api migrateAPI) (string, error) {
-	models, err := api.AllModels()
-	if err != nil {
-		return "", errors.Trace(err)
-	}
-	// Look for the uuid based on name. If the model name doesn't container a
-	// slash, then only accept the model name if there exists only one model
-	// with that name.
-	owner := ""
-	name := c.model
-	if strings.Contains(name, "/") {
-		values := strings.SplitN(name, "/", 2)
-		owner = values[0]
-		name = values[1]
-	}
-	var matches []base.UserModel
-	for _, model := range models {
-		if model.Name == name && (owner == "" || model.Owner == owner) {
-			matches = append(matches, model)
-		}
-	}
-	switch len(matches) {
-	case 0:
-		return "", errors.NotFoundf("model matching %q", c.model)
-	case 1:
-		return matches[0].UUID, nil
-	default:
-		ctx.Infof("Multiple potential matches found, please specify owner to disambiguate:")
-		for _, match := range matches {
-			ctx.Infof("  %s/%s", match.Owner, match.Name)
-		}
-		return "", errors.New("multiple models match name")
-	}
-}
-
 func (c *migrateCommand) getAPI() (migrateAPI, error) {
 	if c.api != nil {
 		return c.api, nil
 	}
-	return c.NewControllerAPIClient()
+	apiRoot, err := c.NewControllerAPIRoot()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return controller.NewClient(apiRoot), nil
 }
 
 func (c *migrateCommand) getTargetControllerMacaroons() ([]macaroon.Slice, error) {

cmd/juju/commands/migrate_test.go

@@ -27,6 +27,7 @@ type MigrateSuite struct {
 	testing.FakeJujuXDGDataHomeSuite
 	api                 *fakeMigrateAPI
 	targetControllerAPI *fakeTargetControllerAPI
+	modelAPI            *fakeModelAPI
 	store               *jujuclient.MemStore
 	password            string
 }
@@ -52,13 +53,13 @@ func (s *MigrateSuite) SetUpTest(c *gc.C) {
 
 	// Define an account for the model in the source controller in the config.
 	err = s.store.UpdateAccount("source", jujuclient.AccountDetails{
-		User: "source",
+		User: "sourceuser",
 	})
 	c.Assert(err, jc.ErrorIsNil)
 
 	// Define the account for the target controller.
 	err = s.store.UpdateAccount("target", jujuclient.AccountDetails{
-		User:     "target",
+		User:     "targetuser",
 		Password: "secret",
 	})
 	c.Assert(err, jc.ErrorIsNil)
@@ -71,19 +72,20 @@ func (s *MigrateSuite) SetUpTest(c *gc.C) {
 	})
 	c.Assert(err, jc.ErrorIsNil)
 
-	s.api = &fakeMigrateAPI{
+	s.api = &fakeMigrateAPI{}
+	s.modelAPI = &fakeModelAPI{
 		models: []base.UserModel{{
 			Name:  "model",
 			UUID:  modelUUID,
-			Owner: "owner",
+			Owner: "sourceuser",
 		}, {
 			Name:  "production",
 			UUID:  "prod-1-uuid",
 			Owner: "alpha",
 		}, {
 			Name:  "production",
 			UUID:  "prod-2-uuid",
-			Owner: "omega",
+			Owner: "sourceuser",
 		}},
 	}
 
@@ -144,14 +146,14 @@ func (s *MigrateSuite) TestSuccess(c *gc.C) {
 		TargetControllerUUID: targetControllerUUID,
 		TargetAddrs:          []string{"1.2.3.4:5"},
 		TargetCACert:         "cert",
-		TargetUser:           "target",
+		TargetUser:           "targetuser",
 		TargetPassword:       "secret",
 	})
 }
 
 func (s *MigrateSuite) TestSuccessMacaroons(c *gc.C) {
 	err := s.store.UpdateAccount("target", jujuclient.AccountDetails{
-		User:     "target",
+		User:     "targetuser",
 		Password: "",
 	})
 	c.Assert(err, jc.ErrorIsNil)
@@ -165,38 +167,41 @@ func (s *MigrateSuite) TestSuccessMacaroons(c *gc.C) {
 		TargetControllerUUID: targetControllerUUID,
 		TargetAddrs:          []string{"1.2.3.4:5"},
 		TargetCACert:         "cert",
-		TargetUser:           "target",
+		TargetUser:           "targetuser",
 		TargetMacaroons:      s.targetControllerAPI.macaroons,
 	})
 }
 
 func (s *MigrateSuite) TestModelDoesntExist(c *gc.C) {
 	cmd := s.makeCommand()
-	cmd.SetModelAPI(&fakeModelAPI{})
 	_, err := cmdtesting.RunCommand(c, cmd, "wat", "target")
 	c.Check(err, gc.ErrorMatches, "model .+ not found")
 	c.Check(s.api.specSeen, gc.IsNil) // API shouldn't have been called
 }
 
 func (s *MigrateSuite) TestMultipleModelMatch(c *gc.C) {
 	cmd := s.makeCommand()
-	cmd.SetModelAPI(&fakeModelAPI{})
+	// Disambiguation is done in the standard way by choosing
+	// the current user's model.
 	ctx, err := cmdtesting.RunCommand(c, cmd, "production", "target")
-	c.Check(err, gc.ErrorMatches, "multiple models match name")
-	expected := "" +
-		"Multiple potential matches found, please specify owner to disambiguate:\n" +
-		"  alpha/production\n" +
-		"  omega/production\n"
-	c.Check(cmdtesting.Stderr(ctx), gc.Equals, expected)
-	c.Check(s.api.specSeen, gc.IsNil) // API shouldn't have been called
+	c.Assert(err, jc.ErrorIsNil)
+	c.Check(cmdtesting.Stderr(ctx), gc.Matches, "Migration started with ID \"uuid:0\"\n")
+	c.Check(s.api.specSeen, jc.DeepEquals, &controller.MigrationSpec{
+		ModelUUID:            "prod-2-uuid",
+		TargetControllerUUID: targetControllerUUID,
+		TargetAddrs:          []string{"1.2.3.4:5"},
+		TargetCACert:         "cert",
+		TargetUser:           "targetuser",
+		TargetPassword:       "secret",
+	})
 }
 
 func (s *MigrateSuite) TestSpecifyOwner(c *gc.C) {
-	ctx, err := s.makeAndRun(c, "omega/production", "target")
+	ctx, err := s.makeAndRun(c, "alpha/production", "target")
 	c.Assert(err, jc.ErrorIsNil)
 
 	c.Check(cmdtesting.Stderr(ctx), gc.Matches, "Migration started with ID \"uuid:0\"\n")
-	c.Check(s.api.specSeen.ModelUUID, gc.Equals, "prod-2-uuid")
+	c.Check(s.api.specSeen.ModelUUID, gc.Equals, "prod-1-uuid")
 }
 
 func (s *MigrateSuite) TestControllerDoesntExist(c *gc.C) {
@@ -209,14 +214,15 @@ func (s *MigrateSuite) makeAndRun(c *gc.C, args ...string) (*cmd.Context, error)
 	return cmdtesting.RunCommand(c, s.makeCommand(), args...)
 }
 
-func (s *MigrateSuite) makeCommand() modelcmd.ControllerCommand {
-	cmd := modelcmd.WrapController(&migrateCommand{
-		api: s.api,
-		newAPIRoot: func(jujuclient.ClientStore, string, string) (api.Connection, error) {
-			return s.targetControllerAPI, nil
-		},
-	})
+func (s *MigrateSuite) makeCommand() modelcmd.ModelCommand {
+	cmd := newMigrateCommand()
 	cmd.SetClientStore(s.store)
+	cmd.SetModelAPI(s.modelAPI)
+	inner := modelcmd.InnerCommand(cmd).(*migrateCommand)
+	inner.api = s.api
+	inner.newAPIRoot = func(jujuclient.ClientStore, string, string) (api.Connection, error) {
+		return s.targetControllerAPI, nil
+	}
 	return cmd
 }
 
@@ -226,31 +232,19 @@ func (s *MigrateSuite) run(c *gc.C, cmd *migrateCommand, args ...string) (*cmd.C
 
 type fakeMigrateAPI struct {
 	specSeen *controller.MigrationSpec
-	models   []base.UserModel
 }
 
 func (a *fakeMigrateAPI) InitiateMigration(spec controller.MigrationSpec) (string, error) {
 	a.specSeen = &spec
 	return "uuid:0", nil
 }
 
-func (a *fakeMigrateAPI) AllModels() ([]base.UserModel, error) {
-	return a.models, nil
-}
-
 type fakeModelAPI struct {
-	model string
+	models []base.UserModel
 }
 
 func (m *fakeModelAPI) ListModels(user string) ([]base.UserModel, error) {
-	if m.model == "" {
-		return []base.UserModel{}, nil
-	}
-	return []base.UserModel{{
-		Name:  m.model,
-		UUID:  modelUUID,
-		Owner: "source",
-	}}, nil
+	return m.models, nil
 }
 
 func (m *fakeModelAPI) Close() error {

cmd/modelcmd/base.go

@@ -283,6 +283,27 @@ func (c *CommandBase) RefreshModels(store jujuclient.ClientStore, controllerName
 	return nil
 }
 
+// ModelUUIDs returns the model UUIDs for the given model names.
+func (c *CommandBase) ModelUUIDs(store jujuclient.ClientStore, controllerName string, modelNames []string) ([]string, error) {
+	var result []string
+	for _, modelName := range modelNames {
+		model, err := store.ModelByName(controllerName, modelName)
+		if errors.IsNotFound(err) {
+			// The model isn't known locally, so query the models available in the controller.
+			logger.Infof("model %q not cached locally, refreshing models from controller", modelName)
+			if err := c.RefreshModels(store, controllerName); err != nil {
+				return nil, errors.Annotatef(err, "refreshing model %q", modelName)
+			}
+			model, err = store.ModelByName(controllerName, modelName)
+		}
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+		result = append(result, model.ModelUUID)
+	}
+	return result, nil
+}
+
 // getAPIContext returns an apiContext for the given controller.
 // It will return the same context if called twice for the same controller.
 // The context will be closed when closeAPIContexts is called.

cmd/modelcmd/controller.go

@@ -225,28 +225,11 @@ func (c *ControllerCommandBase) newAPIRoot(modelName string) (api.Connection, er
 
 // ModelUUIDs returns the model UUIDs for the given model names.
 func (c *ControllerCommandBase) ModelUUIDs(modelNames []string) ([]string, error) {
-	var result []string
-	store := c.ClientStore()
 	controllerName, err := c.ControllerName()
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	for _, modelName := range modelNames {
-		model, err := store.ModelByName(controllerName, modelName)
-		if errors.IsNotFound(err) {
-			// The model isn't known locally, so query the models available in the controller.
-			logger.Infof("model %q not cached locally, refreshing models from controller", modelName)
-			if err := c.RefreshModels(store, controllerName); err != nil {
-				return nil, errors.Annotatef(err, "refreshing model %q", modelName)
-			}
-			model, err = store.ModelByName(controllerName, modelName)
-		}
-		if err != nil {
-			return nil, errors.Trace(err)
-		}
-		result = append(result, model.ModelUUID)
-	}
-	return result, nil
+	return c.CommandBase.ModelUUIDs(c.ClientStore(), controllerName, modelNames)
 }
 
 // CurrentAccountDetails returns details of the account associated with

cmd/modelcmd/modelcommand.go

@@ -282,6 +282,15 @@ func (c *ModelCommandBase) newAPIRoot(modelName string) (api.Connection, error)
 	return c.CommandBase.NewAPIRoot(c.store, controllerName, modelName)
 }
 
+// ModelUUIDs returns the model UUIDs for the given model names.
+func (c *ModelCommandBase) ModelUUIDs(modelNames []string) ([]string, error) {
+	controllerName, err := c.ControllerName()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return c.CommandBase.ModelUUIDs(c.ClientStore(), controllerName, modelNames)
+}
+
 // CurrentAccountDetails returns details of the account associated with
 // the current controller.
 func (c *ModelCommandBase) CurrentAccountDetails() (*jujuclient.AccountDetails, error) {