juju-core

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

  1. log-deprecated-config-warnings
  2. Merge into trunk
Proposed by Ian Booth
Status: Merged
Approved by: Ian Booth
Approved revision: no longer in the source branch.
Merged at revision: 2161
Proposed branch: lp:~wallyworld/juju-core/log-deprecated-config-warnings
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 259 lines (+108/-82)
5 files modified
environs/config.go (+22/-0)
environs/config/config.go (+19/-23)
environs/config/config_test.go (+0/-59)
environs/config_test.go (+61/-0)
environs/export_test.go (+6/-0)
To merge this branch: bzr merge lp:~wallyworld/juju-core/log-deprecated-config-warnings
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+199597@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

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

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Revision history for this message
Ian Booth (wallyworld) wrote :

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/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Revision history for this message
Ian Booth (wallyworld) wrote :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
68 if err := validateEnvironmentKind(attrs); err != nil {68 if err := validateEnvironmentKind(attrs); err != nil {
69 return nil, err69 return nil, err
70 }70 }
71
72 // If deprecated config attributes are used, log warnings so the user can know
73 // that they need to be fixed.
74 if oldToolsURL := attrs["tools-url"]; oldToolsURL != nil && oldToolsURL.(string) != "" {
75 _, newToolsSpecified := attrs["tools-metadata-url"]
76 var msg string
77 if newToolsSpecified {
78 msg = fmt.Sprintf(
79 "Config attribute %q (%v) is deprecated and will be ignored since\n"+
80 "the new tools URL attribute %q has also been used.\n"+
81 "The attribute %q should be removed from your configuration.",
82 "tools-url", oldToolsURL, "tools-metadata-url", "tools-url")
83 } else {
84 msg = fmt.Sprintf(
85 "Config attribute %q (%v) is deprecated.\n"+
86 "The location to find tools is now specified using the %q attribute.\n"+
87 "Your configuration should be updated to set %q as follows\n%v: %v.",
88 "tools-url", oldToolsURL, "tools-metadata-url", "tools-metadata-url", "tools-metadata-url", oldToolsURL)
89 }
90 logger.Warningf(msg)
91 }
92
71 cfg, err := config.New(config.UseDefaults, attrs)93 cfg, err := config.New(config.UseDefaults, attrs)
72 if err != nil {94 if err != nil {
73 return nil, err95 return nil, err
7496
=== 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 @@
177 }177 }
178}178}
179179
180// processDeprecatedAttributes ensures that the config is set up so that it works
181// correctly when used with older versions of Juju which require that deprecated
182// attribute values still be used.
183func (cfg *Config) processDeprecatedAttributes() {
184 // The tools url has changed so ensure that both old and new values are in the config so that
185 // upgrades work. "tools-url" is the old attribute name.
186 if oldToolsURL := cfg.m["tools-url"]; oldToolsURL != nil && oldToolsURL.(string) != "" {
187 _, newToolsSpecified := cfg.ToolsURL()
188 // Ensure the new attribute name "tools-metadata-url" is set.
189 if !newToolsSpecified {
190 cfg.m["tools-metadata-url"] = oldToolsURL
191 }
192 }
193 // Even if the user has edited their environment yaml to remove the deprecated tools-url value,
194 // we still want it in the config for upgrades.
195 cfg.m["tools-url"], _ = cfg.ToolsURL()
196}
197
180// Validate ensures that config is a valid configuration. If old is not nil,198// Validate ensures that config is a valid configuration. If old is not nil,
181// it holds the previous environment configuration for consideration when199// it holds the previous environment configuration for consideration when
182// validating changes.200// validating changes.
@@ -259,29 +277,7 @@
259 }277 }
260 }278 }
261279
262 // The tools url has changed so see if the old one is still in use.280 cfg.processDeprecatedAttributes()
263 if oldToolsURL := cfg.m["tools-url"]; oldToolsURL != nil && oldToolsURL.(string) != "" {
264 _, newToolsSpecified := cfg.ToolsURL()
265 var msg string
266 if newToolsSpecified {
267 msg = fmt.Sprintf(
268 "Config attribute %q (%v) is deprecated and will be ignored since\n"+
269 "the new tools URL attribute %q has also been used.\n"+
270 "The attribute %q should be removed from your configuration.",
271 "tools-url", oldToolsURL, "tools-metadata-url", "tools-url")
272 } else {
273 msg = fmt.Sprintf(
274 "Config attribute %q (%v) is deprecated.\n"+
275 "The location to find tools is now specified using the %q attribute.\n"+
276 "Your configuration should be updated to set %q as follows\n%v: %v.",
277 "tools-url", oldToolsURL, "tools-metadata-url", "tools-metadata-url", "tools-metadata-url", oldToolsURL)
278 cfg.m["tools-metadata-url"] = oldToolsURL
279 }
280 logger.Warningf(msg)
281 }
282 // Even if the user has edited their environment yaml to remove the deprecated tools-url value,
283 // we still want it in the config for upgrades.
284 cfg.m["tools-url"], _ = cfg.ToolsURL()
285 return nil281 return nil
286}282}
287283
288284
=== 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 @@
6import (6import (
7 "fmt"7 "fmt"
8 "regexp"8 "regexp"
9 "strings"
10 stdtesting "testing"9 stdtesting "testing"
11 "time"10 "time"
1211
@@ -1232,61 +1231,3 @@
1232MIIBOgIBAAJAZabKgKInuOxj5vDWLwHHQtK3/45KB+32D15w94Nt83BmuGxo90lw1231MIIBOgIBAAJAZabKgKInuOxj5vDWLwHHQtK3/45KB+32D15w94Nt83BmuGxo90lw
1233-----END CERTIFICATE-----1232-----END CERTIFICATE-----
1234`[1:]1233`[1:]
1235
1236type ConfigDeprecationSuite struct {
1237 ConfigSuite
1238 writer *testWriter
1239}
1240
1241var _ = gc.Suite(&ConfigDeprecationSuite{})
1242
1243func (s *ConfigDeprecationSuite) SetUpTest(c *gc.C) {
1244 s.ConfigSuite.SetUpTest(c)
1245}
1246
1247func (s *ConfigDeprecationSuite) TearDownTest(c *gc.C) {
1248 s.ConfigSuite.TearDownTest(c)
1249}
1250
1251func (s *ConfigDeprecationSuite) setupLogger(c *gc.C) {
1252 var err error
1253 s.writer = &testWriter{}
1254 err = loggo.RegisterWriter("test", s.writer, loggo.WARNING)
1255 c.Assert(err, gc.IsNil)
1256}
1257
1258func (s *ConfigDeprecationSuite) resetLogger(c *gc.C) {
1259 _, _, err := loggo.RemoveWriter("test")
1260 c.Assert(err, gc.IsNil)
1261}
1262
1263type testWriter struct {
1264 messages []string
1265}
1266
1267func (t *testWriter) Write(level loggo.Level, module, filename string, line int, timestamp time.Time, message string) {
1268 t.messages = append(t.messages, message)
1269}
1270
1271func (s *ConfigDeprecationSuite) setupEnv(c *gc.C, deprecatedKey, value string) {
1272 attrs := testing.FakeConfig().Merge(testing.Attrs{
1273 "name": "testenv",
1274 "type": "openstack",
1275 deprecatedKey: value,
1276 })
1277 _, err := config.New(config.NoDefaults, attrs)
1278 c.Assert(err, gc.IsNil)
1279}
1280
1281func (s *ConfigDeprecationSuite) TestDeprecationWarnings(c *gc.C) {
1282 for attr, value := range map[string]string{
1283 "tools-url": "foo",
1284 } {
1285 s.setupLogger(c)
1286 s.setupEnv(c, attr, value)
1287 s.resetLogger(c)
1288 stripped := strings.Replace(s.writer.messages[0], "\n", "", -1)
1289 expected := fmt.Sprintf(`.*Config attribute "%s" \(%s\) is deprecated.*`, attr, value)
1290 c.Assert(stripped, gc.Matches, expected)
1291 }
1292}
12931234
=== 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 @@
4package environs_test4package environs_test
55
6import (6import (
7 "fmt"
7 "os"8 "os"
8 "path/filepath"9 "path/filepath"
9 "sort"10 "sort"
11 "strings"
1012
11 gc "launchpad.net/gocheck"13 gc "launchpad.net/gocheck"
12 "launchpad.net/loggo"14 "launchpad.net/loggo"
@@ -300,3 +302,62 @@
300 expect["ca-private-key"] = ""302 expect["ca-private-key"] = ""
301 c.Assert(cfg1.AllAttrs(), gc.DeepEquals, expect)303 c.Assert(cfg1.AllAttrs(), gc.DeepEquals, expect)
302}304}
305
306type ConfigDeprecationSuite struct {
307 suite
308 writer *loggo.TestWriter
309}
310
311var _ = gc.Suite(&ConfigDeprecationSuite{})
312
313func (s *ConfigDeprecationSuite) setupLogger(c *gc.C) func() {
314 var err error
315 s.writer = &loggo.TestWriter{}
316 err = loggo.RegisterWriter("test", s.writer, loggo.WARNING)
317 c.Assert(err, gc.IsNil)
318 return func() {
319 _, _, err := loggo.RemoveWriter("test")
320 c.Assert(err, gc.IsNil)
321 }
322}
323
324func (s *ConfigDeprecationSuite) checkDeprecationWarning(c *gc.C, attrs testing.Attrs, expectedMsg string) {
325 defer testing.MakeFakeHomeNoEnvironments(c, "only").Restore()
326 content := `
327environments:
328 deprecated:
329 type: dummy
330 state-server: false
331`
332 restore := s.setupLogger(c)
333 defer restore()
334
335 envs, err := environs.ReadEnvironsBytes([]byte(content))
336 c.Check(err, gc.IsNil)
337 environs.UpdateEnvironAttrs(envs, "deprecated", attrs)
338 _, err = envs.Config("deprecated")
339 c.Check(err, gc.IsNil)
340 c.Assert(s.writer.Log, gc.HasLen, 1)
341 stripped := strings.Replace(s.writer.Log[0].Message, "\n", "", -1)
342 c.Assert(stripped, gc.Matches, expectedMsg)
343}
344
345func (s *ConfigDeprecationSuite) TestDeprecatedToolsURLWarning(c *gc.C) {
346 attrs := testing.Attrs{
347 "tools-url": "aknowndeprecatedfield",
348 }
349 expected := fmt.Sprintf(`.*Config attribute "tools-url" \(aknowndeprecatedfield\) is deprecated\.` +
350 `The location to find tools is now specified using the "tools-metadata-url" attribute.*`)
351 s.checkDeprecationWarning(c, attrs, expected)
352}
353
354func (s *ConfigDeprecationSuite) TestDeprecatedToolsURLWithNewURLWarning(c *gc.C) {
355 attrs := testing.Attrs{
356 "tools-url": "aknowndeprecatedfield",
357 "tools-metadata-url": "newvalue",
358 }
359 expected := fmt.Sprintf(
360 `.*Config attribute "tools-url" \(aknowndeprecatedfield\) is deprecated and will be ignored since` +
361 `the new tools URL attribute "tools-metadata-url".*`)
362 s.checkDeprecationWarning(c, attrs, expected)
363}
303364
=== 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 @@
6func Providers() map[string]EnvironProvider {6func Providers() map[string]EnvironProvider {
7 return providers7 return providers
8}8}
9
10func UpdateEnvironAttrs(envs *Environs, name string, newAttrs map[string]interface{}) {
11 for k, v := range newAttrs {
12 envs.rawEnvirons[name][k] = v
13 }
14}

Subscribers

People subscribed via source and target branches

to status/vote changes: