Merge lp:~axwalk/juju-core/cmd-juju-unset-env into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Introduce unset-environment subcommand

A new juju subcommand, unset-environment (with unset-env alias)
is introduced. This subcommand will take the existing environment
config, remove the specified keys, and update it in state. If
the specified keys have associated defaults, then they will be
assigned.

This is necessary for removing configuration for which an empty
value is not permitted, e.g. for string attributes which are
omitted by default, such as http-proxy.

Fixes lp:1295372

https://codereview.appspot.com/80380043/

Introduce unset-environment subcommand

A new juju subcommand, unset-environment (with unset-env alias)
is introduced. This subcommand will take the existing environment
config, remove the specified keys, and update it in state. If
the specified keys have associated defaults, then they will be
assigned.

This is necessary for removing configuration for which an empty
value is not permitted, e.g. for string attributes which are
omitted by default, such as http-proxy.

Fixes lp:1295372

https://codereview.appspot.com/80380043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+212771_code.launchpad.net,

Message:
Please take a look.

Description:
Introduce unset-environment subcommand

A new juju subcommand, unset-environment (with unset-env alias)
is introduced. This subcommand will take the existing environment
config, remove the specified keys, and update it in state. If
the specified keys have associated defaults, then they will be
assigned.

This is necessary for removing configuration for which an empty
value is not permitted, e.g. for string attributes which are
omitted by default, such as http-proxy.

Fixes lp:1295372

https://code.launchpad.net/~axwalk/juju-core/cmd-juju-unset-env/+merge/212771

(do not edit description out of merge proposal)

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

Affected files (+196, -2 lines):
   A [revision details]
   M cmd/juju/environment.go
   M cmd/juju/environment_test.go
   M cmd/juju/main.go
   M cmd/juju/main_test.go
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go

[Ian Booth (wallyworld)]

LGTM but I'd rather see more useful error message if an unset fails

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go#newcode192
cmd/juju/environment.go:192: Set one or more the environment
configuration attributes to its default value
s/Set/Reset ?

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go#newcode193
cmd/juju/environment.go:193: in a running Juju instance. Attributes
without defaults are removed.
Perhaps include an example to show they are space separated? Or mention
that in the help?

https://codereview.appspot.com/80380043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/80380043/diff/1/state/api/params/params.go#newcode594
state/api/params/params.go:594: type EnvironmentUnset struct {
I wish there were a generic string args struct. Maybe there is, not
sure.

https://codereview.appspot.com/80380043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/80380043/diff/1/state/apiserver/client/client_test.go#newcode1579
state/apiserver/client/client_test.go:1579: c.Assert(err,
gc.ErrorMatches, "type: expected string, got nothing")
I don't like this error message. I realise it's the output of validate
but I'd expect to see "cannot unset blah" or something if i were a user.
Perhaps we could just wrap the error so it reads `cannot unset
environment attribute "type": blah blah`

https://codereview.appspot.com/80380043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go#newcode192
cmd/juju/environment.go:192: Set one or more the environment
configuration attributes to its default value
On 2014/03/26 06:13:43, wallyworld wrote:
> s/Set/Reset ?

Done.

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go#newcode193
cmd/juju/environment.go:193: in a running Juju instance. Attributes
without defaults are removed.
On 2014/03/26 06:13:43, wallyworld wrote:
> Perhaps include an example to show they are space separated? Or
mention that in
> the help?

Clarified in documentation.

https://codereview.appspot.com/80380043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/80380043/diff/1/state/api/params/params.go#newcode594
state/api/params/params.go:594: type EnvironmentUnset struct {
On 2014/03/26 06:13:43, wallyworld wrote:
> I wish there were a generic string args struct. Maybe there is, not
sure.

Nope, that I can see. Anyway, having API-specific ones means the struct
can be updated with additional fields if necessary.

https://codereview.appspot.com/80380043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/80380043/diff/1/state/apiserver/client/client_test.go#newcode1579
state/apiserver/client/client_test.go:1579: c.Assert(err,
gc.ErrorMatches, "type: expected string, got nothing")
On 2014/03/26 06:13:43, wallyworld wrote:
> I don't like this error message. I realise it's the output of validate
but I'd
> expect to see "cannot unset blah" or something if i were a user.
Perhaps we
> could just wrap the error so it reads `cannot unset environment
attribute
> "type": blah blah`

I agree the error sucks. The operaton isn't that granular, though. We're
saying "make all these changes to the config", and then checking if the
result is any good.

You can't necessarily remove them one at a time, either. It's feasible
that there are co-requisite attributes.

Perhaps the right thing to do is to have a method on EnvironProvider
that returns a config with defaults for all attributes that have
defaults. Then the attribute removal code can just reset them back to
those values, or error if there's no default and it's required. That's a
big change, though, and not something I want to drag into this.

https://codereview.appspot.com/80380043/

=== modified file 'cmd/juju/environment.go'
--- cmd/juju/environment.go	2014-03-12 22:33:09 +0000
+++ cmd/juju/environment.go	2014-03-26 06:33:09 +0000
@@ -181,3 +181,65 @@
 	}
 	return err
 }
