Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fix for output_value[output_key] is not iterable #5309

Merged
merged 9 commits into from
Oct 7, 2021

Conversation

guzzijones
Copy link
Contributor

if output_value[output_key] is not iterable and instead a boolean I receive the following error code from the st2api.
IE:

output_value: {'stdout': '', 'stderr': '', 'exit_code': 0, 'result': True}
spec: {'status': 'bool'}
key: output
output_key: result

ERROR:

ERROR: 500 Server Error: Internal Server Error
MESSAGE: Internal Server Error for url: https://stackstorm-vf.ifp.lmco.com/api/executions/60f94708c3a6d1a338dc0ad7/children/?depth=-1

if output_value[output_key] is not iterable and instead a boolean I receive the following error code from the st2api.
IE:
```
output_value: {'stdout': '', 'stderr': '', 'exit_code': 0, 'result': True}
spec: {'status': 'bool'}
key: output
output_key: result
```
ERROR:
```
ERROR: 500 Server Error: Internal Server Error
MESSAGE: Internal Server Error for url: https://stackstorm-vf.ifp.lmco.com/api/executions/60f94708c3a6d1a338dc0ad7/children/?depth=-1
```
@amanda11 amanda11 added this to To Do in StackStorm v3.6.0 via automation Jul 22, 2021
@amanda11 amanda11 added this to the 3.6.0 milestone Jul 22, 2021
@cognifloyd
Copy link
Member

This would probably also apply to int, float, and other types that are not Collections.

@guzzijones
Copy link
Contributor Author

guzzijones commented Jul 22, 2021 via email

guzzijones and others added 3 commits July 22, 2021 12:59
switch to using Collection instead of bool check

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
add import collections
whitespace fix
StackStorm v3.6.0 automation moved this from To Do to In Progress Aug 3, 2021
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

Can you add a unit test that reproduce this issue so we know this patch is going to fix?

@cognifloyd
Copy link
Member

Hmm. So there were two issues: 1) the bug you found. 2) your spec is not well formed.

Instead of this spec:

status: bool

You need this jsonschema spec:

status:
  type: bool

I'm looking at how to adjust the tests now.

@cognifloyd
Copy link
Member

K. I added #5319 based on this.
It turns out that output_schema_validation is actually quite limited.
Your output has to be an object and all properties in that object have to be objects as well. Then, you can use secret masking.
I left a FIXME comment about adjusting the schema to allow output_schema to describe more kinds of output (and then allow secret masking on those additional types as well).

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021

CLA assistant check
All committers have signed the CLA.

@guzzijones
Copy link
Contributor Author

Where did we leave this?

@cognifloyd
Copy link
Member

cognifloyd commented Oct 2, 2021

I have too many irons in the fire. I can't work on this for now. I'll come back to it when I can shift focus.

@guzzijones guzzijones modified the milestones: 3.6.0, 3.7.0 Oct 5, 2021
@guzzijones
Copy link
Contributor Author

@guzzijones add changelog entry.

@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Oct 5, 2021
@pull-request-size pull-request-size bot removed the size/XS PR that changes 0-9 lines. Quick fix/merge. label Oct 5, 2021
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Oct 5, 2021
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.

OK. I added a changelog entry that mentions that output_schema handling will change in a future release.
I also adjusted bailing out early if the output value is not an object or if a spec in output_schema is mal-formed.

I think this is ready to merge.

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

Conditional approve. The proper fix will be followed up in v3.7.

@cognifloyd
Copy link
Member

Yes. I will rework/expand #5319 before 3.7.0.

@cognifloyd cognifloyd merged commit 924da21 into StackStorm:master Oct 7, 2021
StackStorm v3.6.0 automation moved this from In Progress to Done Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix output schema size/S PR that changes 10-29 lines. Very easy to review.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants