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

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

Details

Reviewers

amckinley

Maniphest Tasks

T13244: 2019 Week 6 Bonus Content

Commits

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

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Feb 7 2019, 6:16 PM

Harbormaster completed remote builds in B21882: Diff 48045.Feb 7 2019, 6:18 PM

epriestley requested review of this revision.Feb 7 2019, 6:18 PM

epriestley added a child revision: D20127: Make "bin/audit delete" synchronize commit audit status, and improve "bin/audit synchronize" documentation.Feb 7 2019, 6:34 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.)

Wordsmithing, spelling adjustments.
Beefier specs.
Properly document that an unaccepted revision doesn't count as a review.

Harbormaster completed remote builds in B21899: Diff 48062.Feb 7 2019, 11:11 PM

Closed by commit rP31a0ed92d08d: Support a wider range of "Audit" rules for Owners packages (authored by epriestley). · Explain WhyFeb 7 2019, 11:39 PM

This revision was automatically updated to reflect the committed changes.

joshuaspence added a subscriber: joshuaspence.Feb 11 2019, 6:45 AM

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);