luci-notify: satisfy Dart infra requirements |
|||
Issue descriptionDart infra cannot use luci-notify. In https://docs.google.com/a/google.com/document/d/1V0jGfGt8l8CNBPhXmjmbR8eHO00UJiooOZUCVLKc2iI/edit?disco=AAAABfusSrw athom@ wrote: Dart stopped using luci-notify primarily because it didn't have blamelist functionality and because it doesn't have a "notify on new failures only" mode (on_change/on_failure are too spammy). The default template of luci-notify used to even worse, because it was not very useful to non-infra developers, confusing (the link was hidded in Inbox) and it threaded poorly in Inbox (might be better with some recent changes). Ideally, I would like our email notifications to iterate towards this (having all the MUSTs would be sufficient for us to use luci-notify, assuming it notifies the blamelist on new failures): MUST new failure in <buildername> MUST <URL> (first to milo, later to a more sophisticated service that displays test results) MUST <blamelist> SHOULD <new failures> (first steps, later a list of tests, link to some page to see the full results) MAY <previous failures> (first steps, later a list of tests, link to some page to see the full results) MAY <reproduction steps> (how to re-run those tests locally and on swarming)
,
Sep 19
,
Sep 19
on_change: true
on_success: false
on_failure: true
would send an email on every failure, but won't send emails about successes. Is that too spammy?
,
Sep 19
on_change: false
on_success: false
on_failure: true
Did you mean this?
Yes, I think that's worse than just on_change: true (which is what we'll do for now).
We want to notify the committer that turned the bot red (in absence of a bisection mechanism, everybody on the blamelist is the best we can do). To everybody else, the emails are spam. The more we spam the less valuable the emails are.
What I'll do to lessen the impact is to use a different subject for the unwanted status->success notifications, so people can easily filter them out.
,
Oct 4
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-go.git/+/69e87bbbaa0dfb2ce471d6ac2a7cf43f622da977 commit 69e87bbbaa0dfb2ce471d6ac2a7cf43f622da977 Author: Nodir Turakulov <nodir@google.com> Date: Thu Oct 04 22:08:48 2018 [luci_notify] Add on_new_failure notification predicate Dart needs notifications on new failures, i.e. a failure after a non-failure status. Add a new canned predicate, as opposed to making breaking changes to existing semantics. Bug: 826545 Change-Id: Icd85ec55fc80343ece3dcebef3d443539eb64dcd Reviewed-on: https://chromium-review.googlesource.com/c/1262080 Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> Commit-Queue: Nodir Turakulov <nodir@chromium.org> [modify] https://crrev.com/69e87bbbaa0dfb2ce471d6ac2a7cf43f622da977/luci_notify/api/config/notify.go [modify] https://crrev.com/69e87bbbaa0dfb2ce471d6ac2a7cf43f622da977/luci_notify/api/config/notify.pb.go [modify] https://crrev.com/69e87bbbaa0dfb2ce471d6ac2a7cf43f622da977/luci_notify/api/config/notify.proto [add] https://crrev.com/69e87bbbaa0dfb2ce471d6ac2a7cf43f622da977/luci_notify/api/config/notify_test.go
,
Oct 5
Cool, with that last change we have a decent blamelist notification system for Dart. Thanks! The only thing that is missing is being able to access a blamelist (the list of people that were notified and some representation of their commits) in the template. It is probably more of a MAY than a MUST because we went live anyway (Gatekeeper was just too unreliable that it was better to take a hit on features).
,
Oct 8
@nodir: I've added on_new_failure to our config but I'm still getting SUCCESS emails: https://dart.googlesource.com/sdk/+/4d002d832973f28afd00b062494bf7eb40da1499/luci-notify.cfg#17 Looking at your change I don't see how that can happen, unless luci-notify doesn't pickup my latest config. Is there some way I can see if that is the case? Or debug what's wrong with my config?
,
Oct 8
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-go.git/+/7076f7972ef662fa6f366fb611663113aaf2711c commit 7076f7972ef662fa6f366fb611663113aaf2711c Author: Nodir Turakulov <nodir@google.com> Date: Mon Oct 08 16:16:54 2018 [luci-notify] Fix cron.yaml indentation TBR=tandrii@chromium.org Bug: 826545 Change-Id: Ib4823b7fae948396cd27a0890124a6d8a8a37087 Reviewed-on: https://chromium-review.googlesource.com/c/1268675 Reviewed-by: Nodir Turakulov <nodir@chromium.org> Commit-Queue: Nodir Turakulov <nodir@chromium.org> [modify] https://crrev.com/7076f7972ef662fa6f366fb611663113aaf2711c/luci_notify/frontend/cron.yaml
,
Oct 8
looks like I failed to actually deploy the feature. It is deployed now.
,
Nov 28
i've filed a separate issue 909548 for the blamelist, let's close this one |
|||
►
Sign in to add a comment |
|||
Comment 1 by athom@google.com
, Sep 19We can now do most of what we need to do to. And given that GK is mostly unmaintained, we'll switch back to luci-notify. The most annoying missing piece is that on_change also sends a notification when the new status "success". @nodir: mknyzsek told me that is "by design". Is there any chance of revisiting that decision? Maybe the meaning of the configuration could be adapted so that this: on_change: true on_success: false on_failure: true would mean the notifier notifies on changes that where the new status is a failure but not if the new status is a success? @mknyszek is there something we can do in the recipe to prevent luci-notify from sending emails for successful builds (we never want emails for those)?