Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Feb 7, 2021

This pull request fixes a bug in action alias array parameter value casting when executing an alias.

The code would incorrectly assume value is always a string and try to cast it to an array (the value may very well be a string when executing command from chat, but even then is not always true).

This would not work if value is already a list (e.g. if parameter contains a default value or a default value is specified using immutable_parameters in action alias definition).

Background, Context

I encountered this bug when trying to define a default value for a parameter in action alias which is a list.

Here is action alias definition in question:

---
name: "test"
pack: "scalyr"
action_ref: "scalyr.timeseries_queries"
description: "Get server temps."
formats:
    - "temps"
result:
  format: |
    CPUs: 

    Core 0: {{ execution.result.result.results[0]["values"][0]|int }} C
    Core 1: {{ execution.result.result.results[1]["values"][0]|int }} C 
immutable_parameters:
  queries:
    -
      filter: "$source='tsdb' $serverHost='foo' metric = 'output' and instance = 'core_0_temp_celsius'"
      function: "mean(value)"
      startTime: "10 minutes"
      buckets: 1
    -
      filter: "$source='tsdb' $serverHost='foo' metric = 'output' and instance = 'core_1_temp_celsius'"
      function: "mean(value)"
      startTime: "10 minutes"
      buckets: 1
 ...

TODO

  • Tests

Code would incorrectly try to cast a value to a list even if it's
already a list which means it would break in scenarios when value is
already a list (e.g. when parameter contains a default value or when
immutable_parameters are used with an action alias).
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Feb 7, 2021
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. 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

Added tests.

It was quite painful. My old development environment was not working and getting a new one up also took much longer then it should (couldn't find any good up to date docs so I created one myself from scratch).

@Kami
Copy link
Member Author

Kami commented Feb 8, 2021

On a related note - I assume the whole str to list casting code was added as a quick hack / workaround at some point in the past to make it easier to specify lists in a chat.

It's not ideal or complete since it doesn't handle more complex scenarios (e.g. lists of objects with key1=value,key2=value notation), but my main and only objective in this PR is to fix it so it works correctly for "already a list" scenario.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

👍

@pull-request-size pull-request-size bot removed the size/S PR that changes 10-29 lines. Very easy to review. label Feb 11, 2021
@Kami Kami merged commit deb3df7 into master Feb 12, 2021
@Kami Kami deleted the fix_action_alias_parameter_cast_bug branch February 12, 2021 18:30
@Kami Kami added this to the 3.4.0 milestone Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action aliases chatops size/L PR that changes 100-499 lines. Requires some effort to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants