juju-core

Merge lp:~wallyworld/juju-core/upgrade-remove-old-config into lp:~go-bot/juju-core/trunk

  1. upgrade-remove-old-config
  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: 2374
Proposed branch: lp:~wallyworld/juju-core/upgrade-remove-old-config
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~axwalk/juju-core/upgrade-syslog-port
Diff against target: 282 lines (+161/-12)
9 files modified
cmd/jujud/upgrade_test.go (+6/-0)
provider/openstack/config.go (+12/-5)
provider/openstack/config_test.go (+22/-6)
upgrades/deprecatedattributes.go (+30/-0)
upgrades/deprecatedattributes_test.go (+83/-0)
upgrades/export_test.go (+1/-0)
upgrades/steps118.go (+5/-0)
upgrades/steps118_test.go (+1/-0)
upgrades/systemsshkey_test.go (+1/-1)
To merge this branch: bzr merge lp:~wallyworld/juju-core/upgrade-remove-old-config
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+208986@code.launchpad.net

Description of the change

Deprecated attributes upgrader

Previous versions of Juju will have left deprecated
attributes hanging around in the environment. This
upgrader will remove such attributes from env state.
eg default-image-id which is ignored will be put
into env state. This upgrader removes it. Also,
the deprecated attributes are no longer propagated
when a new env config is made.

https://codereview.appspot.com/70230049/

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

Reviewers: mp+208986_code.launchpad.net,

Message:
Please take a look.

Description:
Deprecated attributes upgrader

Previous versions of Juju will have left deprecated
attributes hanging around in the environment. This
upgrader will remove such attributes from env state.
eg default-image-id which is ignored will be put
into env state. This upgrader removes it. Also,
the deprecated attributes are no longer propagated
when a new env config is made.

https://code.launchpad.net/~wallyworld/juju-core/upgrade-remove-old-config/+merge/208986

Requires:
https://code.launchpad.net/~axwalk/juju-core/upgrade-syslog-port/+merge/208724

(do not edit description out of merge proposal)

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

Affected files (+165, -16 lines):
   A [revision details]
   M cmd/jujud/machine.go
   M cmd/jujud/unit.go
   M cmd/jujud/upgrade_test.go
   M cmd/logging.go
   M cmd/logging_test.go
   M provider/openstack/config.go
   M provider/openstack/config_test.go
   A upgrades/deprecatedattributes.go
   A upgrades/deprecatedattributes_test.go
   M upgrades/export_test.go
   M upgrades/steps118.go
   M upgrades/steps118_test.go
   M upgrades/systemsshkey_test.go

Revision history for this message
Tim Penhey (thumper) wrote :

LGTM - although not a fan of all the value asserts in the setup.

https://codereview.appspot.com/70230049/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/70230049/diff/1/cmd/jujud/machine.go#newcode121
cmd/jujud/machine.go:121: logger.Infof("machine agent %v start (%s)",
a.Tag(), version.Current)
I'm sure I saw this somewhere else, missing prereq?

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go
File upgrades/deprecatedattributes_test.go (right):

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go#newcode46
upgrades/deprecatedattributes_test.go:46:
c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
Seems a lot to me to have the asserts in setup for this

https://codereview.appspot.com/70230049/

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

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go
File upgrades/deprecatedattributes_test.go (right):

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go#newcode46
upgrades/deprecatedattributes_test.go:46:
c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
On 2014/03/04 03:04:57, thumper wrote:
> Seems a lot to me to have the asserts in setup for this

I can remove them but these serve to double check that the attributes
really are set so that we can be sure the test is testing what we think
it is.

https://codereview.appspot.com/70230049/

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

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go
File upgrades/deprecatedattributes_test.go (right):

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go#newcode46
upgrades/deprecatedattributes_test.go:46:
c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
On 2014/03/04 03:04:57, thumper wrote:
> Seems a lot to me to have the asserts in setup for this

I added an explicit test

https://codereview.appspot.com/70230049/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'cmd/jujud/upgrade_test.go'
--- cmd/jujud/upgrade_test.go 2014-03-04 04:39:34 +0000
+++ cmd/jujud/upgrade_test.go 2014-03-04 04:39:35 +0000
@@ -114,6 +114,12 @@
114 cfg, err := s.State.EnvironConfig()114 cfg, err := s.State.EnvironConfig()
115 c.Assert(err, gc.IsNil)115 c.Assert(err, gc.IsNil)
116 c.Assert(cfg.SyslogPort(), gc.Equals, config.DefaultSyslogPort)116 c.Assert(cfg.SyslogPort(), gc.Equals, config.DefaultSyslogPort)
117 // Deprecated attributes should have been deleted - just test a couple.
118 allAttrs := cfg.AllAttrs()
119 _, ok := allAttrs["public-bucket"]
120 c.Assert(ok, jc.IsFalse)
121 _, ok = allAttrs["public-bucket-region"]
122 c.Assert(ok, jc.IsFalse)
117}123}
118124
119func (s *UpgradeSuite) assertHostUpgrades(c *gc.C) {125func (s *UpgradeSuite) assertHostUpgrades(c *gc.C) {
120126
=== modified file 'provider/openstack/config.go'
--- provider/openstack/config.go 2014-01-24 16:32:15 +0000
+++ provider/openstack/config.go 2014-03-04 04:39:35 +0000
@@ -198,7 +198,8 @@
198198
199 // Check for deprecated fields and log a warning. We also print to stderr to ensure the user sees the message199 // Check for deprecated fields and log a warning. We also print to stderr to ensure the user sees the message
200 // even if they are not running with --debug.200 // even if they are not running with --debug.
201 if defaultImageId := cfg.AllAttrs()["default-image-id"]; defaultImageId != nil && defaultImageId.(string) != "" {201 cfgAttrs := cfg.AllAttrs()
202 if defaultImageId := cfgAttrs["default-image-id"]; defaultImageId != nil && defaultImageId.(string) != "" {
202 msg := fmt.Sprintf(203 msg := fmt.Sprintf(
203 "Config attribute %q (%v) is deprecated and ignored.\n"+204 "Config attribute %q (%v) is deprecated and ignored.\n"+
204 "Your cloud provider should have set up image metadata to provide the correct image id\n"+205 "Your cloud provider should have set up image metadata to provide the correct image id\n"+
@@ -208,7 +209,7 @@
208 "default-image-id", defaultImageId)209 "default-image-id", defaultImageId)
209 logger.Warningf(msg)210 logger.Warningf(msg)
210 }211 }
211 if defaultInstanceType := cfg.AllAttrs()["default-instance-type"]; defaultInstanceType != nil && defaultInstanceType.(string) != "" {212 if defaultInstanceType := cfgAttrs["default-instance-type"]; defaultInstanceType != nil && defaultInstanceType.(string) != "" {
212 msg := fmt.Sprintf(213 msg := fmt.Sprintf(
213 "Config attribute %q (%v) is deprecated and ignored.\n"+214 "Config attribute %q (%v) is deprecated and ignored.\n"+
214 "The correct instance flavor is determined using constraints, globally specified\n"+215 "The correct instance flavor is determined using constraints, globally specified\n"+
@@ -217,7 +218,13 @@
217 "default-instance-type", defaultInstanceType)218 "default-instance-type", defaultInstanceType)
218 logger.Warningf(msg)219 logger.Warningf(msg)
219 }220 }
220221 // Construct a new config with the deprecated attributes removed.
221 // Apply the coerced unknown values back into the config.222 for _, attr := range []string{"default-image-id", "default-instance-type"} {
222 return cfg.Apply(ecfg.attrs)223 delete(cfgAttrs, attr)
224 delete(ecfg.attrs, attr)
225 }
226 for k, v := range ecfg.attrs {
227 cfgAttrs[k] = v
228 }
229 return config.New(config.NoDefaults, cfgAttrs)
223}230}
224231
=== modified file 'provider/openstack/config_test.go'
--- provider/openstack/config_test.go 2014-02-13 02:46:58 +0000
+++ provider/openstack/config_test.go 2014-03-04 04:39:35 +0000
@@ -55,7 +55,6 @@
55 envVars map[string]string55 envVars map[string]string
56 region string56 region string
57 controlBucket string57 controlBucket string
58 toolsURL string
59 useFloatingIP bool58 useFloatingIP bool
60 useDefaultSecurityGroup bool59 useDefaultSecurityGroup bool
61 network string60 network string
@@ -152,11 +151,6 @@
152 c.Assert(err, gc.IsNil)151 c.Assert(err, gc.IsNil)
153 c.Assert(expected, gc.DeepEquals, actual)152 c.Assert(expected, gc.DeepEquals, actual)
154 }153 }
155 if t.toolsURL != "" {
156 toolsURL, ok := ecfg.ToolsURL()
157 c.Assert(ok, jc.IsTrue)
158 c.Assert(toolsURL, gc.Equals, t.toolsURL)
159 }
160 if t.firewallMode != "" {154 if t.firewallMode != "" {
161 c.Assert(ecfg.FirewallMode(), gc.Equals, t.firewallMode)155 c.Assert(ecfg.FirewallMode(), gc.Equals, t.firewallMode)
162 }156 }
@@ -452,6 +446,28 @@
452 }446 }
453}447}
454448
449func (s *ConfigSuite) TestDeprecatedAttributesRemoved(c *gc.C) {
450 s.setupEnvCredentials()
451 attrs := testing.FakeConfig().Merge(testing.Attrs{
452 "type": "openstack",
453 "control-bucket": "x",
454 "default-image-id": "id-1234",
455 "default-instance-type": "big",
456 })
457
458 cfg, err := config.New(config.NoDefaults, attrs)
459 c.Assert(err, gc.IsNil)
460 // Keep err for validation below.
461 valid, err := providerInstance.Validate(cfg, nil)
462 c.Assert(err, gc.IsNil)
463 // Check deprecated attributes removed.
464 allAttrs := valid.AllAttrs()
465 for _, attr := range []string{"default-image-id", "default-instance-type"} {
466 _, ok := allAttrs[attr]
467 c.Assert(ok, jc.IsFalse)
468 }
469}
470
455func (s *ConfigSuite) TestPrepareInsertsUniqueControlBucket(c *gc.C) {471func (s *ConfigSuite) TestPrepareInsertsUniqueControlBucket(c *gc.C) {
456 s.setupEnvCredentials()472 s.setupEnvCredentials()
457 attrs := testing.FakeConfig().Merge(testing.Attrs{473 attrs := testing.FakeConfig().Merge(testing.Attrs{
458474
=== added file 'upgrades/deprecatedattributes.go'
--- upgrades/deprecatedattributes.go 1970-01-01 00:00:00 +0000
+++ upgrades/deprecatedattributes.go 2014-03-04 04:39:35 +0000
@@ -0,0 +1,30 @@
1// Copyright 2014 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package upgrades
5
6import (
7 "fmt"
8
9 "launchpad.net/juju-core/environs/config"
10)
11
12func processDeprecatedAttributes(context Context) error {
13 st := context.State()
14 cfg, err := st.EnvironConfig()
15 if err != nil {
16 return fmt.Errorf("failed to read current config: %v", err)
17 }
18 newAttrs := cfg.AllAttrs()
19 delete(newAttrs, "public-bucket")
20 delete(newAttrs, "public-bucket-region")
21 delete(newAttrs, "public-bucket-url")
22 delete(newAttrs, "default-image-id")
23 delete(newAttrs, "default-instance-type")
24 // TODO (wallyworld) - delete tools-url in 1.20
25 newCfg, err := config.New(config.NoDefaults, newAttrs)
26 if err != nil {
27 return fmt.Errorf("failed to create new config: %v", err)
28 }
29 return st.SetEnvironConfig(newCfg, cfg)
30}
031
=== added file 'upgrades/deprecatedattributes_test.go'
--- upgrades/deprecatedattributes_test.go 1970-01-01 00:00:00 +0000
+++ upgrades/deprecatedattributes_test.go 2014-03-04 04:39:35 +0000
@@ -0,0 +1,83 @@
1// Copyright 2014 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package upgrades_test
5
6import (
7 gc "launchpad.net/gocheck"
8
9 jujutesting "launchpad.net/juju-core/juju/testing"
10 "launchpad.net/juju-core/state"
11 jc "launchpad.net/juju-core/testing/checkers"
12 "launchpad.net/juju-core/upgrades"
13)
14
15type processDeprecatedAttributesSuite struct {
16 jujutesting.JujuConnSuite
17 ctx upgrades.Context
18}
19
20var _ = gc.Suite(&processDeprecatedAttributesSuite{})
21
22func (s *processDeprecatedAttributesSuite) SetUpTest(c *gc.C) {
23 s.JujuConnSuite.SetUpTest(c)
24 apiState, _ := s.OpenAPIAsNewMachine(c, state.JobManageEnviron)
25 s.ctx = &mockContext{
26 agentConfig: &mockAgentConfig{dataDir: s.DataDir()},
27 apiState: apiState,
28 state: s.State,
29 }
30 cfg, err := s.State.EnvironConfig()
31 c.Assert(err, gc.IsNil)
32 // Add in old public bucket config.
33 newCfg, err := cfg.Apply(map[string]interface{}{
34 "public-bucket": "foo",
35 "public-bucket-region": "bar",
36 "public-bucket-url": "shazbot",
37 "default-instance-type": "vulch",
38 "default-image-id": "1234",
39 })
40 c.Assert(err, gc.IsNil)
41 err = s.State.SetEnvironConfig(newCfg, cfg)
42 c.Assert(err, gc.IsNil)
43}
44
45func (s *processDeprecatedAttributesSuite) TestAttributesSet(c *gc.C) {
46 cfg, err := s.State.EnvironConfig()
47 c.Assert(err, gc.IsNil)
48 allAttrs := cfg.AllAttrs()
49 c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
50 c.Assert(allAttrs["public-bucket-region"], gc.Equals, "bar")
51 c.Assert(allAttrs["public-bucket-url"], gc.Equals, "shazbot")
52 c.Assert(allAttrs["default-instance-type"], gc.Equals, "vulch")
53 c.Assert(allAttrs["default-image-id"], gc.Equals, "1234")
54}
55
56func (s *processDeprecatedAttributesSuite) assertConfigProcessed(c *gc.C) {
57 cfg, err := s.State.EnvironConfig()
58 c.Assert(err, gc.IsNil)
59 allAttrs := cfg.AllAttrs()
60 for _, deprecated := range []string{
61 "public-bucket", "public-bucket-region", "public-bucket-url",
62 "default-image-id", "default-instance-type",
63 } {
64 _, ok := allAttrs[deprecated]
65 c.Assert(ok, jc.IsFalse)
66 }
67}
68
69func (s *processDeprecatedAttributesSuite) TestOldConfigRemoved(c *gc.C) {
70 err := upgrades.ProcessDeprecatedAttributes(s.ctx)
71 c.Assert(err, gc.IsNil)
72 s.assertConfigProcessed(c)
73}
74
75func (s *processDeprecatedAttributesSuite) TestIdempotent(c *gc.C) {
76 err := upgrades.ProcessDeprecatedAttributes(s.ctx)
77 c.Assert(err, gc.IsNil)
78 s.assertConfigProcessed(c)
79
80 err = upgrades.ProcessDeprecatedAttributes(s.ctx)
81 c.Assert(err, gc.IsNil)
82 s.assertConfigProcessed(c)
83}
084
=== modified file 'upgrades/export_test.go'
--- upgrades/export_test.go 2014-03-04 04:39:34 +0000
+++ upgrades/export_test.go 2014-03-04 04:39:35 +0000
@@ -12,4 +12,5 @@
12 EnsureLockDirExistsAndUbuntuWritable = ensureLockDirExistsAndUbuntuWritable12 EnsureLockDirExistsAndUbuntuWritable = ensureLockDirExistsAndUbuntuWritable
13 EnsureSystemSSHKey = ensureSystemSSHKey13 EnsureSystemSSHKey = ensureSystemSSHKey
14 UpdateRsyslogPort = updateRsyslogPort14 UpdateRsyslogPort = updateRsyslogPort
15 ProcessDeprecatedAttributes = processDeprecatedAttributes
15)16)
1617
=== modified file 'upgrades/steps118.go'
--- upgrades/steps118.go 2014-03-04 04:39:34 +0000
+++ upgrades/steps118.go 2014-03-04 04:39:35 +0000
@@ -26,5 +26,10 @@
26 targets: []Target{AllMachines},26 targets: []Target{AllMachines},
27 run: installRsyslogGnutls,27 run: installRsyslogGnutls,
28 },28 },
29 &upgradeStep{
30 description: "remove deprecated attribute values",
31 targets: []Target{StateServer},
32 run: processDeprecatedAttributes,
33 },
29 }34 }
30}35}
3136
=== modified file 'upgrades/steps118_test.go'
--- upgrades/steps118_test.go 2014-03-04 04:39:34 +0000
+++ upgrades/steps118_test.go 2014-03-04 04:39:35 +0000
@@ -21,6 +21,7 @@
21 "generate system ssh key",21 "generate system ssh key",
22 "update rsyslog port",22 "update rsyslog port",
23 "install rsyslog-gnutls",23 "install rsyslog-gnutls",
24 "remove deprecated attribute values",
24}25}
2526
26func (s *steps118Suite) TestUpgradeOperationsContent(c *gc.C) {27func (s *steps118Suite) TestUpgradeOperationsContent(c *gc.C) {
2728
=== modified file 'upgrades/systemsshkey_test.go'
--- upgrades/systemsshkey_test.go 2014-02-27 03:17:45 +0000
+++ upgrades/systemsshkey_test.go 2014-03-04 04:39:35 +0000
@@ -34,7 +34,7 @@
34 _, err := os.Stat(s.keyFile())34 _, err := os.Stat(s.keyFile())
35 c.Assert(err, jc.Satisfies, os.IsNotExist)35 c.Assert(err, jc.Satisfies, os.IsNotExist)
36 // There's initially one authorised key for the test user.36 // There's initially one authorised key for the test user.
37 cfg, err := s.JujuConnSuite.State.EnvironConfig()37 cfg, err := s.State.EnvironConfig()
38 c.Assert(err, gc.IsNil)38 c.Assert(err, gc.IsNil)
39 authKeys := ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())39 authKeys := ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
40 c.Assert(authKeys, gc.HasLen, 1)40 c.Assert(authKeys, gc.HasLen, 1)

Subscribers

People subscribed via source and target branches

to status/vote changes: