Merge lp:~wallyworld/juju-core/log-deprecated-config-warnings into lp:~go-bot/juju-core/trunk

[Ian Booth]

Fix config depcared attr warnings

Deprecated config warnings are being logged
multiple times. A previous fix moved the entire
deprecated config processing logic so that the
warnings were only logged once. However this broke
upgrades.

This new fix moves the logging of the deprecation
warnings but leaves the processing of the depcated
attributes in the config Validate method so
upgrades continue to work.

https://codereview.appspot.com/37720049/

Fix config depcared attr warnings

Deprecated config warnings are being logged
multiple times. A previous fix moved the entire
deprecated config processing logic so that the
warnings were only logged once. However this broke
upgrades.

This new fix moves the logging of the deprecation
warnings but leaves the processing of the depcated
attributes in the config Validate method so
upgrades continue to work.

https://codereview.appspot.com/37720049/

[Ian Booth (wallyworld)]

Reviewers: mp+199597_code.launchpad.net,

Message:
Please take a look.

Description:
Fix config depcared attr warnings

Deprecated config warnings are being logged
multiple times. A previous fix moved the entire
deprecated config processing logic so that the
warnings were only logged once. However this broke
upgrades.

This new fix moves the logging of the deprecation
warnings but leaves the processing of the depcated
attributes in the config Validate method so
upgrades continue to work.

https://code.launchpad.net/~wallyworld/juju-core/log-deprecated-config-warnings/+merge/199597

(do not edit description out of merge proposal)

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

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

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/37720049/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/37720049/diff/1/environs/config/config.go#newcode190
environs/config/config.go:190: cfg.m["tools-metadata-url"] = oldToolsURL
Doesn't doing this cause the second of the deprecation warnings to never
be hit?

https://codereview.appspot.com/37720049/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/37720049/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/37720049/diff/1/environs/config/config.go#newcode190
environs/config/config.go:190: cfg.m["tools-metadata-url"] = oldToolsURL
On 2013/12/19 02:28:12, axw wrote:
> Doesn't doing this cause the second of the deprecation warnings to
never be hit?

This code is executed after the warnings are logged. The warnings are
logged as the environment is parsed from the yaml. The config validation
is done after that as the environment is constructed.

https://codereview.appspot.com/37720049/

[Andrew Wilkins (axwalk)]

LGTM apart from the removed test

https://codereview.appspot.com/37720049/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/37720049/diff/1/environs/config/config.go#newcode190
environs/config/config.go:190: cfg.m["tools-metadata-url"] = oldToolsURL
On 2013/12/19 02:43:33, wallyworld wrote:
> On 2013/12/19 02:28:12, axw wrote:
> > Doesn't doing this cause the second of the deprecation warnings to
never be
> hit?

> This code is executed after the warnings are logged. The warnings are
logged as
> the environment is parsed from the yaml. The config validation is done
after
> that as the environment is constructed.

Got it. Sorry, wasn't looking at the full context.

https://codereview.appspot.com/37720049/diff/1/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/37720049/diff/1/environs/config_test.go#oldcode91
environs/config_test.go:91: func (*suite)
TestNoWarningForDeprecatedButUnusedEnv(c *gc.C) {
I think this is still relevant?

https://codereview.appspot.com/37720049/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/37720049/diff/1/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/37720049/diff/1/environs/config_test.go#oldcode91
environs/config_test.go:91: func (*suite)
TestNoWarningForDeprecatedButUnusedEnv(c *gc.C) {
On 2013/12/19 02:56:57, axw wrote:
> I think this is still relevant?

Yes it is. I messed up merging back my previous work into the trunk
which had the code reverted.

https://codereview.appspot.com/37720049/

=== modified file 'environs/config.go'
--- environs/config.go	2013-12-18 08:45:42 +0000
+++ environs/config.go	2013-12-19 03:01:43 +0000
@@ -68,6 +68,28 @@
 	if err := validateEnvironmentKind(attrs); err != nil {
 		return nil, err
 	}
+
+	// If deprecated config attributes are used, log warnings so the user can know
+	// that they need to be fixed.
+	if oldToolsURL := attrs["tools-url"]; oldToolsURL != nil && oldToolsURL.(string) != "" {
+		_, newToolsSpecified := attrs["tools-metadata-url"]
+		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 %q 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 updated to set %q as follows\n%v: %v.",
+				"tools-url", oldToolsURL, "tools-metadata-url", "tools-metadata-url", "tools-metadata-url", oldToolsURL)
+		}
+		logger.Warningf(msg)
+	}
+
 	cfg, err := config.New(config.UseDefaults, attrs)
 	if err != nil {
 		return nil, err

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2013-12-18 08:45:42 +0000
+++ environs/config/config.go	2013-12-19 03:01:43 +0000
@@ -177,6 +177,24 @@
 	}
 }
 
+// processDeprecatedAttributes ensures that the config is set up so that it works
+// correctly when used with older versions of Juju which require that deprecated
+// attribute values still be used.
+func (cfg *Config) processDeprecatedAttributes() {
+	// The tools url has changed so ensure that both old and new values are in the config so that
+	// upgrades work. "tools-url" is the old attribute name.
+	if oldToolsURL := cfg.m["tools-url"]; oldToolsURL != nil && oldToolsURL.(string) != "" {
+		_, newToolsSpecified := cfg.ToolsURL()
+		// Ensure the new attribute name "tools-metadata-url" is set.
+		if !newToolsSpecified {
+			cfg.m["tools-metadata-url"] = oldToolsURL
+		}
+	}
+	// Even if the user has edited their environment yaml to remove the deprecated tools-url value,
+	// we still want it in the config for upgrades.
+	cfg.m["tools-url"], _ = cfg.ToolsURL()
+}
+
 // Validate ensures that config is a valid configuration.  If old is not nil,
 // it holds the previous environment configuration for consideration when
 // validating changes.
@@ -259,29 +277,7 @@
 		}
 	}
 
-	// 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 %q 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 updated 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)
-	}
-	// Even if the user has edited their environment yaml to remove the deprecated tools-url value,
-	// we still want it in the config for upgrades.
-	cfg.m["tools-url"], _ = cfg.ToolsURL()
+	cfg.processDeprecatedAttributes()
 	return nil
 }
 

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2013-12-18 08:45:42 +0000
+++ environs/config/config_test.go	2013-12-19 03:01:43 +0000
@@ -6,7 +6,6 @@
 import (
 	"fmt"
 	"regexp"
-	"strings"
 	stdtesting "testing"
 	"time"
 
@@ -1232,61 +1231,3 @@
 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)
-	}
-}

=== modified file 'environs/config_test.go'
--- environs/config_test.go	2013-12-18 08:45:42 +0000
+++ environs/config_test.go	2013-12-19 03:01:43 +0000
@@ -4,9 +4,11 @@
 package environs_test
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"sort"
+	"strings"
 
 	gc "launchpad.net/gocheck"
 	"launchpad.net/loggo"
@@ -300,3 +302,62 @@
 	expect["ca-private-key"] = ""
 	c.Assert(cfg1.AllAttrs(), gc.DeepEquals, expect)
 }
+
+type ConfigDeprecationSuite struct {
+	suite
+	writer *loggo.TestWriter
+}
+
+var _ = gc.Suite(&ConfigDeprecationSuite{})
+
+func (s *ConfigDeprecationSuite) setupLogger(c *gc.C) func() {
+	var err error
+	s.writer = &loggo.TestWriter{}
+	err = loggo.RegisterWriter("test", s.writer, loggo.WARNING)
+	c.Assert(err, gc.IsNil)
+	return func() {
+		_, _, err := loggo.RemoveWriter("test")
+		c.Assert(err, gc.IsNil)
+	}
+}
+
+func (s *ConfigDeprecationSuite) checkDeprecationWarning(c *gc.C, attrs testing.Attrs, expectedMsg string) {
+	defer testing.MakeFakeHomeNoEnvironments(c, "only").Restore()
+	content := `
+environments:
+    deprecated:
+        type: dummy
+        state-server: false
+`
+	restore := s.setupLogger(c)
+	defer restore()
+
+	envs, err := environs.ReadEnvironsBytes([]byte(content))
+	c.Check(err, gc.IsNil)
+	environs.UpdateEnvironAttrs(envs, "deprecated", attrs)
+	_, err = envs.Config("deprecated")
+	c.Check(err, gc.IsNil)
+	c.Assert(s.writer.Log, gc.HasLen, 1)
+	stripped := strings.Replace(s.writer.Log[0].Message, "\n", "", -1)
+	c.Assert(stripped, gc.Matches, expectedMsg)
+}
+
+func (s *ConfigDeprecationSuite) TestDeprecatedToolsURLWarning(c *gc.C) {
+	attrs := testing.Attrs{
+		"tools-url": "aknowndeprecatedfield",
+	}
+	expected := fmt.Sprintf(`.*Config attribute "tools-url" \(aknowndeprecatedfield\) is deprecated\.` +
+		`The location to find tools is now specified using the "tools-metadata-url" attribute.*`)
+	s.checkDeprecationWarning(c, attrs, expected)
+}
+
+func (s *ConfigDeprecationSuite) TestDeprecatedToolsURLWithNewURLWarning(c *gc.C) {
+	attrs := testing.Attrs{
+		"tools-url":          "aknowndeprecatedfield",
+		"tools-metadata-url": "newvalue",
+	}
+	expected := fmt.Sprintf(
+		`.*Config attribute "tools-url" \(aknowndeprecatedfield\) is deprecated and will be ignored since` +
+			`the new tools URL attribute "tools-metadata-url".*`)
+	s.checkDeprecationWarning(c, attrs, expected)
+}

=== modified file 'environs/export_test.go'
--- environs/export_test.go	2013-12-18 08:45:42 +0000
+++ environs/export_test.go	2013-12-19 03:01:43 +0000
@@ -6,3 +6,9 @@
 func Providers() map[string]EnvironProvider {
 	return providers
 }
+
+func UpdateEnvironAttrs(envs *Environs, name string, newAttrs map[string]interface{}) {
+	for k, v := range newAttrs {
+		envs.rawEnvirons[name][k] = v
+	}
+}