Implement juju-log command

Implement juju-log command

Inspired by comments on go-jujuc-server, this involved changing
server.Context to expose GetCommand rather than a Log method (which has
moved to the new JujuLogCommand type).

https://code.launchpad.net/~fwereade/juju/go-juju-log-command/+merge/104099

(do not edit description out of merge proposal)

[fwereade, 2012-04-30 12:33:41]

Please take a look.

[TheMue, 2012-04-30 13:16:18]

On 2012/04/30 12:33:41, fwereade wrote:
> Please take a look.

LGTM, but will it be "juju log ..." or "juju juju-log ..."?

[rog, 2012-04-30 13:16:20]

LGTM with some minor suggestions and one must-fix (Debugf("%s"))

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:30: c = &JujuLogCommand{ctx: ctx}
return &JujuLogCommand{ctx: ctx}, nil
?

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:34: return
return nil, fmt.Errorf("unknown command: %s", name)

and lose the default clause?

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:26: f.BoolVar(&c.Debug, "debug", false, "log
<message> at debug level")
s/<message> //

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:32: return errors.New("no <message> specified")
s/<message>/message/

i don't think the angle-brackets help much.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:40: s := []string{}
this seems quite awkward but i tried and couldn't come up with a much better
version.

for the record: 

var msg interface{} = c.Message
if loc, rel := c.ctx.LocalUnitName, c.ctx.RelationName; loc != "" || rel != "" {
	buf.WriteString(loc)
	if loc != "" && rel != "" {
		buf.WriteString(" ")
	}
	fmt.Fprintf(&buf, "%s: %s", rel, msg)
	msg = buf.Bytes()
}
if c.Debug {
	log.Debugf("%s", msg)
} else {
	log.Printf("%s", msg)
}

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:49: msg = fmt.Sprintf("%s: ", strings.Join(s, " "))
+ msg
msg = strings.Join(s, " ") + ": " + msg
?

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:52: log.Debugf(msg)
you need Debugf("%s", msg) here and below otherwise %'s in the debug message
will be corrupted.

[fwereade, 2012-04-30 13:18:33]

On 2012/04/30 13:16:18, TheMue wrote:
> On 2012/04/30 12:33:41, fwereade wrote:
> > Please take a look.
> 
> LGTM, but will it be "juju log ..." or "juju juju-log ..."?

It's `juju-log [--debug] "some message"` (it's one of the commands you can use
in hooks)

[fwereade, 2012-04-30 13:33:01]

Please take a look.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:30: c = &JujuLogCommand{ctx: ctx}
On 2012/04/30 13:16:20, rog wrote:
> return &JujuLogCommand{ctx: ctx}, nil
> ?

Done.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:34: return
On 2012/04/30 13:16:20, rog wrote:
> return nil, fmt.Errorf("unknown command: %s", name)
> 
> and lose the default clause?

Done.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:26: f.BoolVar(&c.Debug, "debug", false, "log
<message> at debug level")
On 2012/04/30 13:16:20, rog wrote:
> s/<message> //

Done.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:32: return errors.New("no <message> specified")
On 2012/04/30 13:16:20, rog wrote:
> s/<message>/message/
> 
> i don't think the angle-brackets help much.

Done.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:40: s := []string{}
On 2012/04/30 13:16:20, rog wrote:
> this seems quite awkward but i tried and couldn't come up with a much better
> version.
> 
> for the record: 
> 
> var msg interface{} = c.Message
> if loc, rel := c.ctx.LocalUnitName, c.ctx.RelationName; loc != "" || rel != ""
{
> 	buf.WriteString(loc)
> 	if loc != "" && rel != "" {
> 		buf.WriteString(" ")
> 	}
> 	fmt.Fprintf(&buf, "%s: %s", rel, msg)
> 	msg = buf.Bytes()
> }
> if c.Debug {
> 	log.Debugf("%s", msg)
> } else {
> 	log.Printf("%s", msg)
> }

I feel that mine, while a touch lumpen, is easier to understand; would prefer to
keep it.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:49: msg = fmt.Sprintf("%s: ", strings.Join(s, " "))
+ msg
On 2012/04/30 13:16:20, rog wrote:
> msg = strings.Join(s, " ") + ": " + msg
> ?

Done.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:52: log.Debugf(msg)
On 2012/04/30 13:16:20, rog wrote:
> you need Debugf("%s", msg) here and below otherwise %'s in the debug message
> will be corrupted.

Eh? What's talking abou... oh.

Thank you, nice catch. Done.

[rog, 2012-04-30 14:17:12]

LGTM
awesome chickens indeed.

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6129053/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:40: s := []string{}
On 2012/04/30 13:33:01, fwereade wrote:
> On 2012/04/30 13:16:20, rog wrote:
> > this seems quite awkward but i tried and couldn't come up with a much better
> > version.
> > 
> > for the record: 
> > 
> > var msg interface{} = c.Message
> > if loc, rel := c.ctx.LocalUnitName, c.ctx.RelationName; loc != "" || rel !=
""
> {
> > 	buf.WriteString(loc)
> > 	if loc != "" && rel != "" {
> > 		buf.WriteString(" ")
> > 	}
> > 	fmt.Fprintf(&buf, "%s: %s", rel, msg)
> > 	msg = buf.Bytes()
> > }
> > if c.Debug {
> > 	log.Debugf("%s", msg)
> > } else {
> > 	log.Printf("%s", msg)
> > }
> 
> I feel that mine, while a touch lumpen, is easier to understand; would prefer
to
> keep it.

that's fine. it's unlikely ever to be a performance bottleneck.

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:27: func (ctx *Context) GetCommand(name string) (c
cmd.Command, err error) {
no need for the named return values now.

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:25: f.BoolVar(&c.Debug, "debug", false, "log
message at debug level")
i think "log at debug level" reads *slightly* more nicely. (my ambiguity sensors
seem to kick in on the above message, verb vs adjective for "log").
perhaps "log the message at debug level" as a compromise?

[niemeyer, 2012-05-01 06:15:46]

Nice, LGTM, with a suggestion about the command behavior:

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:33: c.Message = args[0]
This should probably be something like:

    c.Message = strings.Join(args, " ")

so that something like this works:

    juju-log Something bad happened.

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:34: return cmd.CheckEmpty(args[1:])
and return nil here.

[fwereade, 2012-05-02 08:01:52]

*** Submitted:

Implement juju-log command

Inspired by comments on go-jujuc-server, this involved changing
server.Context to expose GetCommand rather than a Log method (which has
moved to the new JujuLogCommand type).

R=rog, niemeyer
CC=
https://codereview.appspot.com/6129053

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:27: func (ctx *Context) GetCommand(name string) (c
cmd.Command, err error) {
On 2012/04/30 14:17:12, rog wrote:
> no need for the named return values now.

Done.

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:25: f.BoolVar(&c.Debug, "debug", false, "log
message at debug level")
On 2012/04/30 14:17:12, rog wrote:
> i think "log at debug level" reads *slightly* more nicely. (my ambiguity
sensors
> seem to kick in on the above message, verb vs adjective for "log").
> perhaps "log the message at debug level" as a compromise?

Plain "log at debug level" is fine by me. Done.

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:33: c.Message = args[0]
On 2012/05/01 06:15:47, niemeyer wrote:
> This should probably be something like:
> 
>     c.Message = strings.Join(args, " ")
> 
> so that something like this works:
> 
>     juju-log Something bad happened.

Done.

https://codereview.appspot.com/6129053/diff/6001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:34: return cmd.CheckEmpty(args[1:])
On 2012/05/01 06:15:47, niemeyer wrote:
> and return nil here.

Done.