+
+// UnsetEnvironment
+type UnsetEnvironmentCommand struct {
+	cmd.EnvCommandBase
+	keys []string
+}
+
+const unsetEnvHelpDoc = `
+Reset one or more the environment configuration attributes to its default
+value in a running Juju instance.  Attributes without defaults are removed,
+and attempting to remove a required attribute with no default will result
+in an error.
+
+Multiple attributes may be removed at once; keys are space-separated.
+`
+
+func (c *UnsetEnvironmentCommand) Info() *cmd.Info {
+	return &cmd.Info{
+		Name:    "unset-environment",
+		Args:    "<environment key> ...",
+		Purpose: "unset environment values",
+		Doc:     strings.TrimSpace(unsetEnvHelpDoc),
+		Aliases: []string{"unset-env"},
+	}
+}
+
+func (c *UnsetEnvironmentCommand) Init(args []string) (err error) {
+	if len(args) == 0 {
+		return fmt.Errorf("No keys specified")
+	}
+	c.keys = args
+	return nil
+}
+
+// run1dot16 runs matches client.EnvironmentUnset using a direct DB
+// connection to maintain compatibility with an API server running 1.16 or
+// older (when EnvironmentUnset was not available). This fallback can be removed
+// when we no longer maintain 1.16 compatibility.
+func (c *UnsetEnvironmentCommand) run1dot16() error {
+	conn, err := juju.NewConnFromName(c.EnvName)
+	if err != nil {
+		return err
+	}
+	defer conn.Close()
+	return conn.State.UpdateEnvironConfig(nil, c.keys, nil)
+}
+
+func (c *UnsetEnvironmentCommand) Run(ctx *cmd.Context) error {
+	client, err := juju.NewAPIClientFromName(c.EnvName)
+	if err != nil {
+		return err
+	}
+	defer client.Close()
+
+	err = client.EnvironmentUnset(c.keys...)
+	if params.IsCodeNotImplemented(err) {
+		logger.Infof("EnvironmentUnset not supported by the API server, " +
+			"falling back to 1.16 compatibility mode (direct DB access)")
+		err = c.run1dot16()
+	}
+	return err
+}

=== modified file 'cmd/juju/environment_test.go'
--- cmd/juju/environment_test.go	2014-03-17 06:03:51 +0000
+++ cmd/juju/environment_test.go	2014-03-26 06:33:09 +0000
@@ -7,8 +7,10 @@
 	"fmt"
 	"strings"
 
+	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/environs/config"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/provider/dummy"
 	_ "launchpad.net/juju-core/provider/local"
@@ -178,3 +180,73 @@
 		c.Assert(err, gc.ErrorMatches, errorPattern)
 	}
 }
+
+type UnsetEnvironmentSuite struct {
+	jujutesting.RepoSuite
+}
+
+var _ = gc.Suite(&UnsetEnvironmentSuite{})
+
+var unsetEnvTests = []struct {
+	args       []string
+	err        string
+	expected   attributes
+	unexpected []string
+}{
+	{
+		args: []string{},
+		err:  "No keys specified",
+	}, {
+		args:       []string{"xyz", "xyz"},
+		unexpected: []string{"xyz"},
+	}, {
+		args: []string{"type", "xyz"},
+		err:  "type: expected string, got nothing",
+		expected: attributes{
+			"type": "dummy",
+			"xyz":  123,
+		},
+	}, {
+		args: []string{"syslog-port"},
+		expected: attributes{
+			"syslog-port": config.DefaultSyslogPort,
+		},
+	}, {
+		args:       []string{"xyz2", "xyz"},
+		unexpected: []string{"xyz"},
+	},
+}
+
+func (s *UnsetEnvironmentSuite) initConfig(c *gc.C) {
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{
+		"syslog-port": 1234,
+		"xyz":         123,
+	}, nil, nil)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *UnsetEnvironmentSuite) TestUnsetEnvironment(c *gc.C) {
+	for _, t := range unsetEnvTests {
+		c.Logf("testing unset-env %v", t.args)
+		s.initConfig(c)
+		_, err := testing.RunCommand(c, &UnsetEnvironmentCommand{}, t.args)
+		if t.err != "" {
+			c.Assert(err, gc.ErrorMatches, t.err)
+		} else {
+			c.Assert(err, gc.IsNil)
+		}
+		if len(t.expected)+len(t.unexpected) != 0 {
+			stateConfig, err := s.State.EnvironConfig()
+			c.Assert(err, gc.IsNil)
+			for k, v := range t.expected {
+				vstate, ok := stateConfig.AllAttrs()[k]
+				c.Assert(ok, jc.IsTrue)
+				c.Assert(vstate, gc.Equals, v)
+			}
+			for _, k := range t.unexpected {
+				_, ok := stateConfig.AllAttrs()[k]
+				c.Assert(ok, jc.IsFalse)
+			}
+		}
+	}
+}

