Retrieve live params first, include template-driven defaults

[Mierdin]

Problem

#3820 reported an issue that parameter defaults that use Jinja templates that return something other than string, simply don't work.

This is caused by two things:

• The code in the API controller that handles execution scheduling currently makes a call to action_service.create_request before the live parameters are retrieved, which is where any templates are rendered into a result value. So if a parameter is an integer but the default value is still a template (string) - even if that template is expected to return an int, naturally this will cause problems because it hasn't even rendered yet.
• The call to param_utils.render_live_params and the function calls within will render the templates, but will not actually use the resulting value, as the default values for parameters are not seen as "live". So even though there's a rendered value available to use, it's not placed in the params list because it wasn't passed in explicitly by the user, and the same problem occurs, as the unrendered template value is used (string).

Proposed Solution

To fix the behavior observed in #3820, I propose the following changes:

• A simple re-order will address the first problem. Wait to call action_service.request until the live params are retrieved.
• After the existing behavior around live parameters, I suggest that we detect if a parameter's default is a template, and the resulting value of that template is actually being used (i.e. isn't being overridden by the user). In effect, we treat the resulting value as a live parameter (we're literally adding it to the same dict). If a default value is rendered and used, it shows up as if the user had passed that value in.

I think treating rendered default values as "live" parameters makes sense. Inherently, since we're using a template, it could easily be user-provided, even if the user provides it via st2kv. It's a bit of a paradigm change but I think it makes sense, personally.

One potential consequence to note is that the parameter schema will now have to validate against what the template's value will be, but IMO this should already be happening anyways. I would expect that if I'm getting a value from a template, that the datatype should be predictable, and if it's different for any reason, I would expect a validation error at runtime. We've already inherently moved this problem to runtime by using templates in params like this anyways - this just makes the resulting behavior more intuitive.

Demo

I threw together a quick action to demonstrate the new logic:

---
description: Run a local linux command
enabled: true
runner_type: local-shell-cmd
name: nmaludy-test
pack: testpack
parameters:
  kv_test:
    type: integer
    default: "{{ st2kv.system.nick_test | int }}"

I'll create the referenced key in the datastore and execute my action:

~$ st2 key set -s system nick_test 31415
+------------------+-----------+
| Property         | Value     |
+------------------+-----------+
| name             | nick_test |
| value            | 31415     |
| expire_timestamp |           |
+------------------+-----------+
~$ st2 run testpack.nmaludy-test cmd=date
.
id: 5a2667e632ed353390a8bc7a
status: succeeded
parameters:
  cmd: date
  kv_test: 31415
result:
  failed: false
  return_code: 0
  stderr: ''
  stdout: Tue Dec  5 09:33:27 UTC 2017
  succeeded: true

Note that kv_test appears in the resulting live parameters as if I had passed it in explicitly. While it's true, I didn't do this at the CLI, I believe the intention is the same - the default value is based on dynamic retrieval from the datastore via the template.

Note also that I can still override this with my own value - a "true" live param:

~$ st2 run testpack.nmaludy-test cmd=date kv_test=12345
.
id: 5a2667f832ed353390a8bc7d
status: succeeded
parameters:
  cmd: date
  kv_test: 12345
result:
  failed: false
  return_code: 0
  stderr: ''
  stdout: Tue Dec  5 09:33:44 UTC 2017
  succeeded: true

Closes #3820

[nmaludy]

+100

[Kami]

Is it possible to add some test cases just for this function for various scenarios which are possible here?

[Kami]

I approved it, but it would be great if we can add some more test cases for that, because just this for loop shows that at least 4 different scenarios are possible just in this code block.

[Mierdin]

Thanks for the approve, I'll add more test cases for this before merging.

[Kami]

Overall, from a high level perspective the change and approach looks good to me.

We had some discussion about this in the past (dynamic default parameter values) and I was strongly against it then.

My main reason was that is very hard to reason about - different values are available at different point in lifecycle of an execution (before any rendering is happen, after rendering happens, when execution is created, etc).

I'm still personally not a big fan of it (static > dynamic - a lot easier to reason about, develop, troubleshoot, see what is going on, etc.), but apparently other users see value in it and in a sense it's opt-in so I'm fine with it.

[Mierdin]

I agree with you actually. I far prefer predictable values than moving the problem to runtime. One of the reasons I go write Go code when Python starts grinding my gears again :)

In any case, my point was that IMO we crossed this boundary when we allowed templates in default values for actions in the first place. This PR just finishes that work by making the type cast portion work the way you'd expect.