Page MenuHomePhabricator

Harbormaster Herald Rules
Open, Needs TriagePublic

Description

I would like the ability to trigger notifications based on the result of a Harbormaster buildable. I would imagine the end result would allow you to set a build plan and status/statuses that you would like to filter by. I see this as being important for long running tests that should alert someone as soon as a failure is detected as I can't depend on all responsible parties to check the harbormaster application hourly.

I'm willing to do the work for this feature / I already have a working implementation.

Revisions and Commits

Event Timeline

michaeljs1990 updated the task description. (Show Details)
michaeljs1990 updated the task description. (Show Details)Feb 2 2016, 6:07 PM

How are these tests executing?

michaeljs1990 added a comment.EditedFeb 3 2016, 5:18 PM

Good point. When i was testing this i hooked into the Test Consul and allowed buildables to be passed in so everything was seemingly working for me. I am guessing i need to dig a littler deeper into the harbormaster transactions to get this working properly.

To be more specific, I just mean:

  • Currently, Diffusion alerts authors/etc when a commit fails to build.
  • Differential does not alert on revision failures, but will after T9276. The lack of alerting is not intended, there's just a complicated race condition that needs to be resolved. If these builds are executing because users are submitting Differential revisions and the users you want to notify are the author/reviewers, that might be a complete (or, at least, substantial) fix.
  • If the builds are triggering in some other way and the users you want to notify are not guessable by Phabricator (e.g., they're operations or build wizards or something) then Herald might be a good approach, but other approaches might be better (let users subscribe to build plans?). Having more information about your situation would help us expand the range of possible solutions.
  • If the builds are somehow release or deployment related, future integrations via T9530 may be relevant.

Our use case is fairly atypical unfortunately. We have builds setup to run every time a diff is pushed. These diffs run integration tests that could take anywhere from 30minutes to a few hours and we don't require them passing since it's an extra step on top of unit tests running. However in the event that a longer running test breaks after a diff is landed we want to be able to alert not only the user who broke the integration test but also a group of users who are actively maintaining them incase they need to coordinate work.

avivey added a subscriber: avivey.Feb 3 2016, 7:28 PM
avivey added a project: Restricted Project.EditedFeb 4 2016, 7:39 PM

My users want to send an email to the entire team when a build fails.

(I'm not exactly sure it's would do any good, but I can't convince them that they ignore all their emails anyway).

This is something we've needed for like over a year or more, because we need to filter the importance of different things failing, or we explicitly want messages when things pass.

elesh added a subscriber: elesh.Feb 5 2016, 5:59 AM
eadler added a project: Restricted Project.Feb 23 2016, 7:17 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

Maybe a more natural approach to this would be to add a "Notify" build step? We'd also need an "on failure" set of actions or something which implicitly accomplished the same goal, but that seems reasonable to me.

We could also make build plans subscribable, and include subscribers in all passes and failures.

Basically, my guess is that the application can easily support 95% of the use cases here with a couple of simple features like those. Herald is really powerful and can also support this stuff, but I'd like to to be a tool that you only break out when you need the complexity, and none of these use cases seem like they require the power of Herald.

Provided there was a way to distinguish between "all notifications" and "notifications on failure only", it sounds like a "Notify Users" build step would be sufficient for every use case discussed here (provided we also fix T9276):

However in the event that a longer running test breaks after a diff is landed we want to be able to alert not only the user who broke the integration test but also a group of users who are actively maintaining them incase they need to coordinate work.

The author should be notified by default since they own the thing that failed the build; you could add a Notify Users on Failure: #whatever_maintainers build step for the other half.

My users want to send an email to the entire team when a build fails.

Notify Users on Failure: #everyone

we need to filter the importance of different things failing, or we explicitly want messages when things pass

Not sure what the filtering bit would require, but Notify Users on Completion: #whoever should solve the second half.

We could even build explicit notification settings into build plans, since it sounds like all of these cases depend only on the plan (e.g., it doesn't matter what is being built) and don't need to be added by onlookers (e.g., whoever owns the plan can edit all the rules in). Conceptually I like making this a step, but this may be simpler to use and to implement as dedicated configuration.

The ability to add subscribers which is now on master should solve most of these issue for people.

Oh, I might not have made subscribers actually do anything, I don't remember. They definitely should, though, even if they don't yet.

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:16 PM
avivey removed a project: Restricted Project.

My management would like to flag Commits for Audit if they either don't have an accepted Revision (already have this setup in Herald) OR they have failed Harbormaster Builds. We're not terribly interested in notifications/emails for this. This use case seems a bit different than others discussed above.

michelkaeser added a subscriber: michelkaeser.