=== modified file 'cmd/juju/main.go'
--- cmd/juju/main.go	2014-03-21 14:28:36 +0000
+++ cmd/juju/main.go	2014-03-26 06:33:09 +0000
@@ -108,6 +108,7 @@
 	jujucmd.Register(wrap(&SetConstraintsCommand{}))
 	jujucmd.Register(wrap(&GetEnvironmentCommand{}))
 	jujucmd.Register(wrap(&SetEnvironmentCommand{}))
+	jujucmd.Register(wrap(&UnsetEnvironmentCommand{}))
 	jujucmd.Register(wrap(&ExposeCommand{}))
 	jujucmd.Register(wrap(&SyncToolsCommand{}))
 	jujucmd.Register(wrap(&UnexposeCommand{}))

=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2014-03-26 01:22:46 +0000
+++ cmd/juju/main_test.go	2014-03-26 06:33:09 +0000
@@ -253,6 +253,8 @@
 	"terminate-machine", // alias for destroy-machine
 	"unexpose",
 	"unset",
+	"unset-env", // alias for unset-environment
+	"unset-environment",
 	"upgrade-charm",
 	"upgrade-juju",
 	"version",

=== modified file 'state/api/client.go'
--- state/api/client.go	2014-03-26 06:28:38 +0000
+++ state/api/client.go	2014-03-26 06:33:09 +0000
@@ -441,6 +441,12 @@
 	return c.call("EnvironmentSet", args, nil)
 }
 
+// EnvironmentUnset sets the given key-value pairs in the environment.
+func (c *Client) EnvironmentUnset(keys ...string) error {
+	args := params.EnvironmentUnset{Keys: keys}
+	return c.call("EnvironmentUnset", args, nil)
+}
+
 // SetEnvironAgentVersion sets the environment agent-version setting
 // to the given value.
 func (c *Client) SetEnvironAgentVersion(version version.Number) error {

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2014-03-26 04:13:52 +0000
+++ state/api/params/params.go	2014-03-26 06:33:09 +0000
@@ -589,6 +589,12 @@
 	Config map[string]interface{}
 }
 
+// EnvironmentUnset contains the arguments for EnvironmentUnset client API
+// call.
+type EnvironmentUnset struct {
+	Keys []string
+}
+
 // SetEnvironAgentVersion contains the arguments for
 // SetEnvironAgentVersion client API call.
 type SetEnvironAgentVersion struct {

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-03-26 04:13:52 +0000
+++ state/apiserver/client/client.go	2014-03-26 06:33:09 +0000
@@ -802,13 +802,19 @@
 		}
 		return nil
 	}
-
 	// TODO(waigani) 2014-3-11 #1167616
 	// Add a txn retry loop to ensure that the settings on disk have not
 	// changed underneath us.
-
 	return c.api.state.UpdateEnvironConfig(args.Config, nil, checkAgentVersion)
+}
 
+// EnvironmentUnset implements the server-side part of the
+// set-environment CLI command.
+func (c *Client) EnvironmentUnset(args params.EnvironmentUnset) error {
+	// TODO(waigani) 2014-3-11 #1167616
+	// Add a txn retry loop to ensure that the settings on disk have not
+	// changed underneath us.
+	return c.api.state.UpdateEnvironConfig(nil, args.Keys, nil)
 }
 
 // SetEnvironAgentVersion sets the environment agent version.

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-03-26 00:24:30 +0000
+++ state/apiserver/client/client_test.go	2014-03-26 06:33:09 +0000
@@ -1542,6 +1542,47 @@
 	c.Assert(err, gc.IsNil)
 }
 
+func (s *clientSuite) TestClientEnvironmentUnset(c *gc.C) {
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"abc": 123}, nil, nil)
+	c.Assert(err, gc.IsNil)
+	envConfig, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	_, found := envConfig.AllAttrs()["abc"]
+	c.Assert(found, jc.IsTrue)
+
+	err = s.APIState.Client().EnvironmentUnset("abc")
+	c.Assert(err, gc.IsNil)
+	envConfig, err = s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	_, found = envConfig.AllAttrs()["abc"]
+	c.Assert(found, jc.IsFalse)
+}
+
+func (s *clientSuite) TestClientEnvironmentUnsetMissing(c *gc.C) {
+	// It's okay to unset a non-existent attribute.
+	err := s.APIState.Client().EnvironmentUnset("not_there")
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *clientSuite) TestClientEnvironmentUnsetError(c *gc.C) {
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"abc": 123}, nil, nil)
+	c.Assert(err, gc.IsNil)
+	envConfig, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	_, found := envConfig.AllAttrs()["abc"]
+	c.Assert(found, jc.IsTrue)
+
+	// "type" may not be removed, and this will cause an error.
+	// If any one attribute's removal causes an error, there
+	// should be no change.
+	err = s.APIState.Client().EnvironmentUnset("abc", "type")
+	c.Assert(err, gc.ErrorMatches, "type: expected string, got nothing")
+	envConfig, err = s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	_, found = envConfig.AllAttrs()["abc"]
+	c.Assert(found, jc.IsTrue)
+}
+
 func (s *clientSuite) TestClientFindTools(c *gc.C) {
 	result, err := s.APIState.Client().FindTools(2, -1, "", "")
 	c.Assert(err, gc.IsNil)