Basic config-get command

Basic config-get command

Note that the command's output is wrong: it's enough to demonstrate that the
right data is being found and used, but that's all. Expect output formatting
more like python in a followup branch.

https://code.launchpad.net/~fwereade/juju/go-config-get-command-start/+merge/104370

(do not edit description out of merge proposal)

[fwereade, 2012-05-02 11:43:59]

Please take a look.

[rog, 2012-05-02 13:29:54]

looking good; just a few naming and wording suggestions.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:9: // ConfigGetCommand implements the
`config-get` command. It requires a Context
i'd lose the backquotes.

and i'm not sure that the second sentence is necessary - it's documented well
enough in checkCtx below, i think.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:16: // checkCtx validates that the command's
Context is suitable.
// checkCtx checks that the command has a non-zero
// state and local unit name.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:20: } else if c.ctx.LocalUnitName == "" {
lose the else.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:26: var purpose = "write service configuration to
stdout"
"print service configuration" ?

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:29: if it is, a single value will be written.
If a key is given, only the value for that key will be printed.

?

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:32: // Info returns usage information.
i think we can lose the comments on these three methods - they're standard
Command methods and the comments don't add much IMHO.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:68: value, _ = conf.Get(c.Key)
s/, _//

although you might want to check and print an error if the key isn't found, i
suppose.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:71: fmt.Fprintf(ctx.Stdout, "%v\n", value)
fmt.Println(value)

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

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:21: St             *state.State
can't we call this "State"?

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:27: type ctxCheckCommand interface {
how about calling this "command" and using it to specify the behaviour common
common to all the server commands?

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:30: // checkCtx should return an error if the
ctxCheckCommand is not
i don't mind "ctx" as a variable name, but i think as a method it reads more
nicely to have a whole word:

// checkContext should return an error if the command
// does not have a suitable Context.

?

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

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:45: LocalUnitName: "minecraft/0"}
i'd put the } on the next line.

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

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:18: // checkCtx validates that the command's
Context is suitable.
d

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:23: // Info returns usage information.
d

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/juju-log.go#ne...
cmd/jujuc/server/juju-log.go:28: // Init parses the command line and returns any
errors encountered.
d

[fwereade, 2012-05-02 15:52:32]

Please take a look.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:9: // ConfigGetCommand implements the
`config-get` command. It requires a Context
On 2012/05/02 13:29:54, rog wrote:
> i'd lose the backquotes.
> 
> and i'm not sure that the second sentence is necessary - it's documented well
> enough in checkCtx below, i think.

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:16: // checkCtx validates that the command's
Context is suitable.
On 2012/05/02 13:29:54, rog wrote:
> // checkCtx checks that the command has a non-zero
> // state and local unit name.

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:20: } else if c.ctx.LocalUnitName == "" {
On 2012/05/02 13:29:54, rog wrote:
> lose the else.

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:26: var purpose = "write service configuration to
stdout"
On 2012/05/02 13:29:54, rog wrote:
> "print service configuration" ?

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:29: if it is, a single value will be written.
On 2012/05/02 13:29:54, rog wrote:
> If a key is given, only the value for that key will be printed.
> 
> ?

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:32: // Info returns usage information.
On 2012/05/02 13:29:54, rog wrote:
> i think we can lose the comments on these three methods - they're standard
> Command methods and the comments don't add much IMHO.

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:68: value, _ = conf.Get(c.Key)
On 2012/05/02 13:29:54, rog wrote:
> s/, _//
> 
> although you might want to check and print an error if the key isn't found, i
> suppose.

I don't think that's possible... (and I don't want an error; to match the
python, I should just return nil).

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/config-get.go#...
cmd/jujuc/server/config-get.go:71: fmt.Fprintf(ctx.Stdout, "%v\n", value)
On 2012/05/02 13:29:54, rog wrote:
> fmt.Println(value)

Fprintln, maybe :).

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

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:21: St             *state.State
On 2012/05/02 13:29:54, rog wrote:
> can't we call this "State"?

Done. (but but consistency with state package)

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:27: type ctxCheckCommand interface {
On 2012/05/02 13:29:54, rog wrote:
> how about calling this "command" and using it to specify the behaviour common
> common to all the server commands?

Done.

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:30: // checkCtx should return an error if the
ctxCheckCommand is not
On 2012/05/02 13:29:54, rog wrote:
> i don't mind "ctx" as a variable name, but i think as a method it reads more
> nicely to have a whole word:
> 
> // checkContext should return an error if the command
> // does not have a suitable Context.
> 
> ?

Done.

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

https://codereview.appspot.com/6163044/diff/1/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:45: LocalUnitName: "minecraft/0"}
On 2012/05/02 13:29:54, rog wrote:
> i'd put the } on the next line.

Done.

[fwereade, 2012-05-03 09:23:35]

Please take a look.

[rog, 2012-05-03 09:39:48]

LGTM bar minors below.

https://codereview.appspot.com/6163044/diff/7001/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/7001/cmd/jujuc/server/config-get....
cmd/jujuc/server/config-get.go:65: fmt.Fprintln(ctx.Stdout, value)
oh yeah, i didn't see the "ctx.".
doh!

https://codereview.appspot.com/6163044/diff/12001/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/12001/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:25: var purpose = "print service configuration"
sorry for not mentioning this before, but is there a reason these aren't
a) constants?
b) inlined into the Info method?

