implement relation-set

implement relation-set

...with HookContext, it's basically trivial. The relation id parsing is
tedious, and will want to be farmed off to something common as soon as I
implement another relation command; for now, it's not really justified.

https://code.launchpad.net/~fwereade/juju-core/relation-set/+merge/117326

Requires: https://code.launchpad.net/~fwereade/juju-core/jujuc-server-unit-context/+merge/117253

(do not edit description out of merge proposal)

[fwereade, 2012-07-30 19:23:03]

Please take a look.

[rog, 2012-07-31 08:28:08]

looking good.

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

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:50: "relation-set": NewRelationSetCommand,
looks like this is implementing relation-set, not relation-get :-)

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:115: // is currently -1, it will return empty
strings. Otherwise, it will panic
Perhaps slightly more obvious as: "If the context is not currently running a
relation hook, it will return empty strings." (means the reader doesn't have to
go back to the RelationId definition to see what RelationId==-1 signifies).

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.go
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.g...
cmd/jujuc/server/relation-set.go:38: return fmt.Errorf("no relation specified")
"running in a non-relation hook" or something similar?
after all, the user isn't specifying the relation - it's already specified (or
not, for non-relation hooks).

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.g...
cmd/jujuc/server/relation-set.go:66: err = goyaml.Unmarshal([]byte(parts[1]),
&value)
this surprises me. 
does this mean it's an error to do a relation-set of anything that doesn't parse
as YAML?

for instance: relation-set "message=can't do something"
would give an error.

what are we gaining by not just setting a simple string value?

[fwereade, 2012-07-31 23:00:11]

Please take a look.

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

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:50: "relation-set": NewRelationSetCommand,
On 2012/07/31 08:28:08, rog wrote:
> looks like this is implementing relation-set, not relation-get :-)

Er. Details, details...

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:115: // is currently -1, it will return empty
strings. Otherwise, it will panic
On 2012/07/31 08:28:08, rog wrote:
> Perhaps slightly more obvious as: "If the context is not currently running a
> relation hook, it will return empty strings." (means the reader doesn't have
to
> go back to the RelationId definition to see what RelationId==-1 signifies).

I'll try again :).

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.go
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.g...
cmd/jujuc/server/relation-set.go:38: return fmt.Errorf("no relation specified")
On 2012/07/31 08:28:08, rog wrote:
> "running in a non-relation hook" or something similar?
> after all, the user isn't specifying the relation - it's already specified (or
> not, for non-relation hooks).

I don't think so -- the user might be specifying the relation, whatever sort of
context we're in. This indicates that -r was not set, and it needs to be.

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.g...
cmd/jujuc/server/relation-set.go:66: err = goyaml.Unmarshal([]byte(parts[1]),
&value)
On 2012/07/31 08:28:08, rog wrote:
> this surprises me. 
> does this mean it's an error to do a relation-set of anything that doesn't
parse
> as YAML?
> 
> for instance: relation-set "message=can't do something"
> would give an error.
> 
> what are we gaining by not just setting a simple string value?

The glorious taste of sweet sweet crack, it emerges. Nice catch.

[rog, 2012-08-03 13:49:42]

LGTM modulo comments below.

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.go
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/1/cmd/jujuc/server/relation-set.g...
cmd/jujuc/server/relation-set.go:66: err = goyaml.Unmarshal([]byte(parts[1]),
&value)
On 2012/07/31 23:00:11, fwereade wrote:
> On 2012/07/31 08:28:08, rog wrote:
> > this surprises me. 
> > does this mean it's an error to do a relation-set of anything that doesn't
> parse
> > as YAML?
> > 
> > for instance: relation-set "message=can't do something"
> > would give an error.
> > 
> > what are we gaining by not just setting a simple string value?
> 
> The glorious taste of sweet sweet crack, it emerges. Nice catch.

that made me smile.

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

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:122: // is does not have a relation, it will return
empty strings. Otherwise, it
s/is //

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:15: Settings   map[string]interface{}
map[string]string ?

