Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rP Phabricator
Branch
audit2
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 21899
Build 29897: Run Core Tests
Build 29896: arc lint + arc unit

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.
src/applications/owners/constants/PhabricatorOwnersAuditRule.php
108

"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".

111

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

src/docs/user/userguide/owners.diviner
123–124

"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
src/applications/owners/constants/PhabricatorOwnersAuditRule.php
104

(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.
src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
177

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.

<?php

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

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

$viewer = PhabricatorUser::getOmnipotentUser();

$commit = (new DiffusionCommitQuery())
  ->setViewer($viewer)
  ->withIdentifiers([$commit_identifier])
  ->needCommitData(true)
  ->executeOne();

$owners_package = (new PhabricatorOwnersPackageQuery())
  ->setViewer($viewer)
  ->withIDs([$owners_package_id])
  ->executeOne();

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

$revision = (new DifferentialRevisionQuery())
  ->setViewer($viewer)
  ->withIDs([$revision_id])
  ->needReviewers(true)
  ->executeOne();

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