Merge pull request juju#6442 from wallyworld/fix-modeldefaults

Fix region handling in model-defaults --reset A few fixes here: 1. juju model-defaults region --reset foo Fixed to work properly with region specified. The fix was to revert a change made elsewhere in sysCmdWrapper 2. Fix case of table headers in model-defaults command 3. Improve error message for juju grant Also added feature tests for model-defaults command to test the issue fixed.
gabriel-samfira · Oct 13, 2016 · 12e978b · 12e978b
2 parents 4410306 + dfa6eff
commit 12e978b
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 38 deletions.
diff --git a/cmd/juju/cloud/updatecredential_test.go b/cmd/juju/cloud/updatecredential_test.go
@@ -24,7 +24,13 @@ type updateCredentialSuite struct {
 var _ = gc.Suite(&updateCredentialSuite{})
 
 func (s *updateCredentialSuite) TestBadArgs(c *gc.C) {
-	cmd := cloud.NewUpdateCredentialCommand()
+	store := &jujuclienttesting.MemStore{
+		Controllers: map[string]jujuclient.ControllerDetails{
+			"controller": {},
+		},
+		CurrentControllerName: "controller",
+	}
+	cmd := cloud.NewUpdateCredentialCommandForTest(store, nil)
 	_, err := testing.RunCommand(c, cmd)
 	c.Assert(err, gc.ErrorMatches, "Usage: juju update-credential <cloud-name> <credential-name>")
 	_, err = testing.RunCommand(c, cmd, "cloud", "credential", "extra")

diff --git a/cmd/juju/model/defaultscommand.go b/cmd/juju/model/defaultscommand.go
@@ -40,8 +40,12 @@ Examples:
     juju model-defaults http-proxy
     juju model-defaults -m mymodel type
     juju model-defaults ftp-proxy=10.0.0.1:8000
+    juju model-defaults aws/us-east-1 ftp-proxy=10.0.0.1:8000
+    juju model-defaults us-east-1 ftp-proxy=10.0.0.1:8000
     juju model-defaults -m othercontroller:mymodel default-series=yakkety test-mode=false
     juju model-defaults --reset default-series test-mode
+    juju model-defaults aws/us-east-1 --reset http-proxy
+    juju model-defaults us-east-1 --reset http-proxy
 
 See also:
     models
@@ -596,7 +600,7 @@ func formatDefaultConfigTabular(writer io.Writer, value interface{}) error {
 	}
 	sort.Strings(valueNames)
 
-	w.Println("ATTRIBUTE", "DEFAULT", "CONTROLLER")
+	w.Println("Attribute", "Default", "Controller")
 
 	for _, name := range valueNames {
 		info := defaultValues[name]

diff --git a/cmd/juju/model/defaultscommand_test.go b/cmd/juju/model/defaultscommand_test.go
@@ -307,7 +307,7 @@ func (s *DefaultsCommandSuite) TestGetSingleValue(c *gc.C) {
 
 	output := strings.TrimSpace(testing.Stdout(context))
 	expected := "" +
-		"ATTRIBUTE       DEFAULT      CONTROLLER\n" +
+		"Attribute       Default      Controller\n" +
 		"attr2           -            bar\n" +
 		"  dummy-region  dummy-value  -"
 	c.Assert(output, gc.Equals, expected)
@@ -353,7 +353,7 @@ func (s *DefaultsCommandSuite) TestGetAllValuesTabular(c *gc.C) {
 
 	output := strings.TrimSpace(testing.Stdout(context))
 	expected := "" +
-		"ATTRIBUTE       DEFAULT      CONTROLLER\n" +
+		"Attribute       Default      Controller\n" +
 		"attr            foo          -\n" +
 		"attr2           -            bar\n" +
 		"  dummy-region  dummy-value  -"

diff --git a/cmd/juju/model/grantrevoke.go b/cmd/juju/model/grantrevoke.go
@@ -81,9 +81,9 @@ See also:
 type accessCommand struct {
 	modelcmd.ControllerCommandBase
 
-	User        string
-	ModelNames  []string
-	ModelAccess string
+	User       string
+	ModelNames []string
+	Access     string
 }
 
 // Init implements cmd.Command.
@@ -98,16 +98,23 @@ func (c *accessCommand) Init(args []string) error {
 
 	c.User = args[0]
 	c.ModelNames = args[2:]
-	c.ModelAccess = args[1]
+	c.Access = args[1]
 	// Special case for backwards compatibility.
-	if c.ModelAccess == "addmodel" {
-		c.ModelAccess = "add-model"
+	if c.Access == "addmodel" {
+		c.Access = "add-model"
 	}
 	if len(c.ModelNames) > 0 {
-		err := permission.ValidateModelAccess(permission.Access(c.ModelAccess))
-		if err != nil {
-			return err
+		if err := permission.ValidateControllerAccess(permission.Access(c.Access)); err == nil {
+			return errors.Errorf("You have specified a controller access permission %q.\n"+
+				"If you intended to change controller access, do not specify any model names.\n"+
+				"See 'juju help grant'.", c.Access)
 		}
+		return permission.ValidateModelAccess(permission.Access(c.Access))
+	}
+	if err := permission.ValidateModelAccess(permission.Access(c.Access)); err == nil {
+		return errors.Errorf("You have specified a model access permission %q.\n"+
+			"If you intended to change model access, you need to specify one or more model names.\n"+
+			"See 'juju help grant'.", c.Access)
 	}
 	return nil
 }
@@ -171,7 +178,7 @@ func (c *grantCommand) runForController() error {
 	}
 	defer client.Close()
 
-	return block.ProcessBlockedError(client.GrantController(c.User, c.ModelAccess), block.BlockChange)
+	return block.ProcessBlockedError(client.GrantController(c.User, c.Access), block.BlockChange)
 }
 
 func (c *grantCommand) runForModel() error {
@@ -185,7 +192,7 @@ func (c *grantCommand) runForModel() error {
 	if err != nil {
 		return err
 	}
-	return block.ProcessBlockedError(client.GrantModel(c.User, c.ModelAccess, models...), block.BlockChange)
+	return block.ProcessBlockedError(client.GrantModel(c.User, c.Access, models...), block.BlockChange)
 }
 
 // NewRevokeCommand returns a new revoke command.
@@ -247,7 +254,7 @@ func (c *revokeCommand) runForController() error {
 	}
 	defer client.Close()
 
-	return block.ProcessBlockedError(client.RevokeController(c.User, c.ModelAccess), block.BlockChange)
+	return block.ProcessBlockedError(client.RevokeController(c.User, c.Access), block.BlockChange)
 }
 
 func (c *revokeCommand) runForModel() error {
@@ -261,5 +268,5 @@ func (c *revokeCommand) runForModel() error {
 	if err != nil {
 		return err
 	}
-	return block.ProcessBlockedError(client.RevokeModel(c.User, c.ModelAccess, models...), block.BlockChange)
+	return block.ProcessBlockedError(client.RevokeModel(c.User, c.Access, models...), block.BlockChange)
 }
diff --git a/cmd/juju/model/grantrevoke_test.go b/cmd/juju/model/grantrevoke_test.go
@@ -4,6 +4,8 @@
 package model_test
 
 import (
+	"strings"
+
 	"github.com/juju/cmd"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
@@ -127,7 +129,7 @@ func (s *grantSuite) TestInitGrantAddModel(c *gc.C) {
 	// The backwards-compatible case, addmodel.
 	err = testing.InitCommand(wrappedCmd, []string{"bob", "addmodel"})
 	c.Check(err, jc.ErrorIsNil)
-	c.Assert(grantCmd.ModelAccess, gc.Equals, "add-model")
+	c.Assert(grantCmd.Access, gc.Equals, "add-model")
 }
 
 type revokeSuite struct {
@@ -171,7 +173,21 @@ func (s *grantSuite) TestInitRevokeAddModel(c *gc.C) {
 	// The backwards-compatible case, addmodel.
 	err = testing.InitCommand(wrappedCmd, []string{"bob", "addmodel"})
 	c.Check(err, jc.ErrorIsNil)
-	c.Assert(revokeCmd.ModelAccess, gc.Equals, "add-model")
+	c.Assert(revokeCmd.Access, gc.Equals, "add-model")
+}
+
+func (s *grantSuite) TestModelAccessForController(c *gc.C) {
+	wrappedCmd, _ := model.NewRevokeCommandForTest(s.fake, s.store)
+	err := testing.InitCommand(wrappedCmd, []string{"bob", "write"})
+	msg := strings.Replace(err.Error(), "\n", "", -1)
+	c.Check(msg, gc.Matches, `You have specified a model access permission "write".*`)
+}
+
+func (s *grantSuite) TestControllerAccessForModel(c *gc.C) {
+	wrappedCmd, _ := model.NewRevokeCommandForTest(s.fake, s.store)
+	err := testing.InitCommand(wrappedCmd, []string{"bob", "superuser", "default"})
+	msg := strings.Replace(err.Error(), "\n", "", -1)
+	c.Check(msg, gc.Matches, `You have specified a controller access permission "superuser".*`)
 }
 
 type fakeGrantRevokeAPI struct {

diff --git a/cmd/modelcmd/controller.go b/cmd/modelcmd/controller.go
@@ -270,10 +270,6 @@ func (w *sysCommandWrapper) Init(args []string) error {
 	store = QualifyingClientStore{store}
 	w.SetClientStore(store)
 
-	return w.ControllerCommand.Init(args)
-}
-
-func (w *sysCommandWrapper) Run(ctx *cmd.Context) error {
 	if w.setControllerFlags {
 		if w.controllerName == "" && w.useDefaultController {
 			store := w.ClientStore()
@@ -292,7 +288,7 @@ func (w *sysCommandWrapper) Run(ctx *cmd.Context) error {
 			return translateControllerError(w.ClientStore(), err)
 		}
 	}
-	return w.ControllerCommand.Run(ctx)
+	return w.ControllerCommand.Init(args)
 }
 
 func translateControllerError(store jujuclient.ClientStore, err error) error {

diff --git a/cmd/modelcmd/controller_test.go b/cmd/modelcmd/controller_test.go
@@ -23,9 +23,7 @@ type ControllerCommandSuite struct {
 var _ = gc.Suite(&ControllerCommandSuite{})
 
 func (s *ControllerCommandSuite) TestControllerCommandNoneSpecified(c *gc.C) {
-	cmd, _, err := initTestControllerCommand(c, nil)
-	c.Assert(err, jc.ErrorIsNil)
-	err = cmd.Run(nil)
+	_, _, err := initTestControllerCommand(c, nil)
 	c.Assert(errors.Cause(err), gc.Equals, modelcmd.ErrNoControllersDefined)
 }
 

diff --git a/featuretests/cmd_juju_model_test.go b/featuretests/cmd_juju_model_test.go
@@ -16,6 +16,8 @@ import (
 
 	"github.com/juju/juju/cmd/juju/commands"
 	"github.com/juju/juju/core/description"
+	"github.com/juju/juju/environs"
+	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/feature"
 	jujutesting "github.com/juju/juju/juju/testing"
 	"github.com/juju/juju/permission"
@@ -106,25 +108,78 @@ func (s *cmdModelSuite) TestModelUsersCmd(c *gc.C) {
 
 }
 
-func (s *cmdModelSuite) TestGet(c *gc.C) {
+func (s *cmdModelSuite) TestModelConfigGet(c *gc.C) {
 	err := s.State.UpdateModelConfig(map[string]interface{}{"special": "known"}, nil, nil)
 	c.Assert(err, jc.ErrorIsNil)
 
 	context := s.run(c, "model-config", "special")
 	c.Assert(testing.Stdout(context), gc.Equals, "known\n")
 }
 
-func (s *cmdModelSuite) TestSet(c *gc.C) {
+func (s *cmdModelSuite) TestModelConfigSet(c *gc.C) {
 	s.run(c, "model-config", "special=known")
-	s.assertEnvValue(c, "special", "known")
+	s.assertModelValue(c, "special", "known")
 }
 
-func (s *cmdModelSuite) TestUnset(c *gc.C) {
+func (s *cmdModelSuite) TestModelConfigReset(c *gc.C) {
 	err := s.State.UpdateModelConfig(map[string]interface{}{"special": "known"}, nil, nil)
 	c.Assert(err, jc.ErrorIsNil)
 
 	s.run(c, "model-config", "--reset", "special")
-	s.assertEnvValueMissing(c, "special")
+	s.assertModelValueMissing(c, "special")
+}
+
+func (s *cmdModelSuite) TestModelDefaultsGet(c *gc.C) {
+	err := s.State.UpdateModelConfigDefaultValues(map[string]interface{}{"special": "known"}, nil, nil)
+	c.Assert(err, jc.ErrorIsNil)
+
+	context := s.run(c, "model-defaults", "special")
+	c.Assert(testing.Stdout(context), gc.Equals, `
+Attribute  Default  Controller
+special    -        known
+
+`[1:])
+}
+
+func (s *cmdModelSuite) TestModelDefaultsSet(c *gc.C) {
+	s.run(c, "model-defaults", "special=known")
+	defaults, err := s.State.ModelConfigDefaultValues()
+	c.Assert(err, jc.ErrorIsNil)
+	value, found := defaults["special"]
+	c.Assert(found, jc.IsTrue)
+	c.Assert(value.Controller, gc.Equals, "known")
+}
+
+func (s *cmdModelSuite) TestModelDefaultsSetRegion(c *gc.C) {
+	s.run(c, "model-defaults", "dummy/dummy-region", "special=known")
+	defaults, err := s.State.ModelConfigDefaultValues()
+	c.Assert(err, jc.ErrorIsNil)
+	value, found := defaults["special"]
+	c.Assert(found, jc.IsTrue)
+	c.Assert(value.Controller, gc.IsNil)
+	c.Assert(value.Regions, jc.SameContents, []config.RegionDefaultValue{{"dummy-region", "known"}})
+}
+
+func (s *cmdModelSuite) TestModelDefaultsReset(c *gc.C) {
+	err := s.State.UpdateModelConfigDefaultValues(map[string]interface{}{"special": "known"}, nil, nil)
+	c.Assert(err, jc.ErrorIsNil)
+
+	s.run(c, "model-defaults", "--reset", "special")
+	defaults, err := s.State.ModelConfigDefaultValues()
+	c.Assert(err, jc.ErrorIsNil)
+	_, found := defaults["special"]
+	c.Assert(found, jc.IsFalse)
+}
+
+func (s *cmdModelSuite) TestModelDefaultsResetRegion(c *gc.C) {
+	err := s.State.UpdateModelConfigDefaultValues(map[string]interface{}{"special": "known"}, nil, &environs.RegionSpec{"dummy", "dummy-region"})
+	c.Assert(err, jc.ErrorIsNil)
+
+	s.run(c, "model-defaults", "dummy-region", "--reset", "special")
+	defaults, err := s.State.ModelConfigDefaultValues()
+	c.Assert(err, jc.ErrorIsNil)
+	_, found := defaults["special"]
+	c.Assert(found, jc.IsFalse)
 }
 
 func (s *cmdModelSuite) TestRetryProvisioning(c *gc.C) {
@@ -170,17 +225,17 @@ func (s *cmdModelSuite) TestDumpModelDB(c *gc.C) {
 	c.Assert(modelMap["name"], gc.Equals, "controller")
 }
 
-func (s *cmdModelSuite) assertEnvValue(c *gc.C, key string, expected interface{}) {
-	envConfig, err := s.State.ModelConfig()
+func (s *cmdModelSuite) assertModelValue(c *gc.C, key string, expected interface{}) {
+	modelConfig, err := s.State.ModelConfig()
 	c.Assert(err, jc.ErrorIsNil)
-	value, found := envConfig.AllAttrs()[key]
+	value, found := modelConfig.AllAttrs()[key]
 	c.Assert(found, jc.IsTrue)
 	c.Assert(value, gc.Equals, expected)
 }
 
-func (s *cmdModelSuite) assertEnvValueMissing(c *gc.C, key string) {
-	envConfig, err := s.State.ModelConfig()
+func (s *cmdModelSuite) assertModelValueMissing(c *gc.C, key string) {
+	modelConfig, err := s.State.ModelConfig()
 	c.Assert(err, jc.ErrorIsNil)
-	_, found := envConfig.AllAttrs()[key]
+	_, found := modelConfig.AllAttrs()[key]
 	c.Assert(found, jc.IsFalse)
 }