Merge lp:~wallyworld/juju-core/deprecate-tools-url into lp:~go-bot/juju-core/trunk

[Ian Booth]

Add back support for deprecated tools-url

Removing tools-url from config too early breaks
upgrades so add it back for the next release.

https://codereview.appspot.com/24340043/

Add back support for deprecated tools-url

Removing tools-url from config too early breaks
upgrades so add it back for the next release.

https://codereview.appspot.com/24340043/

[Ian Booth (wallyworld)]

Reviewers: mp+194649_code.launchpad.net,

Message:
Please take a look.

Description:
Add back support for deprecated tools-url

Removing tools-url from config too early breaks
upgrades so add it back for the next release.

https://code.launchpad.net/~wallyworld/juju-core/deprecate-tools-url/+merge/194649

(do not edit description out of merge proposal)

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

Affected files (+117, -3 lines):
   A [revision details]
   M environs/config/config.go
   M environs/config/config_test.go

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/24340043/

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/deprecate-tools-url into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.131s
ok launchpad.net/juju-core/agent/tools 0.224s
ok launchpad.net/juju-core/bzr 6.920s
ok launchpad.net/juju-core/cert 3.767s
ok launchpad.net/juju-core/charm 0.557s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.026s
ok launchpad.net/juju-core/cmd 0.226s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 177.696s
ok launchpad.net/juju-core/cmd/jujud 55.687s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.814s
ok launchpad.net/juju-core/constraints 0.027s
ok launchpad.net/juju-core/container/lxc 0.289s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.272s
ok launchpad.net/juju-core/environs 3.156s
ok launchpad.net/juju-core/environs/bootstrap 5.013s
ok launchpad.net/juju-core/environs/cloudinit 0.527s
ok launchpad.net/juju-core/environs/config 1.419s
ok launchpad.net/juju-core/environs/configstore 0.040s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.989s
ok launchpad.net/juju-core/environs/imagemetadata 0.500s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.051s
ok launchpad.net/juju-core/environs/jujutest 0.248s
ok launchpad.net/juju-core/environs/manual 5.727s
ok launchpad.net/juju-core/environs/simplestreams 0.348s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.065s
ok launchpad.net/juju-core/environs/storage 1.133s
ok launchpad.net/juju-core/environs/sync 28.383s
ok launchpad.net/juju-core/environs/testing 0.186s
ok launchpad.net/juju-core/environs/tools 6.671s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.021s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 17.879s
ok launchpad.net/juju-core/juju/osenv 0.018s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.024s
ok launchpad.net/juju-core/names 0.024s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 6.427s
ok launchpad.net/juju-core/provider/common 0.333s
ok launchpad.net/juju-core/provider/dummy 21.159s
ok launchpad.net/juju-core/provider/ec2 5.056s
ok launchpad.net/juju-core/provider/ec2/httpstorage 0.233s
ok launchpad.net/juju-core/provider/local 2.196s
ok launchpad.net/juju-core/provider/maas 9.985s
ok launchpad.net/juju-core/provider/null 1.347s
ok launchpad.net/juju-core/provider/openstack 11.639s
ok launchpad.net/juju-core/rpc 0.077s
ok launchpad.net/juju-core/rpc/json...

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2013-11-04 02:01:19 +0000
+++ environs/config/config.go	2013-11-11 02:04:03 +0000
@@ -244,6 +244,28 @@
 			}
 		}
 	}
+
+	// The tools url has changed so see if the old one is still in use.
+	if oldToolsURL := cfg.m["tools-url"]; oldToolsURL != nil && oldToolsURL.(string) != "" {
+		_, newToolsSpecified := cfg.ToolsURL()
+		var msg string
+		if newToolsSpecified {
+			msg = fmt.Sprintf(
+				"Config attribute %q (%v) is deprecated and will be ignored since\n"+
+					"the new tools URL attribute %q has also been used.\n"+
+					"The attribute % should be removed from your configuration.",
+				"tools-url", oldToolsURL, "tools-metadata-url", "tools-url")
+		} else {
+			msg = fmt.Sprintf(
+				"Config attribute %q (%v) is deprecated.\n"+
+					"The location to find tools is now specified using the %q attribute.\n"+
+					"Your configuration should be upddated to set %q as follows\n%v: %v.",
+				"tools-url", oldToolsURL, "tools-metadata-url", "tools-metadata-url", "tools-metadata-url", oldToolsURL)
+			cfg.m["tools-metadata-url"] = oldToolsURL
+		}
+		logger.Warningf(msg)
+		delete(cfg.m, "tools-url")
+	}
 	return nil
 }
 
@@ -495,6 +517,9 @@
 	"state-port":                schema.ForceInt(),
 	"api-port":                  schema.ForceInt(),
 	"logging-config":            schema.String(),
