Page MenuHomePhabricator

Herald Rules - Trigger when Diff content contains
Closed, WontfixPublic


I created a herald rule with the condition "any changed file content" contains "something" and I thought it to mean that whenever a changed occurred in any file of the diff and the files on that diff contained "something" then the rule would fire, but it does not. Is there any way to accomplish this? I tried using "body" contains but I imagine that means the body of the diff description.

Event Timeline

anthony.victorio raised the priority of this task from to Needs Triage.
anthony.victorio updated the task description. (Show Details)
anthony.victorio added a subscriber: anthony.victorio.

There is no way to accomplish this. In the general case, diffs may be uploaded without any context. In these cases, we can't test against unmodified content which simply appears elsewhere in the file.

What's your use case?

We have certain objects that are financially sensitive, so whenever a diff is submitted and the file (either the old or new version) contains a reference to one of those objects, we would like to add a blocking reviewer. This reviewer would then go through the details of the diff and ensure that all appropriate documentation is present for compliance auditing.

The reason I would want this rule to fire when content of the files contain the keyword as opposed to just when the changed content contains the keyboard is because an object may be reference by a variable and that variable name could be any number of things.

I imagine that there are many similar uses cases out there by others. A business process could require that additional reviewers be added to a diff when the existing or changing file content contains a specific keyword (or set of keywords).

If I have something like

FinaciallySenstiveObject fsb = new FinaciallySenstiveObject();
fsb.Amount = 1000;

And in a diff, only line 2 is modified.

fsb.Amount = 2000;

Then I would want to easily set up rules that says, whenever file contains FinaciallySenstiveObject then add blocking reviewer: "financial auditor".

chad triaged this task as Wishlist priority.Jun 19 2014, 6:14 PM

There's no technical barrier to implementing this, it just won't work some of the time since we don't have the context. I'm generally a bit skeptical about the value of these rules, but reasonable use cases aren't totally unthinkable. T5029 may make it practical more of the time.

epriestley claimed this task.

I don't think this is a good fit for the upstream: it's inherently a bit unreliable, can't be made reliable, and I don't think the use case described here is a particularly robust one even if it's reasonable for some organizations.

This could be mostly implemented as an extension today by extending HeraldField. DifferentialDiffContentHeraldField is a starting point if anyone wants to pursue this.