then we can lose the dynamic type conversion in Run.

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:60: if parts[0] == "" {
if len(parts) != 2 {

(consider what happens if kv is non-empty with no equals sign.)

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set_test.go (right):

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set_test.go:108: }, {
{
    arg: []string{"haha"},
    err: `cannot parse "=haha": no key specified`,
}

should probably test the empty arg too, and one with several  "=" characters.

[fwereade, 2012-08-06 13:26:20]

Please take a look.

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

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:122: // is does not have a relation, it will return
empty strings. Otherwise, it
On 2012/08/03 13:49:42, rog wrote:
> s/is //

Done.

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:15: Settings   map[string]interface{}
On 2012/08/03 13:49:42, rog wrote:
> map[string]string ?
> 
> then we can lose the dynamic type conversion in Run.

Good idea.

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:60: if parts[0] == "" {
On 2012/08/03 13:49:42, rog wrote:
> if len(parts) != 2 {
> 
> (consider what happens if kv is non-empty with no equals sign.)

Done.

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set_test.go (right):

https://codereview.appspot.com/6446067/diff/6001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set_test.go:108: }, {
On 2012/08/03 13:49:42, rog wrote:
> {
>     arg: []string{"haha"},
>     err: `cannot parse "=haha": no key specified`,
> }
> 
> should probably test the empty arg too, and one with several  "=" characters.

Done.

[niemeyer, 2012-08-06 15:19:36]

LGTM

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:24: "relation-set", "<key>=<value> [...]", "set
relation settings", "",
I suggest something like this for the sample parameters:

"key=value [key=value ...]"

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:29: _, defaultId := c.relationIdentifiers()
I've noticed a bunch of call sites ignore one of them, and it's a bit mysterious
at a glance which one it is (defaultId? relationIdentifiers? Which id is that?)

I'm wondering if breaking down this method into two would be sensible. It'd also
be great to be more clear whether it is an internal id or a user-level relation
id.

I suggest envRelation and envRelationId for the names. What do you think?

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:35: if relationId == "" {
And here is the proof of the earlier point. We're returning an empty identifier
as a valid identifier, and then having to verify if it's actually valid or not.
I'd rather be clearly reminded that the id may be invalid via the API.

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:56: return fmt.Errorf("no settings specified")
`expected "key=value" parameters, got nothing`

This is both an error and a nice hint of what must be done.

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:65: }
Both checks can be nicely put into a single one that is both an error and a
strong hint of how the command works:

if len(parts) != 2 || len(parts[0]) == 0 {
    return fmt.Errorf(`expected "key=value", got %q`, kv)
}

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set_test.go (right):

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set_test.go:42: var relationSetInitTests = []struct {
Nice set of tests.

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set_test.go:173: map[string]string{"base": ""},
"old" and "new" might be better name for these settings, to give the reader an
idea of what's going on.

[niemeyer, 2012-08-06 15:29:00]

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:35: if relationId == "" {
On 2012/08/06 15:19:36, niemeyer wrote:
> And here is the proof of the earlier point. We're returning an empty
identifier
> as a valid identifier, and then having to verify if it's actually valid or
not.
> I'd rather be clearly reminded that the id may be invalid via the API.

Sorry, I intended to remove this comment. Please ignore it.

[fwereade, 2012-08-06 16:43:22]

*** Submitted:

implement relation-set

...with HookContext, it's basically trivial. The relation id parsing is
tedious, and will want to be farmed off to something common as soon as I
implement another relation command; for now, it's not really justified.

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

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set.go (right):

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:24: "relation-set", "<key>=<value> [...]", "set
relation settings", "",
On 2012/08/06 15:19:36, niemeyer wrote:
> I suggest something like this for the sample parameters:
> 
> "key=value [key=value ...]"

Much nicer. Done.

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:29: _, defaultId := c.relationIdentifiers()
On 2012/08/06 15:19:36, niemeyer wrote:
> I've noticed a bunch of call sites ignore one of them, and it's a bit
mysterious
> at a glance which one it is (defaultId? relationIdentifiers? Which id is
that?)
> 
> I'm wondering if breaking down this method into two would be sensible. It'd
also
> be great to be more clear whether it is an internal id or a user-level
relation
> id.
> 
> I suggest envRelation and envRelationId for the names. What do you think?

SGTM, or at least sounds better than what I currently have. I think I'll want to
change it again, sooner or later, but I'm not yet sure what to; so... Done :).

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:56: return fmt.Errorf("no settings specified")
On 2012/08/06 15:19:36, niemeyer wrote:
> `expected "key=value" parameters, got nothing`
> 
> This is both an error and a nice hint of what must be done.

Done.

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set.go:65: }
On 2012/08/06 15:19:36, niemeyer wrote:
> Both checks can be nicely put into a single one that is both an error and a
> strong hint of how the command works:
> 
> if len(parts) != 2 || len(parts[0]) == 0 {
>     return fmt.Errorf(`expected "key=value", got %q`, kv)
> }

Done.

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
File cmd/jujuc/server/relation-set_test.go (right):

https://codereview.appspot.com/6446067/diff/9001/cmd/jujuc/server/relation-se...
cmd/jujuc/server/relation-set_test.go:173: map[string]string{"base": ""},
On 2012/08/06 15:19:36, niemeyer wrote:
> "old" and "new" might be better name for these settings, to give the reader an
> idea of what's going on.

Went with "change" and "expect" plus comment as discussed live.