Prevent slack Update policy from posting new messages #298
Conversation
Signed-off-by: Andrei Petrus <andreipetrus2000@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #298 +/- ##
==========================================
- Coverage 55.35% 55.27% -0.08%
==========================================
Files 35 35
Lines 3438 3439 +1
==========================================
- Hits 1903 1901 -2
- Misses 1256 1258 +2
- Partials 279 280 +1 ☔ View full report in Codecov by Sentry. |
| if ts == "" || policy == Post || policy == PostAndUpdate { | ||
| newTs, channelID, err := SendMessageRateLimited( | ||
| // Updating an existing message | ||
| if ts != "" && (policy == Update || policy == PostAndUpdate) { |
There was a problem hiding this comment.
Updating is the first condition so it always takes priority. This way, when a message has a policy of PostAndUpdate, it always updates the existing message instead of posting a new one in a thread.
This might also fix a race condition in which PostAndUpdate would post a message in a thread instead of updating the initial message. I didn't encounter this behavior a lot so I'm not 100% sure. Either way, it makes sense for updates to take priority in this case.
|
Hey @pasha-codefresh, could you maybe take a look at this PR when you have some spare time? Not sure who else to ping. Thanks! |
|
Hey @pasha-codefresh. Any chance someone could look over this PR? |
The issue:
At the moment, a message with a policy of
Updatecan post a new message on Slack. This happens when the message gets sent for agroupingKeywhich doesn't have a recorded timestamp, which happens when there was no previous message posted for the respectivegrouping key.This is not correct and it is quite misleading since one would expect a policy of
Updateto never be able to actually post new messages. It also means that there is no practical difference between thePostAndUpdateandUpdatepolicies when it comes to sending a message for agroupingKeywhich didn't have any previous message recorded.It's important to know that just because a timestamp wasn't recorded, it doesn't mean that a message with a certain
groupingKeywasn't previously sent. The dictionary ofgroupingKey: timestampis kept in-memory, so upon a complete engine restart, these records would get lost.This could be considered a breaking change if someone relied on the
Updatepolicy to post new messages. It could also be considered a fix if the correct behavior ofUpdateis to never post a new message.The use-case/scenario with which this behavior was found:
We have multiple argo apps and we want to receive notifications when an error occurs. This would mean notifications for failed syncs, maybe degraded apps, etc.
At the moment this is doable, but it would be a bit hard to keep track of which apps were fixed and which weren't since the error messages are static. Even if the error for an app is now fixed, the error notification still stays in the slack channel, unchanged.
As a way to improve this experience, we want to do the following:
PostAndUpdate. Of course, this would have agroupingKeywhich is related to a certain revision.Update. They would have the samegroupingKeyas the error message.Having these 2 notifications would basically mean that errors would get posted to the channel, and once fixed, the error messages could be updated to reflect that the issues has been solved. This makes it much easier to follow and keep note of errors that still need fixing.
At the moment this doesn't work correctly. The successful sync messages do update existing error messages, but they also get posted when there is no corresponding error message for them.