Add support for immutable_parameters in ChatOps action aliases

[nicholasamorim]

Hi, this is an attempt to solve #1704.

Closes #1704

There's an use case that consistently comes up where a dev wants a certain action alias but keep certain action parameters static. The current workarounds are to either have some really complex regexes or to create redundant dummy actions.

This proposal solves this by adding action_parameters parameter to the action alias definition. Here's a very simple example:

---
name: "remote_static_shell_cmd"
action_ref: "core.remote"
description: "Execute a command the development machine"
formats:
  - "run {{cmd}} on dev"
immutable_parameters:
  hosts: the-dev-server

Whatever is defined in immutable_parameters will be passed to the action unmodified. This is a very simple PR and I'd love to get suggestions on how to improve the concept and/or the tests.

[CLAassistant]

All committers have signed the CLA.

[nmaludy]

One small feature enhancement would be to evaluate Jinja expressions for the static variables, that way we can load from the datastore.

[nicholasamorim]

Okay, I'll add the jinja evaluation.

[arm4b]

It's overall good feature to have and great addition to existing chatops functionality 👍

Once we agree on naming, would be good to have st2docs update for this new feature in /~https://github.com/StackStorm/st2docs/blob/master/docs/source/chatops/aliases.rst (https://docs.stackstorm.com/chatops/aliases.html)
Please include a CHANGELOG record for this feature as well.

[arm4b]

@namachieli @hd-deman @pixelrebel, you probably want to look at code/implementation too ^^ as someone who participated in #1704.

cc @blag #chatops

[blag]

Thanks for taking the initiative and putting this together! I definitely like the simplicity of this PR.

One minor complaint with this approach is that st2chatops does not display the static parameters with the !help command:

Example alias with static parameters:

---
name: "remote_shell_cmd"
pack: "examples"
action_ref: "core.remote"
description: "Execute a command on a remote host via SSH."
formats:
  - "run {{cmd}} {{hosts}}"
action_parameters:
  - sudo: false

And running the !help command:

>>> !help
... run {{cmd}} on {{hosts=localhost}} - Execute a command on a remote host via SSH.

doesn't display the static parameter or its value.

So it would be nice if this had a companion PR in hubot-stackstorm to include static parameters in the command descriptions/help strings, but that isn't required.

I'm also not a huge fan of how the static parameters silently override any explicitly matched parameters, as this may cause some serious issues for users. Consider this alias:

---
name: "clean_server"
pack: "examples"
action_ref: "core.remote"
description: "Clean up server files."
formats:
  - "cleanup server"
action_parameters:
  - cmd: "rm -rf /var/log/app"
  - hosts: production

To a user, they would see this !help:

>>> !help
... cleanup server - Clean up server files.

