juju-core

Code review comment for lp:~thumper/juju-core/show-log

  1. show-log
  2. Merge into trunk
Revision history for this message
Tim Penhey (thumper) wrote :

Reviewers: mp+185193_code.launchpad.net,

Message:
Please take a look.

Description:
Make show-log an explicit arg.

This is part of the prelude to making --verbose mean
something else.

Also now, you can specify a log file and ask for show-log
and the logging goes to both places.

There is now a deprecation warning written out if people use
--verbose.

https://code.launchpad.net/~thumper/juju-core/show-log/+merge/185193

(do not edit description out of merge proposal)

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

Affected files (+71, -23 lines):
   A [revision details]
   M cmd/juju/main_test.go
   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: tarmac-20130912013106-u5aifia59afghpiv
+New revision: <email address hidden>

Index: cmd/logging.go
=== modified file 'cmd/logging.go'
--- cmd/logging.go 2013-06-20 11:03:10 +0000
+++ cmd/logging.go 2013-09-12 03:48:50 +0000
@@ -4,7 +4,6 @@
  package cmd

  import (
- "io"
   "os"

   "launchpad.net/gnuflag"
@@ -17,6 +16,7 @@
   Path string
   Verbose bool
   Debug bool
+ ShowLog bool
   Config string
  }

@@ -28,38 +28,49 @@
   f.BoolVar(&l.Verbose, "verbose", false, "if set, log additional messages")
   f.BoolVar(&l.Debug, "debug", false, "if set, log debugging messages")
   f.StringVar(&l.Config, "log-config", "", "specify log levels for modules")
+ f.BoolVar(&l.ShowLog, "show-log", false, "if set, write the log file to
stderr")
  }

  // Start starts logging using the given Context.
