Added Actions to charm.

Added Actions to charm.

Actions is a type encapsulating a charm's Action definitions and metadata.
Actions reads a YAML string conforming to the following spec:

actionspecs:
  <action name>:
    description: <string>
    params:
      ...
  <another action name>:
    description: <string>
    params:
      ...

The named Action must contain a "description" field and a "params" field.  The "params" field must be conformant to JSON-Schema Draft 4 as defined at http://json-schema.org/draft-04/schema# (see http://json-schema.org/latest/json-schema-core.html).  It should be a list of parameters to the Action, such as "outfile", "compression-ratio", etc for a snapshot Action; JSON-Schema properties such as $schema may also be defined.  For example:

...
    params:
      outfile: string
      compression-ratio: double
      ...

After being built, the "params" field is checked for conformity to
JSON-Schema.  See http://json-schema.org/ for details.  If it does not
conform to JSON-Schema, the YAML string will be rejected with an error.

https://code.launchpad.net/~binary132/juju-core/charm-actions/+merge/219926

(do not edit description out of merge proposal)

[Bodie, 2014-05-17 02:57:12]

Please take a look.

[fwereade, 2014-05-17 15:07:04]

This looks pretty much like what I was after. Just a bunch of trivials, really.
Ping me if any need discussion :).

https://codereview.appspot.com/94540044/diff/1/charm/actions.go
File charm/actions.go (right):

https://codereview.appspot.com/94540044/diff/1/charm/actions.go#newcode12
charm/actions.go:12: "github.com/xeipuuv/gojsonschema"
we need a godeps entry for this

https://codereview.appspot.com/94540044/diff/1/charm/actions.go#newcode43
charm/actions.go:43: return nil, fmt.Errorf("Empty actions definition")
we tend to go with lowercase for error messages -- they often get prepended to:
And caps look weird in that situation.

https://codereview.appspot.com/94540044/diff/1/charm/actions.go#newcode46
charm/actions.go:46: return nil, fmt.Errorf("actions.yaml failed to unmarshal --
key mismatch")
I feel like an empty actions probably ought to be valid... although it would
probably also be sensible, once we integrate this with actual charms, to have a
step that checks actions.yaml keys against implemented actions and fails on
mismatch.

https://codereview.appspot.com/94540044/diff/1/charm/actions.go#newcode51
charm/actions.go:51: return nil, fmt.Errorf("error loading Charm actions.yaml --
nonconformant params: %v", err)
we don't know it's an actions.yaml ;p.

I think we want to use the keys and (1) check action names for sanity ([a-z-],
with no leading or trailing -, perhaps? and (2) complain about the specific key.
"invalid params schema for action %q: %v".

Except! I think thumper has, or soon will have, landed some stuff that lets us
annotate errors in a more helpful way. Search the codebase for
"github.com/juju/errors", and use that for preference if it's available.

https://codereview.appspot.com/94540044/diff/1/charm/actions_test.go
File charm/actions_test.go (right):

https://codereview.appspot.com/94540044/diff/1/charm/actions_test.go#newcode21
charm/actions_test.go:21: }{
A few notes here:

(1) there's not much reason to make these tests global, and it's just an
encouragement to others to do really stupid stuff like use (or, worse, modify)
yamlReaderTests[1]. It's happened before :/. So please put them in the narrowest
scope that's practical.

(2) This feels like it'd be nicer as 2 separate tests -- one for errors, and one
for valid data. (and it'd be nice to have one or two more tests for the valid
stuff -- not to mention for the errors, for extra cases like missing
description, malformed action name, etc).

(3) It might just be me, but I'm finding the formatting a bit tricky to follow.
A few more linebreaks, possibly between fields and certainly between individual
tests, would help a lot.

https://codereview.appspot.com/94540044/diff/1/charm/actions_test.go#newcode58
charm/actions_test.go:58: func (s *ActionsSuite) TestNewActions(c *gc.C) {
I'm not sure how helpful NewActions is, really. But if you think you'll use it,
let it stand.

https://codereview.appspot.com/94540044/diff/1/charm/actions_test.go#newcode61
charm/actions_test.go:61: c.Assert(newActions, gc.NotNil)
newActions.Actions, maybe? and a (HasLen, 0) too, perhaps

https://codereview.appspot.com/94540044/diff/1/charm/actions_test.go#newcode64
charm/actions_test.go:64: // Does ReadActionsYaml properly handle malformed
YAML?
I don't think this is very accurate -- I'd drop it and go with "description"
fields on the test structs...

https://codereview.appspot.com/94540044/diff/1/charm/actions_test.go#newcode67
charm/actions_test.go:67: c.Logf("ReadActionsYaml test %d", i)
...and drop the "ReadActionsYaml" here, so we just have ("test %d: %s", i,
test.description).

And FWIW, please use "test" not "t". And feel free to fix that in any other code
you happen to edit ;).

[Bodie, 2014-05-21 21:09:39]

Please take a look.

[fwereade, 2014-05-21 21:33:51]

A bunch more comments, but hopefully they're pretty trivial to address. Let me
know.

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go
File charm/actions.go (right):

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go#newcode34
charm/actions.go:34: unmarshaledActions := newActions()
var unmarshaledActions Actions
goyaml.Unmarshal(data, &unmarshaledActions)

?

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go#newcode39
charm/actions.go:39: return nil, fmt.Errorf("empty actions definition")
I don't think a lack of actions is necessarily invalid, is it? And I don't see a
test for this...

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go#newcode43
charm/actions.go:43: return nil, fmt.Errorf("actions failed to unmarshal from
YAML %s", data)
I'm not sure that's a serious problem. It's just that there aren't any actions.
Similarly, if there's no config in config.yaml, it should just mean there's
nothing to configure.

Not tested anyway, right? ;p

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go#newcode60
charm/actions.go:60: return nil, fmt.Errorf("bad param name %s", paramName)
needs tests for param names too, I think

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go
File charm/actions_test.go (right):

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:22: actions     *charm.Actions
s/actions/expect/ perhaps?

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:24: // Test 1
The numbering will be tedious to maintain; and it'll be off by one from what the
test reports anyway ;p. I'd just drop it.

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:26: "A simple snapshot actions YAML with one parameter.",
description: "..."

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:28: `
yaml: `

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:29: actionspecs:
For general juju consistency, that should be "action-specs"; but honestly charm
authors don't want to see that. "actions" should be fine IMO.

Also, being explicit about serialization names is a good idea even if the
automatic conversion happens to be what you want -- depending on no future
programmer changing the names is a bit too optimistic for my tastes ;).

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:39: &charm.Actions{map[string]charm.ActionSpec{
actions: &charm.Actions{...

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:127: params:
How about one with "params" missing entirely?

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:133: Params:      map[string]interface{}(nil)}}},
I think you can just use nil there.

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:151: actions     *charm.Actions
`error string` -- and then test the error. gc.NotNil hides a multitude of sins.

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:157: `
similarly to above, please use explicit field initializers

https://codereview.appspot.com/94540044/diff/20001/charm/actions_test.go#newc...
charm/actions_test.go:199: outfile:
I'd drop the params in the tests that would work fine with empty ones.

[dave_cheney.net, 2014-05-21 21:42:36]

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go
File charm/actions.go (right):

https://codereview.appspot.com/94540044/diff/20001/charm/actions.go#newcode41
charm/actions.go:41: // If there's data but the Actions is still empty, there's
a problem.
Reflect is the wrong hammer here.

if len(data) != 0 is a simpler way of writing !reflect.DeepEqual(data, []byte{})

The sample applies to checking if unmarshalled actions is zero. i cannot see how
that can happen if there is more than 0 bytes of data and no error was returned
from Unmarshall

https://codereview.appspot.com/94540044/diff/20001/dependencies.tsv
File dependencies.tsv (right):

https://codereview.appspot.com/94540044/diff/20001/dependencies.tsv#newcode14
dependencies.tsv:14:
github.com/xeipuuv/gojsonschema	git	8b52be5671082fa71acba81b0e0fd53d0f1821e7
do these dependencies pass on both gc and gccgo. Sorry, this is a new
requirement as now we use both compilers to deliver juju on supported platforms.

[Bodie, 2014-05-22 05:10:50]

Please take a look.

[fwereade, 2014-05-22 08:52:08]

Just nitpicks in the actual code; a few more things with the tests. Several just
about tighter formatting, but a couple missing and one incorrect. Nearly there,
sorry it's slow.

https://codereview.appspot.com/94540044/diff/60001/charm/actions.go
File charm/actions.go (right):

https://codereview.appspot.com/94540044/diff/60001/charm/actions.go#newcode38
charm/actions.go:38: nameRule := regexp.MustCompile("^[^-][a-z-]+[^-]$")
FWIW I'd move this out to global scope (or at least the string used to create
the regexp)

https://codereview.appspot.com/94540044/diff/60001/charm/actions.go#newcode42
charm/actions.go:42: if badName {
FWIW I would write this slightly differently:

if valid := nameRule.MatchString(name); !valid {
    ...
}

...because (1) it keeps the var from leaking into a wider scope than it needs
and (2) I find it easier to see the negation; but it's a matter of taste. And if
the line gets too long I break that rule myself all the time anyway...

https://codereview.appspot.com/94540044/diff/60001/charm/actions.go#newcode46
charm/actions.go:46: if err != nil {
eg I probably wouldn't do it here.

https://codereview.appspot.com/94540044/diff/60001/charm/actions.go#newcode51
charm/actions.go:51: if badParam {
...but I would here.

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go
File charm/actions_test.go (right):

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:51: expectedActions: &charm.Actions{ActionSpecs:
map[string]charm.ActionSpec(nil)}},
just nil again, I think

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:171: {
}{{

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:194: description: Take a snapshot of the database.
s/  /    /

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:203: expectedError: "YAML error: line 4: mapping values
are not allowed in this context",
wrong error :)

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:206: {
}, {

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:208: 
d

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:214: 
d

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:218: {
}, {

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:220: 
d

https://codereview.appspot.com/94540044/diff/60001/charm/actions_test.go#newc...
charm/actions_test.go:229: }
missing tests for bad param names

https://codereview.appspot.com/94540044/diff/60001/dependencies.tsv
File dependencies.tsv (right):

https://codereview.appspot.com/94540044/diff/60001/dependencies.tsv#newcode14
dependencies.tsv:14:
github.com/xeipuuv/gojsonschema	git	8b52be5671082fa71acba81b0e0fd53d0f1821e7
did we sort out the licensing here? and in the other 2 for that matter?

[Bodie, 2014-05-22 16:54:37]

Please take a look.

https://codereview.appspot.com/94540044/diff/60001/dependencies.tsv
File dependencies.tsv (right):

https://codereview.appspot.com/94540044/diff/60001/dependencies.tsv#newcode14
dependencies.tsv:14:
github.com/xeipuuv/gojsonschema	git	8b52be5671082fa71acba81b0e0fd53d0f1821e7
On 2014/05/22 08:52:06, fwereade wrote:
> did we sort out the licensing here? and in the other 2 for that matter?

Good question -- xeipuuv's gojsonschema repo has Apache v2 License included in
the root directory now, but all the files are licensed with Apache v2.  Do we
need to open MR's to get a LICENSE.txt included in the repo root or is it
sufficient to have in the source files?

[fwereade, 2014-05-26 13:09:39]

LGTM, modulo gojsonschema uncertainty. Let's talk about that today.

https://codereview.appspot.com/94540044/diff/80001/charm/actions_test.go
File charm/actions_test.go (right):

https://codereview.appspot.com/94540044/diff/80001/charm/actions_test.go#newc...
charm/actions_test.go:259: c.Assert(err.Error(), gc.Equals, test.expectedError)
I'd probably go with a `c.Check(unma, IsNil); c.Check(err, ErrorMatches,
test.error)` and drop most of the logs.

(the reason for check in particular is that the test will continue to run, so we
get to see the results for the whole table)

[Bodie, 2014-05-26 16:43:04]

On 2014/05/26 13:09:39, fwereade wrote:
> LGTM, modulo gojsonschema uncertainty. Let's talk about that today.
> 
> https://codereview.appspot.com/94540044/diff/80001/charm/actions_test.go
> File charm/actions_test.go (right):
> 
>
https://codereview.appspot.com/94540044/diff/80001/charm/actions_test.go#newc...
> charm/actions_test.go:259: c.Assert(err.Error(), gc.Equals,
test.expectedError)
> I'd probably go with a `c.Check(unma, IsNil); c.Check(err, ErrorMatches,
> test.error)` and drop most of the logs.
> 
> (the reason for check in particular is that the test will continue to run, so
we
> get to see the results for the whole table)

Oops, those were left over from my debugging.  Cleaned up!

I'm patching up the obvious broken parts of gojsonreference and then we'll have
another look over this library.  Hopefully it will be usable.

[Bodie, 2014-05-28 19:46:52]

Please take a look.

[Bodie, 2014-05-28 19:56:33]

On 2014/05/28 19:46:52, Bodie wrote:
> Please take a look.

Bumping this re. William's request for review.

I've stripped out the bad code in gojsonreference and replaced it with clean
code that doesn't skip tests or ditch errors, etc.

However, it still doesn't satisfy the last two tests:

https://github.com/binary132/gojsonschema/blob/master/schema_test.go#L289

"TODO Pass failed tests : id(s) as scope for references is not implemented yet"

I'm not certain this is a critical requirement yet (it excludes a small subset
of possible JSON-Schemas) but it probably should be in before releasing the full
Actions feature since it's specified as a MUST in the IETF spec:

http://json-schema.org/latest/json-schema-core.html#anchor27

However, I don't imagine implementing it will be much additional work.

What do we think of this?

Pragmatically speaking, while it doesn't perfectly fit the spec, I think this is
good enough to go, bare minimum putting it on the roadmap to ensure it's wrapped
up before we expose the client API.

[gz, 2014-05-29 15:16:32]

A couple of things to fix, also you'll need to pull trunk to resolve
dependencies.tsv conflict.

On 2014/05/28 19:56:33, Bodie wrote:
> 
> I'm not certain this is a critical requirement yet (it excludes a small subset
> of possible JSON-Schemas) but it probably should be in before releasing the
full
> Actions feature since it's specified as a MUST in the IETF spec:
> 
> http://json-schema.org/latest/json-schema-core.html#anchor27
> 
> However, I don't imagine implementing it will be much additional work.
> 
> What do we think of this?
> 
> Pragmatically speaking, while it doesn't perfectly fit the spec, I think this
is
> good enough to go, bare minimum putting it on the roadmap to ensure it's
wrapped
> up before we expose the client API.

Sounds reasonable to me, we can land this without that for now.

[gz, 2014-05-29 15:17:05]

Small notes, tidying up the tests is optional.

https://codereview.appspot.com/94540044/diff/100001/charm/actions.go
File charm/actions.go (right):

https://codereview.appspot.com/94540044/diff/100001/charm/actions.go#newcode46
charm/actions.go:46: return nil, fmt.Errorf("invalid params schema for action
%q: %v", err)
This error branch doesn't seem to be tested. The fmt.Errorf is invalid?

https://codereview.appspot.com/94540044/diff/100001/charm/actions_test.go
File charm/actions_test.go (right):

https://codereview.appspot.com/94540044/diff/100001/charm/actions_test.go#new...
charm/actions_test.go:33: default: foo.bz2
Would really like to make these tests more reable by pulling out the literal
yaml somehow. The point is they're all subtly different, but a helper that takes
some args and gives back yamly text in the with the actions->params wrapping
would cover many.

[Bodie, 2014-05-30 05:21:46]

Please take a look.

[gz, 2014-05-30 14:23:46]

LGTM

[Bodie, 2014-05-30 15:21:59]

Please take a look.

[Bodie, 2014-05-30 15:37:28]

On 2014/05/30 15:21:59, Bodie wrote:
> Please take a look.

Very minor addition of more informative comments.