Now, if the user runs !cleanup server hosts=testing, their explicit parameter will get silently overridden by the static parameter, so they will end up removing logs from the production server instead of the testing server they intended. I think a better way to handle conflicting parameters (eg: user's explicit parameters conflicting with static parameters) is to abort with an error message. This is a silly example but I think it illustrates my point well enough. @armab Thoughts?

A note: I would expect people to want to specify static parameters for only some alias formats but not all of them. I don't think we should support this, since our RBAC implementation only resolves aliases, not individual alias formats. This would be properly implemented as two separate aliases: one with static parameters and one without. And aliases with different sets of static parameters should be specified as...separate aliases.

[nicholasamorim]

Now, if the user runs !cleanup server hosts=testing, their explicit parameter will get silently overridden by the static parameter, so they will end up removing logs from the production server instead of the testing server they intended. I think a better way to handle conflicting parameters (eg: user's explicit parameters conflicting with static parameters) is to abort with an error message.

I had thought about this and the initial idea was just to get the ball rolling. I wanted to mention this drawback but forgot.

I think that a more suitable approach would be to simply invert the logic: parameters passed on the format overrides the defaults, very much like one would expect. What do you think, @blag?

Of course, this PR will be accompanied with a st2docs PR making this clear - I just want to get the details right before writing the docs.

A note: I would expect people to want to specify static parameters for only some alias formats but not all of them

Not sure if I follow, you mean different static parameters for different formats? If this is what you mean, I absolutely agree: a different set of static parameters is a new action alias.

[arm4b]

One minor complaint with this approach is that st2chatops does not display the static parameters with the !help command:

I think this is good that chatops doesn't display static parameters and hiding defaults. That's one of the simplicity/abstraction advantages behind this feature.
The same could be achieved before by someone creating intermediate action with setting action param values explicitly.
There is also a way to control this how the !help shows command with formats.display.

---

Priority/overriding params is a good catch.
Setting and overriding default parameters in alias could be already achieved by formats.representation jinja/regex hackery as described in #1704 (comment) and #1704 (comment)

For this feature I'd stick to initial #1704 with immutable action params and what @blag suggests, meaning something that's defined in alias can't be changed by the input in chat.

We need to make sure we test these edge cases.

If we go that path, I also think it may be more clear to stick with immutable_parameters naming.
Immutability comes from the fact that param value is explicitly configured by the action alias creator and can't be overridden from chat. Dynamic nature in case of datastore used wouldn't break it.
An addition to that point, it's possible to do the same with both immutable: true + datastore reference in a normal action parameters metadata.

[blag]

I think that a more suitable approach would be to simply invert the logic: parameters passed on the format overrides the defaults, very much like one would expect. What do you think, @blag?

I'm not sure I understand this completely. How is this any different than our (mutable) optional variables with default values?

I agree with @armab - the usefulness of explicitly specifying action parameters in the alias is that they are immutable to ChatOps users, so the action_parameters or immutable_parameters specified in the action alias should override any parameters specified by the ChatOps user, or it should at least cause an error if the ChatOps user tries to override the immutable parameters by specifying them explicitly.

And since immutability is the important part of this feature (IMO), I think using the immutable_parameters key is a little bit more clear than action_parameters.

Not sure if I follow, you mean different static parameters for different formats? If this is what you mean, I absolutely agree: a different set of static parameters is a new action alias.

Sorry if I wasn't clear - that is exactly what I mean.

TL;DR: Not showing the immutable action parameters in the !help response and silently overriding values specified by the user is fine.

We have four different ways we can implement this (the 🛑 and ✅ are my opinion):

	Warn/error on user-provided immutable parameters	Silently override user-provided parameters with their immutable values
Show immutable parameters in !help	1. ✅	2. 🛑
Hide immutable parameters in !help	3. 🛑	4. ✅

1. If we show the immutable parameters in !help, it makes sense to me to error out or at/ least warn the user that whatever values they specify for immutable parameters will be overridden.
2. Showing immutable variables and silently overriding them makes it too easy to write aliases that can cause issues.
3. It doesn't make sense to me to hide the immutable parameters in !help and then warn/error if a user specifies them, because then there is relevant information that isn't provided in !help responses.
4. However, hiding the immutable parameters in !help and silently overriding them if the user does specify them is fine, because if the immutable parameters aren't ever shown to the user in the !help response, ChatOps users shouldn't be specifying them anyway.

Aliases are kind of the ChatOps "interface" to actions, which are kind of the "implementation". And I guess hiding implementation details is fine as long as the interface (and documentation via the !help response) is "good enough".

[nicholasamorim]

I'm not sure I understand this completely. How is this any different than our (mutable) optional variables with default values?

It's not different indeed in the effective outcome but leads to more natural format messages while still leaving room for overriding those defined parameters.

I mean, I'm throwing feedback on the opinions put here and try to conciliate the views and understand where I should lead the PR.

My personal opinion is:

• The immutable parameters cannot be overriden via the chat message/format.
• The immutable parameters, though, can be dynamically loaded/evaluated using Jinja.
• If a user happens to try and override it, I would throw a hard error: you cannot override it and if you wish so, then either:
  a. Create another alias
  b. Remove it from the immutable parameters and have it on the format.
• On naming: action_parameters would be my (personal) preference but immutable_parameters makes sense as well and is indeed clearer when associated with aliases, i.e: "immutable in the context of this alias".

---

Regarding showing static parameters on !help, I don't think we should show it if we're going to throw an error.

[arm4b]

Cool, that sounds like a more or less consensus in minimal implementation 👍 !help additions could be added further to that.

[Kami]

I'm personally +1 for a change like that since I believe it makes it more clear and obvious what is going on and also solves the immutable default parameters use case (which doesn't require user to write a custom wrapper action to handle that and specify immutable default parameter values).

I also believe that at some point we also supported some syntax similar to this one, before the large action alias refactor a couple of years ago.

[nicholasamorim]

• I've renamed the key to be immutable_parameters.
• ValueError is now raised if the user tries to override them.
• Jinja expressions are now evaluated.

This last point, I'm not sure if I did the right thing.I couldn't find some function that would already give me the Datastore context and so on so I basically copied the _get_rendered_vars method from the ChainHolder class (action_chain_runnerpy). Is there a better way?

[nicholasamorim]

I have also opened a st2docs PR regarding this feature: StackStorm/st2docs#933

[blag]

Other than the changelog shenanigans, this LGTM.

[arm4b]

Looks good! Thanks again for the contribution 👍

[blag]

I'll double up on that gratitude. This is something our users have been requesting for awhile.