Merge lp:~thumper/juju-core/verbose-means-info into lp:~juju/juju-core/trunk

[Tim Penhey]

Have the verbose flag set the log level to INFO.

When the loggo branch landed yesterday, there was a change in behaviour with
the --verbose flag, where it did log to stderr, but it was at the WARNING
level, not INFO.

https://codereview.appspot.com/9698047/

[Tim Penhey (thumper)]

Reviewers: mp+167687_code.launchpad.net,

Message:
Please take a look.

Description:
Have the verbose flag set the log level to INFO.

When the loggo branch landed yesterday, there was a change in behaviour
with
the --verbose flag, where it did log to stderr, but it was at the
WARNING
level, not INFO.

https://code.launchpad.net/~thumper/juju-core/verbose-means-info/+merge/167687

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/logging.go
   M cmd/logging_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: <email address hidden>
+New revision: <email address hidden>

Index: cmd/logging.go
=== modified file 'cmd/logging.go'
--- cmd/logging.go 2013-06-05 02:44:14 +0000
+++ cmd/logging.go 2013-06-06 03:55:42 +0000
@@ -52,9 +52,13 @@
   } else {
    loggo.RemoveWriter("default")
   }
- if l.Debug {
+ if l.Verbose || l.Debug {
+ var level = loggo.INFO
+ if l.Debug {
+ level = loggo.DEBUG
+ }
    logger := loggo.GetLogger("juju")
- logger.SetLogLevel(loggo.DEBUG)
+ logger.SetLogLevel(level)
   }
   loggo.ConfigureLogging(l.Config)
   return nil

Index: cmd/logging_test.go
=== modified file 'cmd/logging_test.go'
--- cmd/logging_test.go 2013-05-30 03:41:19 +0000
+++ cmd/logging_test.go 2013-06-06 03:55:22 +0000
@@ -5,11 +5,13 @@

  import (
   "io/ioutil"
+ "path/filepath"
+
   . "launchpad.net/gocheck"
   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/log"
   "launchpad.net/juju-core/testing"
- "path/filepath"
+ "launchpad.net/loggo"
  )

  type LogSuite struct {
@@ -38,6 +40,26 @@
   c.Assert(l.Config, Equals, "juju.cmd=INFO;juju.worker.deployer=DEBUG")
  }

+func (s *LogSuite) TestVerboseSetsLogLevel(c *C) {
+ l := &cmd.Log{Verbose: true}
+ ctx := testing.Context(c)
+ err := l.Start(ctx)
+ c.Assert(err, IsNil)
+
+ logger := loggo.GetLogger("juju")
+ c.Assert(logger.GetLogLevel(), Equals, loggo.INFO)
+}
+
+func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
+ l := &cmd.Log{Debug: true}
+ ctx := testing.Context(c)
+ err := l.Start(ctx)
+ c.Assert(err, IsNil)
+
+ logger := loggo.GetLogger("juju")
+ c.Assert(logger.GetLogLevel(), Equals, loggo.DEBUG)
+}
+
  func (s *LogSuite) TestStderr(c *C) {
   l := &cmd.Log{Verbose: true, Config: "<root>=INFO"}
   ctx := testing.Context(c)

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/9698047/

[Dimiter Naydenov (dimitern)]

LGTM with one suggestion.

https://codereview.appspot.com/9698047/diff/1/cmd/logging.go
File cmd/logging.go (right):

https://codereview.appspot.com/9698047/diff/1/cmd/logging.go#newcode56
cmd/logging.go:56: var level = loggo.INFO
level := loggo.INFO

https://codereview.appspot.com/9698047/

[Tim Penhey (thumper)]

*** Submitted:

Have the verbose flag set the log level to INFO.

When the loggo branch landed yesterday, there was a change in behaviour
with
the --verbose flag, where it did log to stderr, but it was at the
WARNING
level, not INFO.

R=fwereade, dimitern
CC=
https://codereview.appspot.com/9698047

https://codereview.appspot.com/9698047/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/9698047/diff/5001/cmd/logging.go
File cmd/logging.go (right):

https://codereview.appspot.com/9698047/diff/5001/cmd/logging.go#newcode60
cmd/logging.go:60: logger := loggo.GetLogger("juju")
i'm not entirely sure it's right to restrict this to juju log messages
only.
surely if i run juju bootstrap --debug, i'll want to see debug messages
from anything that's currently producing log messages (for example goose
or maas)?

i'd change "juju" to "" here.

https://codereview.appspot.com/9698047/

=== modified file 'cmd/logging.go'
--- cmd/logging.go	2013-06-05 02:44:14 +0000
+++ cmd/logging.go	2013-06-06 03:59:28 +0000
@@ -52,9 +52,13 @@
 	} else {
 		loggo.RemoveWriter("default")
 	}
-	if l.Debug {
+	if l.Verbose || l.Debug {
+		var level = loggo.INFO
+		if l.Debug {
+			level = loggo.DEBUG
+		}
 		logger := loggo.GetLogger("juju")
-		logger.SetLogLevel(loggo.DEBUG)
+		logger.SetLogLevel(level)
 	}
 	loggo.ConfigureLogging(l.Config)
 	return nil

=== modified file 'cmd/logging_test.go'
--- cmd/logging_test.go	2013-05-30 03:41:19 +0000
+++ cmd/logging_test.go	2013-06-06 03:59:28 +0000
@@ -5,11 +5,13 @@
 
 import (
 	"io/ioutil"
+	"path/filepath"
+
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/testing"
-	"path/filepath"
+	"launchpad.net/loggo"
 )
 
 type LogSuite struct {
@@ -38,6 +40,26 @@
 	c.Assert(l.Config, Equals, "juju.cmd=INFO;juju.worker.deployer=DEBUG")
 }
 
+func (s *LogSuite) TestVerboseSetsLogLevel(c *C) {
+	l := &cmd.Log{Verbose: true}
+	ctx := testing.Context(c)
+	err := l.Start(ctx)
+	c.Assert(err, IsNil)
+
+	logger := loggo.GetLogger("juju")
+	c.Assert(logger.GetLogLevel(), Equals, loggo.INFO)
+}
+
+func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
+	l := &cmd.Log{Debug: true}
+	ctx := testing.Context(c)
+	err := l.Start(ctx)
+	c.Assert(err, IsNil)
+
+	logger := loggo.GetLogger("juju")
+	c.Assert(logger.GetLogLevel(), Equals, loggo.DEBUG)
+}
+
 func (s *LogSuite) TestStderr(c *C) {
 	l := &cmd.Log{Verbose: true, Config: "<root>=INFO"}
 	ctx := testing.Context(c)