fix(notifications): Don't send failure notifications for cancelled pipelines and stages #663
Conversation
|
@emjburns @marchello2000 Please review. |
...est/groovy/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgentSpec.groovy
Outdated
Show resolved
Hide resolved
| @@ -78,7 +78,13 @@ abstract class AbstractEventNotificationAgent implements EchoEventListener { | |||
| return | |||
| } | |||
|
|
|||
| if (config.type == 'pipeline' && event.content.execution?.status == 'CANCELED') { | |||
| if (config.type == 'stage' && event.content.canceled == true) { | |||
There was a problem hiding this comment.
so one repro/failure scenario is, have a "failure" notification on a stage, cancel a pipeline while that stage is running?
I don't understand the second repro case (below)?
There was a problem hiding this comment.
oh, i see.... when we cancel the execution, we don't refetch the execution from the db hence canceled = true but status is probably still = running..? but I still don't understand why this doesn't happen always..?
There was a problem hiding this comment.
Yes, that’s what I’m thinking, but I couldn’t quite pin it down, so this was more of a hunch... My guess is that it’s a timing issue/race condition somewhere, but I’m not yet familiar enough with the codebase to find it and after a couple of days I thought this was a reasonable compromise.
I could add some logging around this case, in case it continues to happen...
|
@emjburns and @marchello2000 Would you please look this over one more time and give it a 👍 if you think it looks good? Thanks! |
|
@gal-yardeni FYI |
…l one for cancelled pipelines + tests
luispollo
force-pushed
the
fix-SPIN-5003
branch
from
18b828b
to
718a1a9
Compare
This fixes an issue where Echo would send failure notifications for cancelled stages, and tries to account for a similar scenario with cancelled pipelines where the execution object has two different fields for cancellation status and only one was being checked.
Also added tests to cover some of the basic flows in
AbstractEventNotificationAgent, which is where this logic resides.