Skip to content

Fix "st2 action-alias execute" CLI command#5138

Merged
Kami merged 7 commits intomasterfrom
fix_action_alias_cmd
Feb 11, 2021
Merged

Fix "st2 action-alias execute" CLI command#5138
Kami merged 7 commits intomasterfrom
fix_action_alias_cmd

Conversation

@Kami
Copy link
Member

@Kami Kami commented Feb 7, 2021 •
edited
Loading

I tried to use StackStorm after quite a long time and in the first 5 minutes I encountered two exceptions / bugs (like in the good old days - guess I'm always the "lucky" one :D).

There are two bugs in st2 action-alias execute command so it doesn't work.

  1. If user query param is not specified, it will fail with the error below due to API returning an error due to type mismatch (out of the box behavior with st2-docker).
# -------- begin 140132975715104 request ----------
curl -X POST -H  'User-Agent: python-requests/2.23.0' -H  'Accept-Encoding: gzip, deflate' -H  'Accept: */*' -H  'Connection: keep-alive' -H  'X-Auth-Token: XXXX' -H  'content-type: application/json' -H  'Content-Length: 61' --data-binary '{"command": "foo=bar", "user": null, "source_channel": "cli"}' http://st2api:9101/aliasexecution/match_and_execute
# -------- begin 140132975715104 response ----------
{
    "faultstring": "None is not of type 'string'"
}
  1. Result for match_and_execute API operation is not correctly de-serialized in the client so client returns empty object and the code throws an exception. API returns a container object with a list so we need to handle that correctly.
Traceback (most recent call last):
  File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2client/shell.py", line 408, in run
    func(args)
  File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2client/commands/action_alias.py", line 127, in run_and_print
    print("Matching Action-alias: '%s'" % execution.actionalias['ref'])
AttributeError: 'ActionAliasExecution' object has no attribute 'actionalias'

This PR fixes both issues.

TODO

  • Add tests (no idea when I'll get a chance to do it, but for sure we need tests because I assume there are not otherwise they would have caught this bug)

@Kami
Fix broken st2 action-alias execute command.
e2e624e 
There were two bugs:

1. If user it's not specified it will return an error
2. It didn't correctly de-serialize the result so it failed
@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Feb 7, 2021
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Feb 8, 2021
@Kami
Copy link
Member Author

Kami commented Feb 8, 2021

Adding direct st2client tests will be a bit of a pain so to begin with, I added in-direct test for the API endpoint itself (5df4e8e - similar to the ones in #5141) which verify that API endpoint indeed returns a list with specific key present.

Kami added 4 commits February 8, 2021 16:20
@Kami
Update fixture.
ec03d14
@Kami
Add a direct st2 CLI test case for st2 alias-execution execute command.
19e2206
@Kami
Update base CLI test case class where we capture and don't display
75a114c 
captured stdout and stderr to display captured stdout + stderr on test
failure.

This should make troubleshooting those tests much easier and faster.
@Kami
Fix lint.
f340925
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Feb 8, 2021
@Kami
Copy link
Member Author

Kami commented Feb 8, 2021

OK, I couldn't stand not having any tests for such a basic functionality so I also added direct tests in 19e2206 (still not ideal since response is mocked, but that's the pattern we use for CLI tests :/).

I also pushed a small related improvements which such make troubleshooting CLI tests easier - 75a114c.

cognifloyd
cognifloyd approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. I didn't know about st2 action-alias execute. Awesome.
Fixes and tests lgtm

@Kami
Copy link
Member Author

Kami commented Feb 8, 2021

I'm also working on related functionality - execute-and-format which will run the execution which matches an alias and also format and print the result - this will make testing ChatOps easier and faster since you will be able to do it quickly on command line and no need to do it via chat.

@Kami Kami added this to the 3.4.0 milestone Feb 11, 2021
@Kami Kami merged commit 1ad0063 into master Feb 11, 2021
@Kami Kami deleted the fix_action_alias_cmd branch February 11, 2021 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cognifloyd cognifloyd cognifloyd approved these changes

+1 more reviewer

@arm4b arm4b arm4b approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:st2client size/L PR that changes 100-499 lines. Requires some effort to review.

Projects

None yet

Milestone

3.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Kami @arm4b @cognifloyd