when there are several commands around, using the global variable "purpose" for
just one of them seems odd.

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

https://codereview.appspot.com/6163044/diff/12001/cmd/jujuc/server/context.go...
cmd/jujuc/server/context.go:35: getCommand := commands[name]
s/getCommand/c/ ?
or "command" if c is too short for you.

it's just a command, not a "getCommand"...

[fwereade, 2012-05-03 10:14:30]

Please take a look.

https://codereview.appspot.com/6163044/diff/12001/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/12001/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:25: var purpose = "print service configuration"
On 2012/05/03 09:39:49, rog wrote:
> sorry for not mentioning this before, but is there a reason these aren't
> a) constants?
> b) inlined into the Info method?
> 
> when there are several commands around, using the global variable "purpose"
for
> just one of them seems odd.

Let's say there's no sane reason ;). Done.

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

https://codereview.appspot.com/6163044/diff/12001/cmd/jujuc/server/context.go...
cmd/jujuc/server/context.go:35: getCommand := commands[name]
On 2012/05/03 09:39:49, rog wrote:
> s/getCommand/c/ ?
> or "command" if c is too short for you.
> 
> it's just a command, not a "getCommand"...

Done according to IRC discussion.

[niemeyer, 2012-05-03 18:00:29]

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:11: ctx *Context
This is provided by the user when creating the command, so I'm wondering if
there's any value in keeping it private. It feels like even embedding it could
make sense. Something like:

type ConfigGetCommand struct {
        *Context
        Key string // The key to show. If empty, show all.
}

That said, after reading further, I'm now quite confused about why ctx is here
at all. See below.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:12: Key string
Key string // The key to show. If empty, show all.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:46: unit, err :=
c.ctx.State.Unit(c.ctx.LocalUnitName)
How come we have two different contexts here: c.ctx and ctx? Sounds like
something is wrong in the design.

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

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/juju-log.g...
cmd/jujuc/server/juju-log.go:39: func (c *JujuLogCommand) Run(_ *cmd.Context)
error {
Yeah, there's something wrong indeed. The argument is ignored, and we use the
context from the command below. We have to fix this.

[niemeyer, 2012-05-03 19:04:44]

LGTM, assuming we clarify just a bit the relationship as suggested below.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:11: ctx *Context
Alright, thanks for clarifying on IRC. It sounds like it's just a naming issue.
I have a suggestion to run by you. What if we named server.Context as
ClientContext, and used it here as an anonymous field as suggested. Like this:

type ConfigGetCommand struct {
        *ClientContext
        Key string // The config key to show. If empty, show all.
}

Being an anonymous field means we can use, its fields directly below, which
seems alright for this case. For example, c.LocalUnitName, etc.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
File cmd/jujuc/server/config-get_test.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get_test.go:33: return w.(*bytes.Buffer).String()
If it's taking an io.Writer, ideally it should respect io.Writer's interface to
produce the string. For example:

  data, err := ioutil.ReadAll(w)
  if err != nil {
      panic(err)
  }
  return string(data)

[fwereade, 2012-05-03 22:21:30]

I'm not quite sure where you're going with the str() advice, so I'll re-propose
without changing that bit yet; let me know.

PS Hmm, I wonder whether StateContext would fit better than ClientContext..?

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:11: ctx *Context
On 2012/05/03 19:04:44, niemeyer wrote:
> Alright, thanks for clarifying on IRC. It sounds like it's just a naming
issue.
> I have a suggestion to run by you. What if we named server.Context as
> ClientContext, and used it here as an anonymous field as suggested. Like this:
> 
> type ConfigGetCommand struct {
>         *ClientContext
>         Key string // The config key to show. If empty, show all.
> }
> 
> Being an anonymous field means we can use, its fields directly below, which
> seems alright for this case. For example, c.LocalUnitName, etc.

I'm not 100% sold on the Client bit of ClientContext, but this is definitely the
right direction. Thanks; done.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:12: Key string
On 2012/05/03 18:00:29, niemeyer wrote:
> Key string // The key to show. If empty, show all.

Done.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:46: unit, err :=
c.ctx.State.Unit(c.ctx.LocalUnitName)
On 2012/05/03 18:00:29, niemeyer wrote:
> How come we have two different contexts here: c.ctx and ctx? Sounds like
> something is wrong in the design.

Addressed elsewhere.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
File cmd/jujuc/server/config-get_test.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get_test.go:33: return w.(*bytes.Buffer).String()
On 2012/05/03 19:04:44, niemeyer wrote:
> If it's taking an io.Writer, ideally it should respect io.Writer's interface
to
> produce the string. For example:
> 
>   data, err := ioutil.ReadAll(w)
>   if err != nil {
>       panic(err)
>   }
>   return string(data)

I don't follow this. A cmd.Context contains an io.Writer for each of stdout and
stderr, because that's all that "real" code expects to do with them. In tests,
we want something that we can, well, "read"... but not necessarily in the
io.Reader sense (ie we just want to be able to get the data out somehow).

bytes.Buffer fulfils this need admirably; and so, while it would be perfectly
possible to Read it rather than String it, in either case we depend on the
special secret knowledge that it's not really *just* an io.Writer.

The choices I can see are:

1) Make cmd.Context's fields into io.ReadWriters; this feels misleading at best
and evil at worst, given that they will frequently really be
os.Stdout/os.Stderr.

