Refactor chatops to process input alias matching server side #2895
Conversation
…rsions of ubuntu, so this command would fail, normally skip on a shell but causes problems in docker buildfile
|
@estee-tew I don't have access to the url you posted. Would you be able to share the comments here? |
|
This is amazing and long due; one of our chatops roadmap items, actually. The regex is already built here on the back-end: https://github.com/StackStorm/st2/blob/c013fc72111ae9141b5c474f32ec85dae3660498/st2common/st2common/models/utils/action_alias_utils.py |
|
@nzlosh seems like a bug in @estee-tew. The correct URL is https://stackstorm-community.slack.com/archives/community/p1473660729000786. |
| LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ChatopsController(resource.ContentPackResourceController): |
There was a problem hiding this comment.
Is it possible to keep those methods / operations on the alias controller?
The reason is that right now StackStorm concept is called action alias and the code doesn't use "ChatOps" term anywhere else (afaik) so it might be a bit confusing to call it ChatOps here...
|
Nice, indeed! That's been our goal for a while now, but we never got to it. Moving all the parsing and matching to the server will allow user to make clients a lot thinner / dumb which should make it easier to develop clients for other protocols and also reduce the client maintenance burden. Well, at least the basic implementation. For performance reasons, in some scenarios (a lot of messages per second and a lot of aliases) it might make sense to move some pre-parsing / matching to the client. |
…ing an entire mock class
|
done. |
| @@ -58,6 +73,37 @@ def get_all(self, **kwargs): | |||
| def get_one(self, ref_or_id): | |||
| return super(ActionAliasController, self)._get_one(ref_or_id) | |||
|
|
|||
| @jsexpose(arg_types=[str], status_code=http_client.ACCEPTED) | |||
| def match(self, command, **kwargs): | |||
There was a problem hiding this comment.
Can user also pass other arguments to this function? If not, we should probably get rid of **kwargs.
There was a problem hiding this comment.
they could for the purposes of reducing the criteria of the underlying get_all call, which is where they are passed. So filtering by pack for example.
|
|
||
| action_exec_mgr = self.app.client.managers['ActionAliasExecution'] | ||
|
|
||
| execution = action_exec_mgr.create(execution, **kwargs) |
| def match(self, command, **kwargs): | ||
| url = '/%s/match' % self.resource.get_url_path_name() | ||
| query_str = urlencode({'command': command}) | ||
| response = self.client.post_raw(url, query_str, **kwargs) |
There was a problem hiding this comment.
Hm, this is sent as query string, right?
If so, requests library should encode it correctly as long as the requests.post method is called with the params=params argument.
There was a problem hiding this comment.
It wasn't 100% clear on the docs (I can't stand that documentation!). I know this works now :-)
|
Nice, thanks, LGTM! I will check it out and play a bit with it locally and if everything looks good merge it into master. |
| @@ -48,6 +52,17 @@ class ActionAliasController(resource.ContentPackResourceController): | |||
| 'sort': ['pack', 'name'] | |||
| } | |||
|
|
|||
| _custom_actions = { | |||
There was a problem hiding this comment.
Hm, it appears _custom_actions is only supported in newer versions of pecan because I get 404 in execute :/
We still use 0.7.0 because of backward incompatible changes in newer versions which make it hassle to upgrade.
There was a problem hiding this comment.
Actually, it looks like old versions support it as well. Must be something else - looking into it.
| @@ -48,6 +52,17 @@ class ActionAliasController(resource.ContentPackResourceController): | |||
| 'sort': ['pack', 'name'] | |||
| } | |||
|
|
|||
| _custom_actions = { | |||
There was a problem hiding this comment.
Actually, it's a CLI issue - CLI doesn't correctly send query param to the method...
(virtualenv)vagrant@vagrant-ubuntu-trusty-64:/data/stanley$ python ./st2client/st2client/shell.py action-alias match "foo bar foo"
ERROR: 404 Client Error: Not Found
MESSAGE: The resource could not be found. for url: http://127.0.0.1:9101/v1/actionalias/mmatch() takes exactly 2 arguments (1 given)I will make urlencode and requests changes mentioned below, I believe this should fix it.
There was a problem hiding this comment.
Actually, since this is a post we should probably send data as body and not via query string.
This way it will be consistent with other models. Yes, it's a bit "heavy" and we need a wrapper API model, but we already do that in other places.
I will make that change, hope you don't mind :)
There was a problem hiding this comment.
There we go - 4afaa54
It works now, woo!
(virtualenv)vagrant@vagrant-ubuntu-trusty-64:/data/stanley$ python ./st2client/st2client/shell.py action-alias match "run uptime on localhost"
2016-09-15 12:37:32,670 WARNING - Auth API server is not available, skipping authentication.
+------------------+---------------------------------------------+
| name | description |
+------------------+---------------------------------------------+
| remote_shell_cmd | Execute a command on a remote host via SSH. |
+------------------+---------------------------------------------+
(virtualenv)vagrant@vagrant-ubuntu-trusty-64:/data/stanley$ python ./st2client/st2client/shell.py action-alias execute "run uptime on localhost"
2016-09-15 12:37:34,473 WARNING - Auth API server is not available, skipping authentication.
Started execution, id '57da960e0640fd107ab26409'
Hold tight while I run command: *uptime* on hosts *localhost*Minor thing - what do other things, I think execute could be blocking by default (similar as st2 run command) and optionally user can specify --async flag to make it non-blocking. WDYT?
|
|
||
| @resource.add_auth_token_to_kwargs_from_cli | ||
| def run(self, args, **kwargs): | ||
| action_alias, representation = self.manager.match(args.command_text, **kwargs) |
There was a problem hiding this comment.
I'm fine with having this command for now, but for true server side matching and execution which would allow us to make clients event dumber I think we should also move this to the server side (e.g. "match and execute" endpoint or similar).
This is not a blocker right now though. We can leave it here and introduce this new API endpoint in the future. The CLI command is forward compatible anyway (once the feature is available on the server, we just need to change the code here).
This would also save client / user one API call which is actually good in this case, especially if you send every single line posted to chat to the API.
There was a problem hiding this comment.
You should only send the lines that are addressed to Hubot, so that's not a huge concern, but match-and-execute would be pretty good regardless.
There was a problem hiding this comment.
the patterns got a bit wonky crossing resource types between controllers. I'll have a think about it.
|
Merged into master via #2904. Thanks again for great work and a PR! 👍 And in the future if you find yourself with nothing to do (yeah, I know, that never happens :D) it would be great if you could also look into this #2895 (comment). |
|
❤️ ❤️ ❤️ |
This PR is proposing to allow clients to submit text captured via another method (e.g. Python bot or direct input) into the StackStorm chatops system. To cater for this, StackStorm needs to be changed to:
Strategy:
st2common.utilfor the alias matching/regex logicrestdlib compared with the existing hubot code.st2clientproject, e.g.st2 alias match "honk-honk"Functionality in future PRs:
st2streamAPI to support server side filtering (not required but preferred).This is a collaboration with @nzlosh