Integration Tests for Custom Jinja Filters in Mistral #3565
Conversation
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
…t2 into mistral-jinja-filter-itests
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Mierdin
changed the title
Signed-off-by: Matt Oswalt <[email protected]>
There was a problem hiding this comment.
Overall looks good. Need to rework a few test workflows so it's more readable in the tests. Need to rename folder and workflows. I prefer we use the term "function" instead of "filter" in general. The term "filter" is specific to Jinja. In Mistral, the equivalent is called "function". I will be more strict in the use of this term in the patch at st2mistral.
| action: std.noop | ||
| publish: | ||
| result_jinja: '[{"title": "{{ _.input_str|json_escape }}"}]' | ||
| result_yaql: '[{"title": "<% json_escape($.input_str) %>"}]' |
There was a problem hiding this comment.
Fix carriage return here?
There was a problem hiding this comment.
Fixed in 13a2d0d (as well as a few other missing newlines as well as some extra ones)
| --- | ||
| description: Example for using the custom filter "json_escape" | ||
| enabled: true | ||
| entry_point: workflows/customfilters/mistral-json_escape.yaml |
There was a problem hiding this comment.
Can we make the naming of the workflow either with all dashes or all underscores but not mixing them in the name?
There was a problem hiding this comment.
Can we rename the customfilters folder to tests? And explicitly name mistral-json_escape.yaml to something like mistral-test-filter-json-escape.yaml? So end up entry_point: workflows/tests/mistral-test-filter-json-escape.yaml. I may put all other test related workflows into this folder.
| task1: | ||
| action: std.noop | ||
| publish: | ||
| result_jinja: '{{ _.input_str | regex_match(_.regex_pattern)}}' |
There was a problem hiding this comment.
Why do you need to pipe _.input_str? You can't pass it directly into regex_match?
There was a problem hiding this comment.
Just going with the style that's commonly seen in workflows. No technical reason one way or another
There was a problem hiding this comment.
Then please be consistent between the YAQL and Jinja expression and use regex_match(input_str, pattern), unless you're testing whether values can be piped into the function (which may not be a bad idea but do it separately).
There was a problem hiding this comment.
Okay, will change the Jinja usage to look like YAQL
There was a problem hiding this comment.
So I was wrong, there is a technical reason to use this notation (at least the way the underlying function is implemented right now of course). Using these functions in Jinja in the same way that they're used in YAQL will pass context as the first argument. So when I made this change, I got a lot of these types of errors:
JinjaEvaluationException: Can not evaluate Jinja expression [expression= version_strip_patch(_.version) , error=version_strip_patch() takes exactly 1 argument (2 given), data={}]
None of these filters actually require direct access to context, only the value passed in, so while it would be possible to modify the underlying implementation to access context, I'm leaning towards leaving things the way they are because 1) least-privilege principles and 2) the filter format (|) is the generally expected way this is done. Thoughts?
There was a problem hiding this comment.
Calling the filter as normal function call is the minimal requirement IMO. Being able to use the filter from piping is added bonus. Because right now, it's an exception and we will have to document and explain to people the only way to use the function in Jinja is using this format. I also need to know exactly why it can't be used like normal function calls.
There was a problem hiding this comment.
Sure thing. So, I threw together a quick filter to simply print out the positional args that are being passed into it:
def test_filter(*args):
return str(args)
Then put together a workflow for testing (ignore the regex naming, I just used that as a testbed):
version: '2.0'
examples.mistral-customfilters-regex_match:
description: A workflow for testing regex_match custom filter in mistral
type: direct
input:
- input_str
- regex_pattern
output:
result_jinja: <% $.result_jinja %>
result_yaql: <% $.result_yaql %>
tasks:
task1:
action: std.noop
publish:
result_jinja: '{{ _.input_str | test_filter }}'
result_yaql: '<% test_filter($.input_str) %>'
When we use the pipe (in Jinja's eyes, filter usage), the first arg is simply the string value being passed in:
(virtualenv)vagrant@st2dev:~/st2$ segl
id: 597abe9232ed355b4e873d3e
action.ref: examples.mistral-customfilters-regex_match
parameters:
input_str: Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.
regex_pattern: ([0-9]{3} \w+ (?:Ave|St|Dr))
status: succeeded (5s elapsed)
start_timestamp: 2017-07-28T04:33:22.578088Z
end_timestamp: 2017-07-28T04:33:27.492128Z
result:
extra:
state: SUCCESS
state_info: null
result_jinja: (u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.',)
result_yaql: (u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.',)
tasks:
- created_at: '2017-07-28 04:33:23'
id: 213d3c60-b380-49e3-a725-6b76f6f84c0c
input: null
name: task1
published:
result_jinja: (u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.',)
result_yaql: (u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.',)
result: null
state: SUCCESS
state_info: null
updated_at: '2017-07-28 04:33:23'
workflow_execution_id: 37d5f7b6-5af2-445a-b5fe-d72cd490c1c3
workflow_name: examples.mistral-customfilters-regex_match
However, if we change the Jinja snippet to behave like a function instead of a filter....
'{{ test_filter(_.input_str) }}'
...then the first parameter will be the context, followed by the string value:
(virtualenv)vagrant@st2dev:~/st2$ segl
id: 597abfc332ed356072d3b2f1
action.ref: examples.mistral-customfilters-regex_match
parameters:
input_str: Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.
regex_pattern: ([0-9]{3} \w+ (?:Ave|St|Dr))
status: succeeded (5s elapsed)
start_timestamp: 2017-07-28T04:38:27.931985Z
end_timestamp: 2017-07-28T04:38:32.919549Z
result:
extra:
state: SUCCESS
state_info: null
result_jinja: ({}, u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.')
result_yaql: (u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.',)
tasks:
- created_at: '2017-07-28 04:38:28'
id: bd3568b5-ed16-494e-8ca2-c76cb76227db
input: null
name: task1
published:
result_jinja: ({}, u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.')
result_yaql: (u'Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave.',)
result: null
state: SUCCESS
state_info: null
updated_at: '2017-07-28 04:38:28'
workflow_execution_id: 6dc96bc0-f8e0-47d5-bf74-60c313715e12
workflow_name: examples.mistral-customfilters-regex_match
The reason this is happening is that when used as a function, Jinja2 injects it's own Context object as the very first parameter. What complicates things is that Mistral is ALSO passing in its own context parameter (the empty dict we see in the previous example) all the time, but the use of partial in https://github.com/StackStorm/mistral/blob/master/mistral/utils/expression_utils.py#L92 means we don't see this. However, if we use this as a function, the Jinja2 Context object becomes args[0], and Mistral's context becomes args[1], leaving value at args[2].
So, TLDR the reason the examples (see the ActionChains in examples pack, for instance - their back-end functions are implemented in the same way) use the pipe notation is because all of the backend functions have the string value as the first parameter. It would seem that the current implementation requires the pipe notation.
Now, this obviously doesn't HAVE to be the case - I could, for instance, create a decorator to handle these variations seamlessly (and/or use one of the decorators at http://jinja.pocoo.org/docs/2.9/api/#utilities), but whatever we do for Jinja in Mistral should get copied over to st2 so we don't undo the work to sync the two "islands".
Hopefully that makes sense. I'll take a stab at trying to make the underlying functions care less about how they're called, keeping in mind that it will have to also be repeated in st2.
There was a problem hiding this comment.
It also might be worth exploring the idea of modifying Mistral's use of partial to specify a keyword arg instead of positional, to be more precise, but I haven't thought that one through yet.
There was a problem hiding this comment.
Alright, I have something working in StackStorm/st2mistral@cb87499. I've tested both forms and it seems to work well.
Let me know what you think about the approach and I'll add the decorator to the rest of the filters in that PR and then add a new dedicated test in this PR for testing both forms on a few of the filters.
| task2: | ||
| action: std.noop | ||
| publish: | ||
| result_jinja: '[{"title": "{{ _.input_str|json_escape }}"}]' |
There was a problem hiding this comment.
Why do you need to pipe _.input_str? You can't pass it directly into json_escape?
|
|
||
| REGEX_SEARCH_STR = "Your address is 567 Elsewhere Dr. My address is 123 Somewhere Ave." | ||
| REGEX_SEARCH_STR_2 = "567 Elsewhere Dr is your address. My address is 123 Somewhere Ave." | ||
| REGEX_SEARCH_STR_3 = "No address to be found here! Well, maybe 127.0.0.1" |
There was a problem hiding this comment.
Why not use an array? REGEX_SEARCH_STRINGS = ["Your address...", "567 Elsewhere...", "No address..."]
There was a problem hiding this comment.
I could do that, just did it this way since I wasn't planning on iterating over them, just referring to specific ones. Could put in list and refer by index
|
|
||
| def test_regex_substring(self): | ||
| execution = self._execute_workflow('examples.mistral-customfilters-regex_substring', | ||
| parameters={"input_str": REGEX_SEARCH_STR}) |
There was a problem hiding this comment.
Can you move the regex pattern from vars to input parameters here? It was hard to follow at first why the output of the workflow returns "567 Elsewhere Dr"?
There was a problem hiding this comment.
Sounds good - did that for this and all other regex tests in b3fcf21
| inputs = {'input_str': 'foo'} | ||
| execution = self._execute_workflow( | ||
| 'examples.mistral-customfilters-use_none', parameters=inputs | ||
| ) |
There was a problem hiding this comment.
This workflow is a bit confusing to me. How about split this into two separate tests where the input parameter to the workflow is optional and default to null? The first test passes the "foo" string. The second tests don't pass any inputs and let it default to null.
There was a problem hiding this comment.
Ah - good catch, I meant to circle back and do this. My first thought was to literally pass None as a parameter but there's no JSON schema type to support it, so I went with this approach so I could just create a new variable in the workflow that's null. I realized after this that an empty string would suffice just as well (duh), just forgot to go back and simplify. Will do that next.
There was a problem hiding this comment.
Oops, now I remember. The underlying implementation actually explicitly checks for None (I mistakenly thought it was a simple if not value: but it is is literally if value is None) https://github.com/StackStorm/st2mistral/pull/30/files#diff-d0cfa0e285114587d5c640c4937d3d77R26
So to keep from modifying the underlying function (which would technically be a breaking change - or at least a deviation from the sister function in st2), I need to be able to supply null values, and I can't do that if the parameter type is string. Thoughts either way (as in changing this workflow to fit the existing function vs changing the way the function works)?
|
|
||
| def test_to_complex(self): | ||
| execution = self._execute_workflow( | ||
| 'examples.mistral-customfilters-to_complex' |
There was a problem hiding this comment.
I prefer the complex object to be pass in as input parameters instead of using examples.object_return. It's more readable for the test.
There was a problem hiding this comment.
I agree - examples.object_return was available and useful, but it's better to be consistent with the way the rest of the tests are written.
| def test_to_json_string(self): | ||
| execution = self._execute_workflow( | ||
| 'examples.mistral-customfilters-to_json_string' | ||
| ) |
There was a problem hiding this comment.
I prefer the json to be pass in as input parameters instead of using examples.object_return. It's more readable for the test.
|
|
||
| def test_to_yaml_string(self): | ||
| execution = self._execute_workflow( | ||
| 'examples.mistral-customfilters-to_yaml_string' |
There was a problem hiding this comment.
I prefer the json to be pass in as input parameters instead of using examples.object_return. It's more readable for the test.
|
Please see why the st2/packages job is failing of this PR. |
|
@m4dcoder I believe packages was failing because of the issue @Kami fixed in StackStorm/st2-packages#475. I'll push more commits here anyways - if it's still failing I'll tshoot it then |
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
…t file Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
This follows changes being made on the st2mistral side Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
| description: Example for using the custom filter "json_escape" | ||
| enabled: true | ||
| entry_point: workflows/tests/mistral-json-escape.yaml | ||
| name: mistral-customfilters-json-escape |
There was a problem hiding this comment.
Please make naming consistent. Usually, I name the workflow and the file name the same. For example, name the workflow mistral-test-func-json-escape, the meta file should be mistral-test-func-json-escape.yaml and the entry point should be workflows/tests/mistral-test-func-json-escape.yaml.
Signed-off-by: Matt Oswalt <[email protected]>
This PR contains integration tests for custom Jinja filters made available in Mistral via StackStorm/st2mistral#30. Also tests for changes to st2kv in same PR (addition of
decryptparameter). Note that these tests will not pass until that PR is merged (or if you check it out locally).All tests ensure compatibility with both YAQL and Jinja rendering within Mistral.
Test Output
These tests haven't been running in CI lately due to some other issues, so here's some output showing the tests passing: