juju-core

Merge lp:~waigani/juju-core/managers-should-warn-for-any-unknown-options into lp:~go-bot/juju-core/trunk

  1. managers-should-warn-for-any-unknown-options
  2. Merge into trunk
Proposed by Jesse Meek
Status: Merged
Approved by: Jesse Meek
Approved revision: no longer in the source branch.
Merged at revision: 2398
Proposed branch: lp:~waigani/juju-core/managers-should-warn-for-any-unknown-options
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 108 lines (+31/-2)
5 files modified
container/factory/factory_test.go (+1/-2)
container/kvm/kvm.go (+6/-0)
container/kvm/kvm_test.go (+9/-0)
container/lxc/lxc.go (+6/-0)
container/lxc/lxc_test.go (+9/-0)
To merge this branch: bzr merge lp:~waigani/juju-core/managers-should-warn-for-any-unknown-options
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+210114@code.launchpad.net

Commit message

NewContainerManager Unknown Options

Raise warnings for unknown options
passed to NewContainerManager.

Description of the change

NewContainerManager Unknown Options

Raise warnings for unknown options
passed to NewContainerManager.

https://codereview.appspot.com/73360043/

To post a comment you must log in.
Revision history for this message
Jesse Meek (waigani) wrote :
Download full text (3.9 KiB)

Reviewers: mp+210114_code.launchpad.net,

Message:
Please take a look.

Description:
NewContainerManager Unknown Options

Raise warnings for unknown options
passed to NewContainerManager.

https://code.launchpad.net/~waigani/juju-core/managers-should-warn-for-any-unknown-options/+merge/210114

(do not edit description out of merge proposal)

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

Affected files (+32, -0 lines):
   A [revision details]
   M container/kvm/kvm.go
   M container/kvm/kvm_test.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140309103025-wdu183o5b55abqsq
+New revision: <email address hidden>

Index: container/kvm/kvm.go
=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go 2014-03-07 10:53:01 +0000
+++ container/kvm/kvm.go 2014-03-10 02:51:31 +0000
@@ -57,13 +57,19 @@
  // parameter.
  func NewContainerManager(conf container.ManagerConfig) (container.Manager,
error) {
   name := conf[container.ConfigName]
+ delete(conf, container.ConfigName)
   if name == "" {
    return nil, fmt.Errorf("name is required")
   }
   logDir := conf[container.ConfigLogDir]
+ delete(conf, container.ConfigLogDir)
   if logDir == "" {
    logDir = agent.DefaultLogDir
   }
+ for k, v := range conf {
+ logger.Warningf(`Found unused config option with key: "%v" and
value: "%v"`, k, v)
+ }
+
   return &containerManager{name: name, logdir: logDir}, nil
  }

Index: container/kvm/kvm_test.go
=== modified file 'container/kvm/kvm_test.go'
--- container/kvm/kvm_test.go 2014-03-07 01:03:49 +0000
+++ container/kvm/kvm_test.go 2014-03-10 02:51:31 +0000
@@ -40,6 +40,15 @@
   c.Assert(manager, gc.IsNil)
  }

