Add option to specify when to emit st2.generic.actiontrigger #4315
Conversation
|
+100 !!! I was thinking the other day it would be great if there were more triggers in the system for "internal" events like this! |
| LOG.debug('Action trigger is disabled, skipping trigger dispatch...') | ||
| return | ||
|
|
||
| target_statuses = cfg.CONF.action_sensor.emit_when |
There was a problem hiding this comment.
Would be a good idea to log a message under DEBUG if trigger is not emitted due to the status not being in ``emit_when list.
There was a problem hiding this comment.
Will do. Thanks for the comments!
| @@ -388,6 +388,10 @@ def register_opts(ignore_errors=False): | |||
| cfg.BoolOpt( | |||
| 'enable', default=True, | |||
| help='Whether to enable or disable the ability to post a trigger on action.'), | |||
| cfg.ListOpt( | |||
| 'emit_when', default=[], | |||
There was a problem hiding this comment.
For explicitness sake, you could perhaps default this option to LIVEACTION_COMPLETED_STATES.
This way you could also simple the code here a bit - https://github.com/StackStorm/st2/pull/4315/files#diff-70b51c2693ac263742fc28ad30ebc932R234.
There was a problem hiding this comment.
For explicitness sake, you could perhaps default this option to LIVEACTION_COMPLETED_STATES.
If we specify the list here explicitly, then we need to follow LIVEACTION_COMPLETED_STATES changes that may happen in the future. So I thought this way is better but I'm OK with either option. What do you think?
This way you could also simple the code here a bit
Anyways that test case still needs to be there because users might have chance to specify a null or a blank list.
There was a problem hiding this comment.
That's true, but I don't think that should be a big deal because any change to that constant would also mean user would need to update and restart all the services so the change would be picked up.
There was a problem hiding this comment.
I will merge this into master and open a small PR with changelog entry and this change here which will allow us to slightly simplify the if statement.
There was a problem hiding this comment.
And another reason why I prefer to use a default value here is because it's more explicit.
If this option contain no default value here, user needs to dig through the code and documentation to figure out what a default value of None indicates.
|
Nice work, thank you 👍 I've added some comments, nothing major. |
| @@ -79,22 +78,18 @@ def process(self, execution_db): | |||
| extra = {'execution': execution_db} | |||
| LOG.debug('Processing action execution "%s".', execution_id, extra=extra) | |||
|
|
|||
| if execution_db.status not in LIVEACTION_COMPLETED_STATES: | |||
There was a problem hiding this comment.
Not a blocker, but I found previous code a bit easier to read / follow ("return early" vs "nested if", one level less of nesting).
Was there any particular reason why you changed it?
There was a problem hiding this comment.
This change is inevitable since:
- I thought the order of other function calls is important here and shouldn't be changed (to keep this:
apply_post_run_policies->_post_notify_triggers->_post_generic_trigger) - We always need to call
_post_generic_triggerregardless of the status. So If we want to do a "early return" then I need to move that call beforeif execution_db.status not in LIVEACTION_COMPLETED_STATES:line but that will break the order (_post_generic_triggerwill be called before_post_notify_triggers)
|
@Kami PR updated. Please take a look. |
| LOG.debug('Action trigger is disabled, skipping trigger dispatch...') | ||
| return | ||
|
|
||
| execution_id = str(execution_db.id) | ||
| extra = {'execution': execution_db} | ||
|
|
||
| target_statuses = cfg.CONF.action_sensor.emit_when |
There was a problem hiding this comment.
Small style thing - I would do target_statuses = cfg.CONF.action_sensor.emit_when or LIVEACTION_COMPLETED_STATES this way we could get rid of else clause.
Besides that, LGTM 👍
|
Merged, thanks! Changelog and default value change in #4330. |
As per motivation addressed in #4312
action_sensor.emit_whenwhich specifies the states when to emitst2.generic.actiontriggerLIVEACTION_COMPLETED_STATESthat is the exact same behavior as before. Meaning this change will not affect any existing deployments.action_sensor.enableparameter set to False.action_sensor.enableparameter was not honored at all.