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 .*",
  }