2) Return the buffers from dummyContext separately and interact with them as
real bytes.Buffers (or io.ReadWriters) without casting; this gets unwieldy fast.

3) Make assumptions about the underlying types, but only in the tests; this
feels morally somewhat grubby, but is localized and expedient.

So... I picked (3). I'm not sure whether you're comfortable with  the general
idea, and just making a pleasing-symetry argument re Read/Write, or whether
there are other forces in play that I haven't spotted. Please advise :).

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

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/juju-log.g...
cmd/jujuc/server/juju-log.go:39: func (c *JujuLogCommand) Run(_ *cmd.Context)
error {
On 2012/05/03 18:00:29, niemeyer wrote:
> Yeah, there's something wrong indeed. The argument is ignored, and we use the
> context from the command below. We have to fix this.

Addressed elsewhere.

[fwereade, 2012-05-03 22:23:13]

Please take a look.

[niemeyer, 2012-05-04 00:00:16]

LGTM, thanks. It'd be nice to rename the test function. This is looking great
either way.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:11: ctx *Context
On 2012/05/03 22:21:30, fwereade wrote:
> I'm not 100% sold on the Client bit of ClientContext, but this is definitely
the
> right direction. Thanks; done.

This is really the context from the client that is calling this "server"
package, which is why Client seems to make sense to me.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
File cmd/jujuc/server/config-get_test.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get_test.go:33: return w.(*bytes.Buffer).String()
On 2012/05/03 22:21:30, fwereade wrote:
> I don't follow this. A cmd.Context contains an io.Writer for each of stdout
and
> stderr, because that's all that "real" code expects to do with them. In tests,

Sorry, I've seen io.Writer and thought about io.Reader. Obviously reading from
an io.Writer won't work.

Your approach looks good. Can we please just label this function as bufferString
to be more clear what is going on?

[fwereade, 2012-05-04 07:56:17]

*** Submitted:

Basic config-get command

Note that the command's output is wrong: it's enough to demonstrate that the
right data is being found and used, but that's all. Expect output formatting
more like python in a followup branch.

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

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get.go:11: ctx *Context
On 2012/05/04 00:00:16, niemeyer wrote:
> On 2012/05/03 22:21:30, fwereade wrote:
> > I'm not 100% sold on the Client bit of ClientContext, but this is definitely
> the
> > right direction. Thanks; done.
> 
> This is really the context from the client that is calling this "server"
> package, which is why Client seems to make sense to me.

Holding up cmd.Context and server.Context together, though, the cmd.Context is
actually holding data relevant to the client; while the server.Context was
(theoretically) chosen by the client, but contains data that's IMO very specific
to the server (and all very much related to state; hence maybe StateContext).

No reason for this to hold up progress, though, we can figure it out at our
leisure.

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
File cmd/jujuc/server/config-get_test.go (right):

https://codereview.appspot.com/6163044/diff/11003/cmd/jujuc/server/config-get...
cmd/jujuc/server/config-get_test.go:33: return w.(*bytes.Buffer).String()
On 2012/05/04 00:00:16, niemeyer wrote:
> On 2012/05/03 22:21:30, fwereade wrote:
> > I don't follow this. A cmd.Context contains an io.Writer for each of stdout
> and
> > stderr, because that's all that "real" code expects to do with them. In
tests,
> 
> Sorry, I've seen io.Writer and thought about io.Reader. Obviously reading from
> an io.Writer won't work.
> 
> Your approach looks good. Can we please just label this function as
bufferString
> to be more clear what is going on?

Done. (fixed similarly in cmd_test, which uses the same pattern)