Implements "juju actions <service>".

Implements "juju actions <service>".

https://code.launchpad.net/~binary132/juju-core/skeletal_actions/+merge/216651

The client command "actions" queries against the service's charm, building a representation of the available actions from actions.yaml in the charm's root directory.

Actions themselves will be implemented as hooks in a future revision.

The actions.yaml spec is as follows:

actionspecs:
  <an action name>:
    description: "A description string for the action."
    params:
      <a param name>:
        description: "A description string for the parameter."
        type: (string|int|float|boolean)
        default: <a default value>
      <...more param names...>
  <...more action names...>

[Bodie, 2014-04-22 01:12:10]

Please take a look.

[natefinch, 2014-04-22 19:44:35]

Some thoughts and possibilities for cleanup below.  

One additional thought I had was if there's a better way to define parameters. 
It almost seems like we're reinventing json or yaml here, in which case, why not
just use json or yaml?  Specifically the action spec stuff... it seems like we
could eliminate the whole action spec, let the charm specify the parameters it
wants in a plaintext help document, and let the api just pass a yaml or json
document with the parameters.... let it be up to the charm to parse the document
and determine if it's correct or not.

But then again, I don't know what the specific requirements were for actions.

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

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode17
charm/actions.go:17: // This is an actual Action object.
comments about top level names should start with <name>.  So, like here this
should say 

// Action is <something>

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode35
charm/actions.go:35: func (param Param) error(err *error, name string, value
interface{}) {
Our standard is to use a single letter variable for the method receiver (p
Param)... that's also standard Go style.

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode37
charm/actions.go:37: *err = fmt.Errorf("parameter %q expected %s, got %#v",
name, param.Type, value)
Not a fan of discarding the original error message here.  Please tack it on the
end, that way we don't lose information like "strconv.ParseInt: parsing "abc":
invalid syntax"

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode55
charm/actions.go:55: panic(fmt.Errorf("parameter %q has unknown type %q", name,
param.Type))
panic is almost never the answer, especially for data that is getting parsed
from config files etc.... the function already returns an error, just return an
error here.

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode70
charm/actions.go:70: case "string":
Repeating these strings everywhere is bad.  You can do something like

type ParamType int

const (
   StringParam ParamType = iota
   IntParam ParamType
   ...
)

This is basically the way to do enums in Go.  That'll give you unique numbers
for each one.

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode79
charm/actions.go:79: panic(fmt.Errorf("parameter %q has unknown type %q", name,
param.Type))
as above, return an error, don't panic.

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode84
charm/actions.go:84: ActionSpecs map[string]ActionSpec
Instead of this, you could just do type ActionSpecs map[string]ActionSpec   
And then you can define the methods right on the new type.
That would remove the need for a special constructor, you could just do a :=
ActionSpec{}

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode103
charm/actions.go:103: return nil, fmt.Errorf("invalid : empty actions
definition")
try to avoid using colons in error messages, that's how we generally concatenate
errors from one layer to the next, like fmt.Errorf("failed doing foo: %v", err).
 You can probably drop the invalid part.

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode110
charm/actions.go:110: // Missing type is valid in python.
Do we really need compatibility with pyjuju?

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode113
charm/actions.go:113: return nil, fmt.Errorf("invalid actions definition: action
%q has unknown type %q", name, param.Type)
As above, avoid using : in error descriptions.  A dash here would probably work
just as well.

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode117
charm/actions.go:117: // Skip normal validation for compatibility with pyjuju.
Wait, really?  We care about compatibility with pyjuju?  That seems like a
mistake.

https://codereview.appspot.com/90130044/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/90130044/diff/1/charm/dir.go#newcode57
charm/dir.go:57: if _, ok := err.(*os.PathError); ok {
you should use os.IsNotExist(err) here.  I know the rest of this code casts to
PathError, but the rest of the code is wrong, too :)

https://codereview.appspot.com/90130044/diff/1/cmd/juju/actions.go
File cmd/juju/actions.go (right):

https://codereview.appspot.com/90130044/diff/1/cmd/juju/actions.go#newcode23
cmd/juju/actions.go:23: func (c *ActionsCommand) Info() *cmd.Info {
Please put doc comments on all exported names (and preferably on non-trivial
unexported names, too)

https://codereview.appspot.com/90130044/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/90130044/diff/1/state/api/client.go#newcode127
state/api/client.go:127: // func (c *Client) EnqueueAction(service string,
options []string) error {
Don't leave commented out code in the code unless there's a TODO comment with a
very specific time when the code should be uncommented (which should generally
be in the next month if not less)

https://codereview.appspot.com/90130044/diff/1/state/apiserver/client/do_test.go
File state/apiserver/client/do_test.go (right):

https://codereview.appspot.com/90130044/diff/1/state/apiserver/client/do_test...
state/apiserver/client/do_test.go:6: import ()
What is this file for?

[fwereade, 2014-04-23 07:05:29]

This is a very large CL, hitting an awful lot of the codebase, to the extent
that I'm not sure I've properly reviewed it all -- eg I have registered little
worries about cross-version compatibility at various points.

And... it exposes misleading functionality to the user. It gives them an
"actions" command that merely tantalises them with the possibility of actions,
and we're nowhere *near* ready for that -- there's a *load* of subtle and
complex functionality that needs to be in place before we expose this at the UI
level.

I would strongly suggest that you break this work up roughly as follows -- with
the understanding that each numbered step below potentially involves several
CLs:

1) Make changes to the charm package alone
2) Make the changes to the state server necessary to store this information for
all charms (distinguishing between "no actions" and "might have actions, but was
imported by a version of the state server that didn't know about them", and
fixing the latter at runtime, either during upgrade or on-demand)
3) Work on the state-level infrastructure for storing requested actions and
producing their results, taking into account the fact that charm versions can
change and thus so will the validity of pre-queued actions; come up with a plan
for handling this. (Doesn't need to be complex -- just fail, and explain why, is
fine for v1 -- but needs to exist)
4) Figure out how to get the unit agents themselves to actually run the actions
and store the results, again taking differing charm versions into account -- and
figuring out when we *should* run actions, taking unit lifecycle into account,
etc etc. 
5) actually expose fully tested, working functionality, via the API and finally
the CLI -- I would ideally want "do" and all associated commands to land
together, in the same release if not necessarily in the same branch.

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

https://codereview.appspot.com/90130044/diff/1/charm/actions.go#newcode4
charm/actions.go:4: package charm
This file appears to be largely copy/pasted from config.yaml.

There are indeed clear parallels between the two pieces of functionality, but I
am concerned that:

1) similar parallel code paths will drift out of sync, leading to unpleasant UX
2) config *and* actions are needlessly limited by this string/int/float/bool
nonsense, and we will *definitely* hit exactly the same issues here as we have
with charm config in the past. People *will* need to specify (eg) maps, and
having to yaml-base64-encode them *just to get them into juju* is really
unpleasant.

So, I think there are two viable paths here -- we could design actions nicely,
and write fresh code for them, with a view to eventually migrating config to use
the new code as well; or we could make them look just like config, and reap the
benefits of the code already being written, and dump the externalities on our
users.

I hope it's clear that I favour the former path (although I am always
potentially open to argument); but ISTM that the current approach combines the
worst aspects of both, and isn't going to fly.

https://codereview.appspot.com/90130044/diff/1/cmd/juju/actions.go
File cmd/juju/actions.go (right):

https://codereview.appspot.com/90130044/diff/1/cmd/juju/actions.go#newcode14
cmd/juju/actions.go:14: )
This also appears to be a copy/paste job, to the point of cloning comments that
don't make sense.

https://codereview.appspot.com/90130044/diff/1/cmd/juju/actions.go#newcode59
cmd/juju/actions.go:59: return err
If you're dealing with a state server too old to handle a ServiceActions
request, please inform the user of this fact nicely :).

https://codereview.appspot.com/90130044/diff/1/cmd/juju/actions.go#newcode65
cmd/juju/actions.go:65: "actionspecs": results.ActionsSpec,
What's the rationale for this form of results, beyond "get did it"?

https://codereview.appspot.com/90130044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/90130044/diff/1/state/api/params/params.go#new...
state/api/params/params.go:182: type ServiceActions struct {
Bulk calls, please; and *please* don't name objects after the verbs to which
they are tangentially related.

https://codereview.appspot.com/90130044/diff/1/state/apiserver/client/actions.go
File state/apiserver/client/actions.go (right):

https://codereview.appspot.com/90130044/diff/1/state/apiserver/client/actions...
state/apiserver/client/actions.go:22: return params.ServiceActionsResults{
How is this a "Results"? Looks like a "Result" to me -- we *should* indeed be
accepting multiple requests, and returning multiple results, but we're not.

https://codereview.appspot.com/90130044/diff/1/state/apiserver/client/actions...
state/apiserver/client/actions.go:24: Charm:       charm.Meta().Name,
Charm URL is useful information; charm name is ambiguous. Copy/paste from
get.go?

https://codereview.appspot.com/90130044/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/90130044/diff/1/state/state.go#newcode518
state/state.go:518: ActionsSpec:  ch.ActionsSpec(),
I've been biting by tongue a little, but... "ActionsSpec"? Why not "Actions"?

https://codereview.appspot.com/90130044/diff/1/state/state.go#newcode869
state/state.go:869: {"actionspec", ch.ActionsSpec()},
is it an actionspec or an actionsspec?

https://codereview.appspot.com/90130044/diff/1/store/store_test.go
File store/store_test.go (right):

https://codereview.appspot.com/90130044/diff/1/store/store_test.go#newcode97
store/store_test.go:97: return
&charm.ActionsSpec{make(map[string]charm.ActionSpec)}
If you're adding code to the store to handle actions, I think you need to test
it a little more than this.