LGTM

could I suggest also adding the various log functions to
.lbox.check ? perhaps as a separate CL

(I believe the version check should change there - currently
it checks that the Go version is devel, but 1.1 is also
sufficient now).


On 10 June 2013 05:31, Tim Penhey <email address hidden> wrote:
> Reviewers: mp+168342_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> Updates for latest loggo.
>
> Since we don't yet have dependency management, the latest
> changes to loggo are unpushed until I get this agreed upon.
>
> https://code.launchpad.net/~thumper/juju-core/update-loggo/+merge/168342
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/10125046/
>
> Affected files:
>    A [revision details]
>    M cmd/builddb/main.go
>    M cmd/juju/main_test.go
>    M cmd/logging.go
>    M cmd/logging_test.go
>    M log/log.go
>    M testing/log.go
>    M worker/provisioner/provisioner.go
>    M worker/provisioner/provisioner_task.go
>    M worker/uniter/jujuc/juju-log_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-07 01:00:23 +0000
> +++ cmd/logging.go      2013-06-10 04:25:51 +0000
> @@ -57,9 +57,9 @@
>                 if l.Debug {
>                         level = loggo.DEBUG
>                 }
> -               logger := loggo.GetLogger("juju")
> -               logger.SetLogLevel(level)
> +               // Set the level on the root logger.
> +               loggo.GetLogger("").SetLogLevel(level)
>         }
> -       loggo.ConfigureLogging(l.Config)
> +       loggo.ConfigureLoggers(l.Config)
>         return nil
>   }
>
>
> Index: cmd/logging_test.go
> === modified file 'cmd/logging_test.go'
> --- cmd/logging_test.go 2013-06-06 03:55:22 +0000
> +++ cmd/logging_test.go 2013-06-10 04:25:51 +0000
> @@ -46,8 +46,7 @@
>         err := l.Start(ctx)
>         c.Assert(err, IsNil)
>
> -       logger := loggo.GetLogger("juju")
> -       c.Assert(logger.GetLogLevel(), Equals, loggo.INFO)
> +       c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.INFO)
>   }
>
>   func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
> @@ -56,8 +55,7 @@
>         err := l.Start(ctx)
>         c.Assert(err, IsNil)
>
> -       logger := loggo.GetLogger("juju")
> -       c.Assert(logger.GetLogLevel(), Equals, loggo.DEBUG)
> +       c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.DEBUG)
>   }
>
>   func (s *LogSuite) TestStderr(c *C) {
>
>
> Index: cmd/builddb/main.go
> === modified file 'cmd/builddb/main.go'
> --- cmd/builddb/main.go 2013-05-29 05:29:57 +0000
> +++ cmd/builddb/main.go 2013-06-10 04:25:51 +0000
> @@ -57,13 +57,13 @@
>                 return err
>         }
>
> -       logger.Info("Waiting for unit to reach %q status...",
> params.StatusStarted)
> +       logger.Infof("Waiting for unit to reach %q status...",
> params.StatusStarted)
>         unit := units[0]
>         last, info, err := unit.Status()
>         if err != nil {
>                 return err
>         }
> -       logger.Info("Unit status is %q: %s", last, info)
> +       logger.Infof("Unit status is %q: %s", last, info)
>         for last != params.StatusStarted {
>                 time.Sleep(2 * time.Second)
>                 if err := unit.Refresh(); err != nil {
> @@ -74,7 +74,7 @@
>                         return err
>                 }
>                 if status != last {
> -                       logger.Info("Unit status is %q: %s", status, info)
> +                       logger.Infof("Unit status is %q: %s", status, info)
>                         last = status
>                 }
>         }
> @@ -82,7 +82,7 @@
>         if !ok {
>                 return fmt.Errorf("cannot retrieve files: build unit lacks a
> public-address")
>         }
> -       logger.Info("Built files published at http://%s", addr)
> -       logger.Info("Remember to destroy the environment when you're done...")
> +       logger.Infof("Built files published at http://%s", addr)
> +       logger.Infof("Remember to destroy the environment when you're done...")
>         return nil
>   }
>
>
> Index: cmd/juju/main_test.go
> === modified file 'cmd/juju/main_test.go'
> --- cmd/juju/main_test.go       2013-06-04 21:43:25 +0000
> +++ cmd/juju/main_test.go       2013-06-10 04:25:51 +0000
> @@ -73,7 +73,7 @@
>   }
>
>   func (s *MainSuite) TestTearDown(c *C) {
> -       loggo.ResetLogging()
> +       loggo.ResetLoggers()
>   }
>
>   func (s *MainSuite) TestRunMain(c *C) {
>
>
> Index: log/log.go
> === modified file 'log/log.go'
> --- log/log.go  2013-05-30 06:54:40 +0000
> +++ log/log.go  2013-06-10 04:25:51 +0000
> @@ -13,31 +13,31 @@
>
>   // Errorf logs a message using the ERROR priority.
>   func Errorf(format string, a ...interface{}) error {
> -       logger.Log(loggo.ERROR, format, a...)
> +       logger.Logf(loggo.ERROR, format, a...)
>         return nil
>   }
>
>   // Warningf logs a message using the WARNING priority.
>   func Warningf(format string, a ...interface{}) error {
> -       logger.Log(loggo.WARNING, format, a...)
> +       logger.Logf(loggo.WARNING, format, a...)
>         return nil
>   }
>
>   // Noticef logs a message using the NOTICE priority.
>   // Notice doesn't really convert to the loggo priorities...
>   func Noticef(format string, a ...interface{}) error {
> -       logger.Log(loggo.INFO, format, a...)
> +       logger.Logf(loggo.INFO, format, a...)
>         return nil
>   }
>
>   // Infof logs a message using the INFO priority.
>   func Infof(format string, a ...interface{}) error {
> -       logger.Log(loggo.INFO, format, a...)
> +       logger.Logf(loggo.INFO, format, a...)
>         return nil
>   }
>
>   // Debugf logs a message using the DEBUG priority.
>   func Debugf(format string, a ...interface{}) (err error) {
> -       logger.Log(loggo.DEBUG, format, a...)
> +       logger.Logf(loggo.DEBUG, format, a...)
>         return nil
>   }
>
>
> Index: testing/log.go
> === modified file 'testing/log.go'
> --- testing/log.go      2013-05-31 01:40:31 +0000
> +++ testing/log.go      2013-06-10 04:25:51 +0000
> @@ -32,11 +32,11 @@
>   func (t *LoggingSuite) SetUpTest(c *C) {
>         loggo.ResetWriters()
>         loggo.ReplaceDefaultWriter(&gocheckWriter{c})
> -       loggo.ResetLogging()
> +       loggo.ResetLoggers()
>         loggo.GetLogger("juju").SetLogLevel(loggo.DEBUG)
>   }
>
>   func (t *LoggingSuite) TearDownTest(c *C) {
> -       loggo.ResetLogging()
> +       loggo.ResetLoggers()
>         loggo.ResetWriters()
>   }
>
>
> Index: worker/provisioner/provisioner.go
> === modified file 'worker/provisioner/provisioner.go'
> --- worker/provisioner/provisioner.go   2013-06-10 02:52:05 +0000
> +++ worker/provisioner/provisioner.go   2013-06-10 04:25:51 +0000
> @@ -95,14 +95,14 @@
>                         return tomb.ErrDying
>                 case <-environmentProvisioner.Dying():
>                         err := environmentProvisioner.Err()
> -                       logger.Error("environment provisioner died: %v", err)
> +                       logger.Errorf("environment provisioner died: %v", err)
>                         return err
>                 case cfg, ok := <-environWatcher.Changes():
>                         if !ok {
>                                 return watcher.MustErr(environWatcher)
>                         }
>                         if err := p.setConfig(cfg); err != nil {
> -                               logger.Error("loaded invalid environment configuration: %v", err)
> +                               logger.Errorf("loaded invalid environment configuration: %v", err)
>                         }
>                 }
>         }
>
>
> Index: worker/provisioner/provisioner_task.go
> === modified file 'worker/provisioner/provisioner_task.go'
> --- worker/provisioner/provisioner_task.go      2013-06-10 02:51:28 +0000
> +++ worker/provisioner/provisioner_task.go      2013-06-10 04:25:51 +0000
> @@ -97,7 +97,7 @@
>   }
>
>   func (task *provisionerTask) loop() error {
> -       logger.Info("Starting up provisioner task %s", task.machineId)
> +       logger.Infof("Starting up provisioner task %s", task.machineId)
>         defer watcher.Stop(task.machineWatcher, &task.tomb)
>
>         // When the watcher is started, it will have the initial changes be all
> @@ -106,7 +106,7 @@
>         for {
>                 select {
>                 case <-task.tomb.Dying():
> -                       logger.Info("Shutting down provisioner task %s", task.machineId)
> +                       logger.Infof("Shutting down provisioner task %s", task.machineId)
>                         return tomb.ErrDying
>                 case ids, ok := <-task.machineWatcher.Changes():
>                         if !ok {
> @@ -115,7 +115,7 @@
>                         // TODO(dfc; lp:1042717) fire process machines periodically to shut
> down unknown
>                         // instances.
>                         if err := task.processMachines(ids); err != nil {
> -                               logger.Error("Process machines failed: %v", err)
> +                               logger.Errorf("Process machines failed: %v", err)
>                                 return err
>                         }
>                 }
> @@ -124,7 +124,7 @@
>   }
>
>   func (task *provisionerTask) processMachines(ids []string) error {
> -       logger.Trace("processMachines(%v)", ids)
> +       logger.Tracef("processMachines(%v)", ids)
>         // Populate the tasks maps of current instances and machines.
>         err := task.populateMachineMaps(ids)
>         if err != nil {
> @@ -163,7 +163,7 @@
>
>         instances, err := task.broker.AllInstances()
>         if err != nil {
> -               logger.Error("failed to get all instances from broker: %v", err)
> +               logger.Errorf("failed to get all instances from broker: %v", err)
>                 return err
>         }
>         for _, i := range instances {
> @@ -177,12 +177,12 @@
>                 machine, err := task.machineGetter.Machine(id)
>                 switch {
>                 case errors.IsNotFoundError(err):
> -                       logger.Debug("machine %q not found in state", id)
> +                       logger.Debugf("machine %q not found in state", id)
>                         delete(task.machines, id)
>                 case err == nil:
>                         task.machines[id] = machine
>                 default:
> -                       logger.Error("failed to get machine: %v", err)
> +                       logger.Errorf("failed to get machine: %v", err)
>                 }
>         }
>         return nil
> @@ -194,7 +194,7 @@
>         for _, id := range ids {
>                 machine, found := task.machines[id]
>                 if !found {
> -                       logger.Info("machine %q not found", id)
> +                       logger.Infof("machine %q not found", id)
>                         continue
>                 }
>                 switch machine.Life() {
> @@ -202,17 +202,17 @@
>                         if _, ok := machine.InstanceId(); ok {
>                                 continue
>                         }
> -                       logger.Info("killing dying, unprovisioned machine %q", machine)
> +                       logger.Infof("killing dying, unprovisioned machine %q", machine)
>                         if err := machine.EnsureDead(); err != nil {
> -                               logger.Error("failed to ensure machine dead %q: %v", machine, err)
> +                               logger.Errorf("failed to ensure machine dead %q: %v", machine, err)
>                                 return nil, nil, err
>                         }
>                         fallthrough
>                 case state.Dead:
>                         dead = append(dead, machine)
> -                       logger.Info("removing dead machine %q", machine)
> +                       logger.Infof("removing dead machine %q", machine)
>                         if err := machine.Remove(); err != nil {
> -                               logger.Error("failed to remove dead machine %q", machine)
> +                               logger.Errorf("failed to remove dead machine %q", machine)
>                                 return nil, nil, err
>                         }
>                         // now remove it from the machines map
> @@ -222,16 +222,16 @@
>                 if instId, hasInstId := machine.InstanceId(); !hasInstId {
>                         status, _, err := machine.Status()
>                         if err != nil {
> -                               logger.Info("cannot get machine %q status: %v", machine, err)
> +                               logger.Infof("cannot get machine %q status: %v", machine, err)
>                                 continue
>                         }
>                         if status == params.StatusPending {
>                                 pending = append(pending, machine)
> -                               logger.Info("found machine %q pending provisioning", machine)
> +                               logger.Infof("found machine %q pending provisioning", machine)
>                                 continue
>                         }
>                 } else {
> -                       logger.Info("machine %v already started as instance %q", machine,
> instId)
> +                       logger.Infof("machine %v already started as instance %q", machine,
> instId)
>                 }
>         }
>         return
> @@ -254,7 +254,7 @@
>         for _, i := range instances {
>                 unknown = append(unknown, i)
>         }
> -       logger.Trace("unknown: %v", unknown)
> +       logger.Tracef("unknown: %v", unknown)
>         return unknown, nil
>   }
>
> @@ -282,9 +282,9 @@
>         if len(instances) == 0 {
>                 return nil
>         }
> -       logger.Debug("Stopping instances: %v", instances)
> +       logger.Debugf("Stopping instances: %v", instances)
>         if err := task.broker.StopInstances(instances); err != nil {
> -               logger.Error("broker failed to stop instances: %v", err)
> +               logger.Errorf("broker failed to stop instances: %v", err)
>                 return err
>         }
>         return nil
> @@ -306,7 +306,7 @@
>         // state.Info as the PA.
>         stateInfo, apiInfo, err := task.setupAuthentication(machine)
>         if err != nil {
> -               logger.Error("failed to setup authentication: %v", err)
> +               logger.Errorf("failed to setup authentication: %v", err)
>                 return err
>         }
>         cons, err := machine.Constraints()
> @@ -327,10 +327,10 @@
>                 // Set the state to error, so the machine will be skipped next
>                 // time until the error is resolved, but don't return an
>                 // error; just keep going with the other machines.
> -               logger.Error("cannot start instance for machine %q: %v", machine, err)
> +               logger.Errorf("cannot start instance for machine %q: %v", machine, err)
>                 if err1 := machine.SetStatus(params.StatusError, err.Error()); err1 !=
> nil {
>                         // Something is wrong with this machine, better report it back.
> -                       logger.Error("cannot set error status for machine %q: %v", machine,
> err1)
> +                       logger.Errorf("cannot set error status for machine %q: %v", machine,
> err1)
>                         return err1
>                 }
>                 return nil
> @@ -350,7 +350,7 @@
>                 // encounter surprising problems.
>                 return err
>         }
> -       logger.Info("started machine %s as instance %s", machine, inst.Id())
> +       logger.Infof("started machine %s as instance %s", machine, inst.Id())
>         return nil
>   }
>
>
>
> Index: worker/uniter/jujuc/juju-log_test.go
> === modified file 'worker/uniter/jujuc/juju-log_test.go'
> --- worker/uniter/jujuc/juju-log_test.go        2013-05-30 06:54:40 +0000
> +++ worker/uniter/jujuc/juju-log_test.go        2013-06-10 04:25:51 +0000
> @@ -28,7 +28,7 @@
>   }
>
>   func assertLogs(c *C, ctx jujuc.Context, badge string) {
> -       loggo.ConfigureLogging("juju=DEBUG")
> +       loggo.ConfigureLoggers("juju=DEBUG")
>         writer := &loggo.TestWriter{}
>         old_writer, err := loggo.ReplaceDefaultWriter(writer)
>         c.Assert(err, IsNil)
>
>
>
>
>
> --
> https://code.launchpad.net/~thumper/juju-core/update-loggo/+merge/168342
> Your team juju hackers is requested to review the proposed merge of lp:~thumper/juju-core/update-loggo into lp:juju-core.