+
+	// Deprecated fields, retain for backwards compatibility.
+	"tools-url": schema.String(),
 }
 
 // alwaysOptional holds configuration defaults for attributes that may
@@ -514,6 +539,9 @@
 	"ca-private-key-path":  schema.Omit,
 	"logging-config":       schema.Omit,
 
+	// Deprecated fields, retain for backwards compatibility.
+	"tools-url": schema.Omit,
+
 	// For backward compatibility reasons, the following
 	// attributes default to empty strings rather than being
 	// omitted.

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2013-11-04 02:01:19 +0000
+++ environs/config/config_test.go	2013-11-11 02:04:03 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"fmt"
+	"strings"
 	stdtesting "testing"
 	"time"
 
@@ -79,6 +80,23 @@
 			"tools-metadata-url": "tools-metadata-url",
 		},
 	}, {
+		about:       "Deprecated tools metadata URL used",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":      "my-type",
+			"name":      "my-name",
+			"tools-url": "tools-metadata-url",
+		},
+	}, {
+		about:       "Deprecated tools metadata URL ignored",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":               "my-type",
+			"name":               "my-name",
+			"tools-metadata-url": "tools-metadata-url",
+			"tools-url":          "ignore-me",
+		},
+	}, {
 		about:       "Explicit series",
 		useDefaults: config.UseDefaults,
 		attrs: testing.Attrs{
@@ -745,13 +763,21 @@
 	} else {
 		c.Assert(urlPresent, jc.IsFalse)
 	}
-	url, urlPresent = cfg.ToolsURL()
-	if v, _ := test.attrs["tools-metadata-url"].(string); v != "" {
-		c.Assert(url, gc.Equals, v)
+	toolsURL, urlPresent := cfg.ToolsURL()
+	deprecatedToolsURLTestValue, deprecatedURLPresent := test.attrs["tools-url"]
+	toolsURLTestValue := test.attrs["tools-metadata-url"]
+	if toolsURLTestValue == nil {
+		toolsURLTestValue = deprecatedToolsURLTestValue
+	}
+	if toolsURLTestValue != nil && toolsURLTestValue != "" {
+		c.Assert(toolsURL, gc.Equals, toolsURLTestValue)
 		c.Assert(urlPresent, jc.IsTrue)
 	} else {
 		c.Assert(urlPresent, jc.IsFalse)
+		c.Assert(deprecatedURLPresent, jc.IsFalse)
 	}
+	_, urlPresent = cfg.AllAttrs()["tools-url"]
+	c.Assert(urlPresent, jc.IsFalse)
 }
 
 func (s *ConfigSuite) TestConfigAttrs(c *gc.C) {
@@ -1101,3 +1127,61 @@
 MIIBOgIBAAJAZabKgKInuOxj5vDWLwHHQtK3/45KB+32D15w94Nt83BmuGxo90lw
 -----END CERTIFICATE-----
 `[1:]
+
+type ConfigDeprecationSuite struct {
+	ConfigSuite
+	writer *testWriter
+}
+
+var _ = gc.Suite(&ConfigDeprecationSuite{})
+
+func (s *ConfigDeprecationSuite) SetUpTest(c *gc.C) {
+	s.ConfigSuite.SetUpTest(c)
+}
+
+func (s *ConfigDeprecationSuite) TearDownTest(c *gc.C) {
+	s.ConfigSuite.TearDownTest(c)
+}
+
+func (s *ConfigDeprecationSuite) setupLogger(c *gc.C) {
+	var err error
+	s.writer = &testWriter{}
+	err = loggo.RegisterWriter("test", s.writer, loggo.WARNING)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *ConfigDeprecationSuite) resetLogger(c *gc.C) {
+	_, _, err := loggo.RemoveWriter("test")
+	c.Assert(err, gc.IsNil)
+}
+
+type testWriter struct {
+	messages []string
+}
+
+func (t *testWriter) Write(level loggo.Level, module, filename string, line int, timestamp time.Time, message string) {
+	t.messages = append(t.messages, message)
+}
+
+func (s *ConfigDeprecationSuite) setupEnv(c *gc.C, deprecatedKey, value string) {
+	attrs := testing.FakeConfig().Merge(testing.Attrs{
+		"name":        "testenv",
+		"type":        "openstack",
+		deprecatedKey: value,
+	})
+	_, err := config.New(config.NoDefaults, attrs)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *ConfigDeprecationSuite) TestDeprecationWarnings(c *gc.C) {
+	for attr, value := range map[string]string{
+		"tools-url": "foo",
+	} {
+		s.setupLogger(c)
+		s.setupEnv(c, attr, value)
+		s.resetLogger(c)
+		stripped := strings.Replace(s.writer.messages[0], "\n", "", -1)
+		expected := fmt.Sprintf(`.*Config attribute "%s" \(%s\) is deprecated.*`, attr, value)
+		c.Assert(stripped, gc.Matches, expected)
+	}
+}