-func (l *Log) Start(ctx *Context) (err error) {
- var target io.Writer
+func (l *Log) Start(ctx *Context) error {
   if l.Path != "" {
    path := ctx.AbsPath(l.Path)
- target, err = os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE,
0644)
+ target, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE,
0644)
    if err != nil {
     return err
    }
- } else if l.Verbose || l.Debug {
- target = ctx.Stderr
- }
-
- if target != nil {
    writer := loggo.NewSimpleWriter(target, &loggo.DefaultFormatter{})
- _, err = loggo.ReplaceDefaultWriter(writer)
+ err = loggo.RegisterWriter("logfile", writer, loggo.TRACE)
+ if err != nil {
+ return err
+ }
+ }
+ level := loggo.WARNING
+ if l.Verbose {
+ ctx.Stdout.Write([]byte("verbose is deprecated with the current meaning,
use show-log\n"))
+ l.ShowLog = true
+ }
+ if l.ShowLog {
+ level = loggo.INFO
+ }
+ if l.Debug {
+ l.ShowLog = true
+ level = loggo.DEBUG
+ }
+
+ if l.ShowLog {
+ // We replace the default writer to use ctx.Stderr rather than os.Stderr.
+ writer := loggo.NewSimpleWriter(ctx.Stderr, &loggo.DefaultFormatter{})
+ _, err := loggo.ReplaceDefaultWriter(writer)
    if err != nil {
     return err
    }
   } else {
    loggo.RemoveWriter("default")
   }
- if l.Verbose || l.Debug {
- level := loggo.INFO
- if l.Debug {
- level = loggo.DEBUG
- }
- // Set the level on the root logger.
- loggo.GetLogger("").SetLogLevel(level)
- }
+ // Set the level on the root logger.
+ loggo.GetLogger("").SetLogLevel(level)
+ // Override the logging config with specified logging config.
   loggo.ConfigureLoggers(l.Config)
   return nil
  }

Index: cmd/logging_test.go
=== modified file 'cmd/logging_test.go'
--- cmd/logging_test.go 2013-08-19 11:17:19 +0000
+++ cmd/logging_test.go 2013-09-12 03:54:56 +0000
@@ -20,6 +20,11 @@

  var _ = gc.Suite(&LogSuite{})

+func (s *LogSuite) TearDownTest(c *gc.C) {
+ loggo.ResetLoggers()
+ loggo.ResetWriters()
+}
+
  func (s *LogSuite) TestAddFlags(c *gc.C) {
   l := &cmd.Log{}
   f := testing.NewFlagSet()
@@ -48,6 +53,8 @@
   c.Assert(err, gc.IsNil)

   c.Assert(loggo.GetLogger("").LogLevel(), gc.Equals, loggo.INFO)
+ c.Assert(testing.Stderr(ctx), gc.Equals, "")
+ c.Assert(testing.Stdout(ctx), gc.Equals, "verbose is deprecated with the
current meaning, use show-log\n")
  }

  func (s *LogSuite) TestDebugSetsLogLevel(c *gc.C) {
@@ -57,6 +64,19 @@
   c.Assert(err, gc.IsNil)

   c.Assert(loggo.GetLogger("").LogLevel(), gc.Equals, loggo.DEBUG)
+ c.Assert(testing.Stderr(ctx), gc.Equals, "")
+ c.Assert(testing.Stdout(ctx), gc.Equals, "")
+}
+
+func (s *LogSuite) TestShowLogSetsLogLevel(c *gc.C) {
+ l := &cmd.Log{ShowLog: true}
+ ctx := testing.Context(c)
+ err := l.Start(ctx)
+ c.Assert(err, gc.IsNil)
+
+ c.Assert(loggo.GetLogger("").LogLevel(), gc.Equals, loggo.INFO)
+ c.Assert(testing.Stderr(ctx), gc.Equals, "")
+ c.Assert(testing.Stdout(ctx), gc.Equals, "")
  }

  func (s *LogSuite) TestStderr(c *gc.C) {
@@ -65,7 +85,7 @@
   err := l.Start(ctx)
   c.Assert(err, gc.IsNil)
   log.Infof("hello")
- c.Assert(bufferString(ctx.Stderr), gc.Matches, `^.* INFO .* hello\n`)
+ c.Assert(testing.Stderr(ctx), gc.Matches, `^.* INFO .* hello\n`)
  }

  func (s *LogSuite) TestRelPathLog(c *gc.C) {
@@ -74,10 +94,11 @@
   err := l.Start(ctx)
   c.Assert(err, gc.IsNil)
   log.Infof("hello")
- c.Assert(bufferString(ctx.Stderr), gc.Equals, "")
   content, err := ioutil.ReadFile(filepath.Join(ctx.Dir, "foo.log"))
   c.Assert(err, gc.IsNil)
   c.Assert(string(content), gc.Matches, `^.* INFO .* hello\n`)
+ c.Assert(testing.Stderr(ctx), gc.Equals, "")
+ c.Assert(testing.Stdout(ctx), gc.Equals, "")
  }

  func (s *LogSuite) TestAbsPathLog(c *gc.C) {
@@ -87,8 +108,21 @@
   err := l.Start(ctx)
   c.Assert(err, gc.IsNil)
   log.Infof("hello")
- c.Assert(bufferString(ctx.Stderr), gc.Equals, "")
+ c.Assert(testing.Stderr(ctx), gc.Equals, "")
   content, err := ioutil.ReadFile(path)
   c.Assert(err, gc.IsNil)
   c.Assert(string(content), gc.Matches, `^.* INFO .* hello\n`)
  }
+
+func (s *LogSuite) TestLoggingToFileAndStderr(c *gc.C) {
+ l := &cmd.Log{Path: "foo.log", Config: "<root>=INFO", ShowLog: true}
+ ctx := testing.Context(c)
+ err := l.Start(ctx)
+ c.Assert(err, gc.IsNil)
+ log.Infof("hello")
+ content, err := ioutil.ReadFile(filepath.Join(ctx.Dir, "foo.log"))
+ c.Assert(err, gc.IsNil)
+ c.Assert(string(content), gc.Matches, `^.* INFO .* hello\n`)
+ c.Assert(testing.Stderr(ctx), gc.Matches, `^.* INFO .* hello\n`)
+ c.Assert(testing.Stdout(ctx), gc.Equals, "")
+}

Index: cmd/juju/main_test.go
=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go 2013-08-21 18:38:22 +0000
+++ cmd/juju/main_test.go 2013-09-12 04:11:32 +0000
@@ -192,7 +192,7 @@
   // Check global args work when specified before command
   msg := breakJuju(c, "Bootstrap")
   logpath := filepath.Join(c.MkDir(), "log")
- out := badrun(c, 1, "--log-file",
logpath, "--verbose", "--debug", "bootstrap")
+ out := badrun(c, 1, "--log-file", logpath, "bootstrap")
   c.Assert(out, gc.Equals, "error: "+msg+"\n")
   content, err := ioutil.ReadFile(logpath)
   c.Assert(err, gc.IsNil)
@@ -205,7 +205,7 @@
   // Check global args work when specified after command
   msg := breakJuju(c, "Bootstrap")
   logpath := filepath.Join(c.MkDir(), "log")
- out := badrun(c, 1, "bootstrap", "--log-file",
logpath, "--verbose", "--debug")
+ out := badrun(c, 1, "bootstrap", "--log-file", logpath)
   c.Assert(out, gc.Equals, "error: "+msg+"\n")
   content, err := ioutil.ReadFile(logpath)
   c.Assert(err, gc.IsNil)
@@ -313,6 +313,7 @@
   "-h, --help .*",
   "--log-config .*",
   "--log-file .*",
+ "--show-log .*",
   "-v, --verbose .*",
  }

« Back to merge proposal