Page MenuHomePhabricator

Support a wider range of "Audit" rules for Owners packages

Authored by epriestley on Feb 7 2019, 6:16 PM.



Depends on D20124. Ref T13244. See PHI1055. Add a few more builtin audit behaviors to make Owners more flexible.

(At the upper end of flexibility you can trigger audits in a very granular way with Herald, but you tend to need to write one rule per Owners package, and providing a middle ground here has worked reasonably well for "review" rules so far.)

Test Plan
  • Edited a package to select the various different audit rules.
  • Used bin/repository reparse --force --owners <commit> to trigger package audits under varied conditions.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 7 2019, 6:16 PM
epriestley requested review of this revision.Feb 7 2019, 6:18 PM
amckinley accepted this revision.Feb 7 2019, 11:04 PM
amckinley added inline comments.

"Audit Commits With"

Also this is mostly an "English is stupid" thing, but it's weird that the constant is named AUDITING_NO_OWNER_OR_UNREVIEWED but the string says "Involvement and Unreviewed".


Looks like you accidentally stripped the rest of the spec from this option.


"and was landed without being approved."

This revision is now accepted and ready to land.Feb 7 2019, 11:04 PM
epriestley added inline comments.Feb 7 2019, 11:08 PM

(Oops, I also misspelled "Unreviewed" here.)

epriestley updated this revision to Diff 48062.Feb 7 2019, 11:10 PM
  • Wordsmithing, spelling adjustments.
  • Beefier specs.
  • Properly document that an unaccepted revision doesn't count as a review.
This revision was automatically updated to reflect the committed changes.
joshuaspence added inline comments.

I think this is wrong? The way this is written, if the owners are involved (i.e. $this->isOwnerInvolved() returns true) then shouldTriggerAudit will return true. I ran the following script (yes it's gross because it uses reflection, I know) and it returns false before this change and true after.


require_once '/usr/local/src/phabricator/scripts/__init_script__.php';

$commit_identifier = 'rPPdd9e3b4e5f2f4e5105cf4d1b983f41c75242fa9f';
$owners_package_id = 43;

$viewer = PhabricatorUser::getOmnipotentUser();

$commit = (new DiffusionCommitQuery())

$owners_package = (new PhabricatorOwnersPackageQuery())

$author_phid = $commit->getCommitData()->getCommitDetail('authorPHID');
$revision_id = $commit->getCommitData()->getCommitDetail('differential.revisionID');

$revision = (new DifferentialRevisionQuery())

$class = new PhabricatorRepositoryCommitOwnersWorker([]);
$method = new ReflectionMethod($class, 'shouldTriggerAudit');
$result = $method->invoke($class, $commit, $owners_package, $author_phid, $revision);