action-set: a jujuc command for Actions to send values back to state #415
Conversation
| @@ -38,7 +38,14 @@ func (dummyHookContext) ConfigSettings() (charm.Settings, error) { | |||
| func (dummyHookContext) ActionParams() map[string]interface{} { | |||
| return nil | |||
| } | |||
|
|
|||
| func (dummyHookContext) ActionResults() interface{} { | |||
There was a problem hiding this comment.
map[string]interface{} perhaps?
There was a problem hiding this comment.
action-set "ipso facto"?
|
I've updated this with some of the requests @fwereade made:
However, I wasn't able to remove the methods from |
|
LGTM overall. As discussed, we probably want the addition to Context to be just a struct actionResult with a sub map[string]interface{} for the results set, other keys for other result metadata such as the error, process return code and so on. |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1350911', 'fixes-1347715'] |
| } | ||
|
|
||
| // ActionSetFailed sets an error value for the Action and makes it unable to | ||
| // add more values unless SetFailed(nil) is passed. |
There was a problem hiding this comment.
Surely once it's failed, it's failed?
But I think there's still a use case for setting values -- comprehensive debug information might actually be kinda super-awesome.
There was a problem hiding this comment.
It seems to me like a fail state could be something reversible; for example, if a web API is busy, the Action might poll it until it receives a good response, at which point the error could be reset, or for 5 minutes. If the 5 minutes are up before the failure is cleared, the final result will be a fail; otherwise, it seems like it could be cleared.
There was a problem hiding this comment.
Then it shouldn't tell us it's failed -- it might happily block for a bit, but it hasn't failed while it's still retrying. When it's run out of attempts, sure, it fails: "I tried 5 times, still didn't work, sorry".
| c.Assert(err, gc.IsNil) | ||
| hctx := s.getHookContext(c, uuid.String(), -1, "", noProxies) | ||
| hctx.SetActionData(names.ActionTag{}, map[string]interface{}(nil)) | ||
| com, err := jujuc.NewCommand(hctx, "action-set") |
There was a problem hiding this comment.
This is not testing UpdateActionResults, it's testing action-set. (which might call UAR under the hood, or it might not -- who knows? ;p)
|
Few more comments -- fwiw, breaking the changes to Context out and doing it separately from action-set itself would make it easier to review and possibly quicker to merge ;). |
binary132
force-pushed
the
action-set
branch
from
593479c
to
38321ce
Compare
binary132
force-pushed
the
action-set
branch
from
38321ce
to
bfdaa56
Compare
binary132
force-pushed
the
action-set
branch
4 times, most recently
from
9fd2bb9
to
11d6293
Compare
action-set permits action hooks to define an arbitrary map of params which will be returned to the state server at the end of the Action.
Addressed fwereade's concerns for PR 415.
This commit moves the Action results update method into the Context implementation, so that client code is not changing the results map on its own.
Addressed: - need to keep Action results map segregated from jujuc - testing Context interface mock to support this change - removed old Context methods - changed actionResults inner "error" type to string rather than error
action-set now has a test in worker/uniter/uniter. This test demonstrates how action-set overwrites existing values in the results map.
This commit revises action-set behavior to interact correctly with new ActionFinish logic on worker uniter.
binary132
force-pushed
the
action-set
branch
from
11d6293
to
70044c3
Compare
| []string{"map1", "key", "val"}, | ||
| []string{"map1", "val"}, | ||
| }, | ||
| }} |
There was a problem hiding this comment.
Missing a failing test with an invalid key. And one for mykey=. Here I'm not sure if returning an empty values string is specified.
|
Mostly fine, only missing tests of error situations. |
This commit wraps up the changes to `action-set` now that ActionFinish has landed in core.
binary132
force-pushed
the
action-set
branch
from
70044c3
to
1fca7eb
Compare
|
|
Action set action-set permits action hooks to define an arbitrary map of params which will be returned to the state server at the end of the Action. This is a reopen of #415. 415 failed to land due to too many pages of comments.
action-set permits action hooks to define an arbitrary map of params
which will be returned to the state server at the end of the Action.