+func (*KVMSuite) TestManagerWarnsAboutUnknownOption(c *gc.C) {
+ _, err := kvm.NewContainerManager(container.ManagerConfig{
+ container.ConfigName: "BillyBatson",
+ "shazam": "Captain Marvel",
+ })
+ c.Assert(err, gc.IsNil)
+ c.Assert(c.GetTestLog(), gc.Matches, `^.*WARNING juju.container.kvm Found
unused config option with key: "shazam" and value: "Captain Marvel"\n*`)
+}
+
  func (s *KVMSuite) TestListInitiallyEmpty(c *gc.C) {
   containers, err := s.manager.ListContainers()
   c.Assert(err, gc.IsNil)

Index: container/lxc/lxc.go
=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go 2014-03-07 10:53:01 +0000
+++ container/lxc/lxc.go 2014-03-10 02:51:31 +0000
@@ -54,13 +54,19 @@
  // parameter.
  func NewContainerManager(conf container.ManagerConfig) (container.Manager,
error) {
   name := conf[container.ConfigName]
+ delete(conf, container.ConfigName)
   if name == "" {
    return nil, fmt.Errorf("name is required")
   }
   logDir := conf[container.ConfigLogDir]
+ delete(conf, container.ConfigLogDir)
   if logDir == "" {
    logDir = agent.DefaultLogDir
   }
+ for k, v := range conf {
+ logger.Warningf(`Found unused config option with key: "%v" and
value: "%v"`, k, v)
+ }
+
   return &containerManager{name: name, logdir: logDir}, nil
  }

In...

Read more...

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

On 2014/03/10 03:00:34, waigani wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/73360043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (10.2 KiB)

The attempt to merge lp:~waigani/juju-core/managers-should-warn-for-any-unknown-options into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.033s
ok launchpad.net/juju-core/agent/mongo 0.528s
ok launchpad.net/juju-core/agent/tools 0.180s
ok launchpad.net/juju-core/bzr 5.378s
ok launchpad.net/juju-core/cert 2.280s
ok launchpad.net/juju-core/charm 0.420s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.847s
ok launchpad.net/juju-core/cmd 0.188s
ok launchpad.net/juju-core/cmd/charm-admin 0.751s
? 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 189.896s
ok launchpad.net/juju-core/cmd/jujud 63.828s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.156s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.176s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.025s
ok launchpad.net/juju-core/container 0.046s

----------------------------------------------------------------------
FAIL: factory_test.go:21: factorySuite.TestNewContainerManager

factory_test.go:42:
    c.Assert(err, gc.IsNil)
... value *errors.errorString = &errors.errorString{s:"name is required"} ("name is required")

OOPS: 0 passed, 1 FAILED
--- FAIL: Test (0.00 seconds)
FAIL
FAIL launchpad.net/juju-core/container/factory 0.042s
ok launchpad.net/juju-core/container/kvm 0.238s
ok launchpad.net/juju-core/container/kvm/mock 0.053s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.250s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.227s
ok launchpad.net/juju-core/environs 2.360s
ok launchpad.net/juju-core/environs/bootstrap 2.953s
ok launchpad.net/juju-core/environs/cloudinit 0.423s
ok launchpad.net/juju-core/environs/config 2.365s
ok launchpad.net/juju-core/environs/configstore 0.031s
ok launchpad.net/juju-core/environs/filestorage 0.030s
ok launchpad.net/juju-core/environs/httpstorage 0.698s
ok launchpad.net/juju-core/environs/imagemetadata 0.488s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.042s
ok launchpad.net/juju-core/environs/jujutest 0.171s
ok launchpad.net/juju-core/environs/manual 12.814s
ok launchpad.net/juju-core/environs/simplestreams 0.230s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.994s
ok launchpad.net/juju-core/environs/storage 0.816s
ok launchpad.net/juju-core/environs/sync 24.466s
ok launchpad.net/juju-core/environs/testing 0...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'container/factory/factory_test.go'
2--- container/factory/factory_test.go 2014-02-28 18:03:44 +0000
3+++ container/factory/factory_test.go 2014-03-10 09:05:38 +0000
4@@ -19,8 +19,6 @@
5 var _ = gc.Suite(&factorySuite{})
6
7 func (*factorySuite) TestNewContainerManager(c *gc.C) {
8- conf := container.ManagerConfig{container.ConfigName: "test"}
9-
10 for _, test := range []struct {
11 containerType instance.ContainerType
12 valid bool
13@@ -37,6 +35,7 @@
14 containerType: instance.ContainerType("other"),
15 valid: false,
16 }} {
17+ conf := container.ManagerConfig{container.ConfigName: "test"}
18 manager, err := factory.NewContainerManager(test.containerType, conf)
19 if test.valid {
20 c.Assert(err, gc.IsNil)
21
22=== modified file 'container/kvm/kvm.go'
23--- container/kvm/kvm.go 2014-03-07 10:53:01 +0000
24+++ container/kvm/kvm.go 2014-03-10 09:05:38 +0000
25@@ -57,13 +57,19 @@
26 // parameter.
27 func NewContainerManager(conf container.ManagerConfig) (container.Manager, error) {
28 name := conf[container.ConfigName]
29+ delete(conf, container.ConfigName)
30 if name == "" {
31 return nil, fmt.Errorf("name is required")
32 }
33 logDir := conf[container.ConfigLogDir]
34+ delete(conf, container.ConfigLogDir)
35 if logDir == "" {
36 logDir = agent.DefaultLogDir
37 }
38+ for k, v := range conf {
39+ logger.Warningf(`Found unused config option with key: "%v" and value: "%v"`, k, v)
40+ }
41+
42 return &containerManager{name: name, logdir: logDir}, nil
43 }
44
45
46=== modified file 'container/kvm/kvm_test.go'
47--- container/kvm/kvm_test.go 2014-03-07 01:03:49 +0000
48+++ container/kvm/kvm_test.go 2014-03-10 09:05:38 +0000
49@@ -40,6 +40,15 @@
50 c.Assert(manager, gc.IsNil)
51 }
52
53+func (*KVMSuite) TestManagerWarnsAboutUnknownOption(c *gc.C) {
54+ _, err := kvm.NewContainerManager(container.ManagerConfig{
55+ container.ConfigName: "BillyBatson",
56+ "shazam": "Captain Marvel",
57+ })
58+ c.Assert(err, gc.IsNil)
59+ c.Assert(c.GetTestLog(), gc.Matches, `^.*WARNING juju.container.kvm Found unused config option with key: "shazam" and value: "Captain Marvel"\n*`)
60+}
61+
62 func (s *KVMSuite) TestListInitiallyEmpty(c *gc.C) {
63 containers, err := s.manager.ListContainers()
64 c.Assert(err, gc.IsNil)
65
66=== modified file 'container/lxc/lxc.go'
67--- container/lxc/lxc.go 2014-03-07 10:53:01 +0000
68+++ container/lxc/lxc.go 2014-03-10 09:05:38 +0000
69@@ -54,13 +54,19 @@
70 // parameter.
71 func NewContainerManager(conf container.ManagerConfig) (container.Manager, error) {
72 name := conf[container.ConfigName]
73+ delete(conf, container.ConfigName)
74 if name == "" {
75 return nil, fmt.Errorf("name is required")
76 }
77 logDir := conf[container.ConfigLogDir]
78+ delete(conf, container.ConfigLogDir)
79 if logDir == "" {
80 logDir = agent.DefaultLogDir
81 }
82+ for k, v := range conf {
83+ logger.Warningf(`Found unused config option with key: "%v" and value: "%v"`, k, v)
84+ }
85+
86 return &containerManager{name: name, logdir: logDir}, nil
87 }
88
89
90=== modified file 'container/lxc/lxc_test.go'
91--- container/lxc/lxc_test.go 2014-03-07 10:53:01 +0000
92+++ container/lxc/lxc_test.go 2014-03-10 09:05:38 +0000
93@@ -49,6 +49,15 @@
94 return manager
95 }
96
97+func (*LxcSuite) TestManagerWarnsAboutUnknownOption(c *gc.C) {
98+ _, err := lxc.NewContainerManager(container.ManagerConfig{
99+ container.ConfigName: "BillyBatson",
100+ "shazam": "Captain Marvel",
101+ })
102+ c.Assert(err, gc.IsNil)
103+ c.Assert(c.GetTestLog(), gc.Matches, `^.*WARNING juju.container.lxc Found unused config option with key: "shazam" and value: "Captain Marvel"\n*`)
104+}
105+
106 func (s *LxcSuite) TestStartContainer(c *gc.C) {
107 manager := s.makeManager(c, "test")
108 instance := containertesting.StartContainer(c, manager, "1/lxc/0")

Subscribers

People subscribed via source and target branches

to status/vote changes: