I don't think it's likely this will be accepted as it creates unnecessary transactions against the buildable just for Herald.

Yes i should likely abstract the transactions from the herald implementation however it was not immediately apparent to me when I was writing this. However I am trying to dig up the ticket that requested transactions be added for completed builds but can't find it right now.

T9276

So i'm about to change a bunch of this since hooking into PhabricatorApplicationTransaction seems like the proper thing to do. Adding transactions to the timeline i'll split out into another diff.

Edit:
It looks like this is good history on what has happened with buildable/build transactions D9110. Although it leaves me a little confused now on what is the proper way to implement it. Having buildable transactions baked into PhabricatorApplicationTransaction seems like it might be the wrong way to go since these are in most cases annoying. However in specific cases such as long running build plans you may want to know about the status. Herald seems like a better place for this since users can configure it and it doesn't change the current functionality in any way.

Maybe i am understanding this wrong but it seems prior to this diff it wasn't possible to have herald pick up the status of a buildable since the only way these are currently exposed is if another application extends PhabricatorApplicationTransaction. So you would have to write a herald rule for every application separately if you wanted people to be able to trigger notifications

Anything i can do to make this attractive / more attractive to upstream?

Upstream generally doesn't review feature diffs from third parties, if at all. You should probably be expecting a response time in the range of "months" to "never" when it comes to diffs.

Refer to the contributing documentation for more info.

T10363 talks about an alternative approach to the problem of "Notifications"; It's not a 100% pretty, but it would work.

Personally, I think the approach here - adding more transactions records - is right, even if a little odd (The actor is non-user AND the it's not exactly a response to a user-acting transaction); I think we want those transaction, or at least, we want to be able to fake having them: T9276: Display when builds happen with status somewhere handy, like the differential timeline, and showing better history on Buildables.

I'm not sure if this is on the path to "Notify revision author when build fails/completes", it might be (Via a secondary transaction on the revision?).

(I keep meaning to look at the implementation here, but end up distracted).

In D15247#175756, @avivey wrote:

T10363 talks about an alternative approach to the problem of "Notifications"; It's not a 100% pretty, but it would work.

I'll take a look at this

I'm not sure if this is on the path to "Notify revision author when build fails/completes", it might be (Via a secondary transaction on the revision?).

I need to push up one more patch that I have sitting on my local install that will do this. I need to test it a little more first before I am confident in it though.

Do modular transactions make this more feasible now?

More of this could be built outside of the upstream once Harbormaster converts to modular transactions. I think these pieces currently must be upstream, but will be modular "soon":

• The instanceof check in HeraldTestController: this will be modularized by T9719.
• The TYPE_STATUS transaction and its rendering and behavior: modular transactions makes these flexible, and this would be extensible today if Harbormaster updated.

These parts don't have a short path to modularity:

• Having the HarbormasterBuildableTransactionEditor build a HeraldAdapter -- we don't currently expect third-party applications to add Herald support to first-party applications which don't ship with any support, only extend support that already exists.
• Applying the STATUS transaction to the buildable inside the BuildEngine. This is more or less just "run random code at a certain point in the workflow" and there's no hook or extension point for it today.

This isn't terribly helpful since it doesn't really provide a pathway forward.

The major blocker to upstreaming this is that I think we have a simpler set of more natural capabilities we can implement to cover almost all of these use cases (roughly described in T10260#162093) and I'd like to plan a pathway through that first.

In this particular case, Buildables also already have a status field and it's already updated in an appropriate place (right above the changes made here). At first glance, switching that to just be transactional might be a better way to get the TYPE_STATUS in place, although it's updated inside a lock and before we check if we should publish the build results, so it might be correct to not actually apply that change via transaction.

This general approach also only lets you write a rule that approximately means "notify me about every change to a buildable if its state is currently failed", which is likely not the rule anyone really wants to encode (they probably want "notify me when the state changes to failed, only", and maybe an "all clear" notification if it later passes). A side effect of this today is the double mail on restart, but in the future we're likely to add more types of mutable state to Buildables and a Herald rule structured like this would generate emails for all of them.

This isn't an exhaustive list of considerations, but it's fairly easy for me to come up with questions like this that I don't have great answers to yet, so I'm hesitant about pushing forward here before I have more of a chance to plan through all of this.