Merge lp://staging/~dimitern/juju-core/032-deuglify-logging-for-main-packages into lp://staging/~juju/juju-core/trunk

Proposed by Dimiter Naydenov
Status: Merged
Merged at revision: 1149
Proposed branch: lp://staging/~dimitern/juju-core/032-deuglify-logging-for-main-packages
Merge into: lp://staging/~juju/juju-core/trunk
Diff against target: 481 lines (+49/-59)
12 files modified
cmd/juju/main_test.go (+2/-2)
cmd/juju/publish.go (+5/-5)
cmd/juju/ssh.go (+2/-2)
cmd/juju/synctools.go (+6/-6)
cmd/jujud/agent.go (+7/-7)
cmd/jujud/machine.go (+6/-6)
cmd/jujud/unit.go (+1/-1)
cmd/jujud/upgrade.go (+11/-11)
cmd/logging.go (+1/-8)
cmd/logging_test.go (+7/-7)
cmd/supercommand.go (+0/-3)
cmd/supercommand_test.go (+1/-1)
To merge this branch: bzr merge lp://staging/~dimitern/juju-core/032-deuglify-logging-for-main-packages
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+158445@code.staging.launchpad.net

Description of the change

deuglify logging for main packages.

Removed "cmd/juju*: " badge from all commands.

Also remove the "JUJU:" prefix and the automatic
command prefix and name which was just repeating
what's already in the message itself.
So now, instead of:

ERROR JUJU:jujutest:blah jujutest blah command failed: BAM!
ERROR JUJU:juju:bootstrap juju bootstrap command failed: dummy.Bootstrap is broken
INFO JUJU:test hello

We'll have just:
ERROR command failed: BAM!
INFO hello
ERROR command failed: dummy.Bootstrap is broken

The timestamp with format like 2013/04/12 09:59:16
is still there, before the message.

https://codereview.appspot.com/8674043/

To post a comment you must log in.
Revision history for this message
Dimiter Naydenov (dimitern) wrote :
Download full text (13.4 KiB)

Reviewers: mp+158445_code.launchpad.net,

Message:
Please take a look.

Description:
deuglify logging for main packages.

Removed "cmd/juju*: " badge from all commands.

https://code.launchpad.net/~dimitern/juju-core/032-deuglify-logging-for-main-packages/+merge/158445

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/publish.go
   M cmd/juju/ssh.go
   M cmd/juju/synctools.go
   M cmd/jujud/agent.go
   M cmd/jujud/machine.go
   cmd/jujud/unit.go
   M cmd/jujud/upgrade.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/juju/publish.go
=== modified file 'cmd/juju/publish.go'
--- cmd/juju/publish.go 2013-04-09 18:52:24 +0000
+++ cmd/juju/publish.go 2013-04-11 17:51:37 +0000
@@ -117,7 +117,7 @@
   if err != nil {
    return fmt.Errorf("cannot obtain local digest: %v", err)
   }
- log.Infof("cmd/juju: local digest is %s", localDigest)
+ log.Infof("local digest is %s", localDigest)

   ch, err := charm.ReadDir(branch.Location())
   if err != nil {
@@ -131,7 +131,7 @@
   if _, ok := err.(*charm.NotFoundError); ok {
    oldEvent, err = charm.Store.Event(curl, "")
    if _, ok := err.(*charm.NotFoundError); ok {
- log.Infof("cmd/juju: charm %s is not yet in the store", curl)
+ log.Infof("charm %s is not yet in the store", curl)
     err = nil
    }
   }
@@ -143,13 +143,13 @@
    return handleEvent(ctx, curl, oldEvent)
   }

- log.Infof("cmd/juju: sending charm to the charm store...")
+ log.Infof("sending charm to the charm store...")

   err = branch.Push(&bzr.PushAttr{Location: pushLocation, Remember: true})
   if err != nil {
    return err
   }
- log.Infof("cmd/juju: charm sent; waiting for it to be published...")
+ log.Infof("charm sent; waiting for it to be published...")
   for {
    time.Sleep(c.pollDelay)
    newEvent, err := charm.Store.Event(curl, "")
@@ -175,7 +175,7 @@
   switch event.Kind {
   case "published":
    curlRev := curl.WithRevision(event.Revision)
- log.Infof("cmd/juju: charm published at %s as %s", event.Time, curlRev)
+ log.Infof("charm published at %s as %s", event.Time, curlRev)
    fmt.Fprintln(ctx.Stdout, curlRev)
   case "publish-error":
    return fmt.Errorf("charm could not be published: %s",
strings.Join(event.Errors, "; "))

Index: cmd/juju/ssh.go
=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go 2013-03-27 15:42:49 +0000
+++ cmd/juju/ssh.go 2013-04-11 17:51:37 +0000
@@ -72,13 +72,13 @@
  func (c *SSHCommon) hostFromTarget(target string) (string, error) {
   // is the target the id of a machine ?
   if state.IsMachineId(target) {
- log.Infof("cmd/juju: looking up address for machine %s...", target)
+ log.Infof("looking up address for machine %s...", target)
    // TODO(dfc) maybe we should have machine.PublicAddress() ?
    return c.machinePublicAddress(target)
   }
   // maybe the targe...

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 2013/04/11 17:53:00, dimitern wrote:
> Please take a look.

LGTM
though weren't you going to remove JUJU from the log lines too?

https://codereview.appspot.com/8674043/

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

On 2013/04/11 18:06:03, rog wrote:
> On 2013/04/11 17:53:00, dimitern wrote:
> > Please take a look.

> LGTM
> though weren't you going to remove JUJU from the log lines too?

You're right, I missed that, but I did that as well and I'm going to
repropose soon.

https://codereview.appspot.com/8674043/

Revision history for this message
Dimiter Naydenov (dimitern) wrote :
Revision history for this message
Roger Peppe (rogpeppe) wrote :

nice.

LGTM with one small consideration below.

https://codereview.appspot.com/8674043/diff/6001/cmd/logging.go
File cmd/logging.go (left):

https://codereview.appspot.com/8674043/diff/6001/cmd/logging.go#oldcode35
cmd/logging.go:35: return l.Logger.Output(calldepth,
strings.Join(output, " "))
yes!

https://codereview.appspot.com/8674043/diff/6001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/8674043/diff/6001/cmd/supercommand_test.go#newcode112
cmd/supercommand_test.go:112: c.Assert(bufferString(ctx.Stderr),
Matches, `^.* ERROR jujutest blah command failed: BAM!
i wonder if "command" is necessary here.

ERROR jujutest blah failed: BAM!

would perhaps be better i think.

https://codereview.appspot.com/8674043/

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

*** Submitted:

deuglify logging for main packages.

Removed "cmd/juju*: " badge from all commands.

Also remove the "JUJU:" prefix and the automatic
command prefix and name which was just repeating
what's already in the message itself.
So now, instead of:

ERROR JUJU:jujutest:blah jujutest blah command failed: BAM!
ERROR JUJU:juju:bootstrap juju bootstrap command failed: dummy.Bootstrap
is broken
INFO JUJU:test hello

We'll have just:
ERROR command failed: BAM!
INFO hello
ERROR command failed: dummy.Bootstrap is broken

The timestamp with format like 2013/04/12 09:59:16
is still there, before the message.

R=rog, dfc
CC=
https://codereview.appspot.com/8674043

https://codereview.appspot.com/8674043/diff/6001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/8674043/diff/6001/cmd/supercommand_test.go#newcode112
cmd/supercommand_test.go:112: c.Assert(bufferString(ctx.Stderr),
Matches, `^.* ERROR jujutest blah command failed: BAM!
On 2013/04/12 08:27:52, rog wrote:
> i wonder if "command" is necessary here.

> ERROR jujutest blah failed: BAM!

> would perhaps be better i think.

As agreed online, I'll leave just "ERROR command failed: BAM!"

https://codereview.appspot.com/8674043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches