-
-
Notifications
You must be signed in to change notification settings - Fork 782
Add support to skip action notifications when executed under workflow context #5227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for the contribution. The bug fix / change looks good to me overall, I just had some questions / comments as far as tests and action-chain workflows go. |
Kami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks for working.
I will merge this into master and add changelog entry separately so I don't block and hold this change any longer.
|
For some reason after merging this, those new tests are consistently failing for me locally with this error - https://gist.github.com/Kami/52543866f5400e29537f00911654bfe1. No idea (yet) why it happens locally, but not on CI. I likely won't have time to dig in before the weekend. I wonder if it's related to coordination backend? But I don't think we set that up for unit tests on CI, we just do that for integration tests. |
|
OK, I see the issue - https://github.com/StackStorm/st2/pull/5227/files#diff-c86e8df3fb95a5e1b0806ce20a77ee1a92f721e933a1d230e5a302b1ebb10873R210. This function needs some more love. Right now it will return as soon as execution has 1 or more children, but from the code that's not actually what we want - we want to wait for 2 children. So the code needs to be changed to handle that better otherwise it will be prone to timing / race conditions. I will push a fix shortly. |
|
Here we go - 5856fa4. I confirmed that with this change, the tests now consistently pass locally. |
This pull request resolves #5221