mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-22 14:52:41 +01:00
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. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13244 Differential Revision: https://secure.phabricator.com/D20126
This commit is contained in:
parent
8fab8d8a18
commit
31a0ed92d0
3 changed files with 100 additions and 24 deletions
|
@ -4,7 +4,10 @@ final class PhabricatorOwnersAuditRule
|
|||
extends Phobject {
|
||||
|
||||
const AUDITING_NONE = 'none';
|
||||
const AUDITING_AUDIT = 'audit';
|
||||
const AUDITING_NO_OWNER = 'audit';
|
||||
const AUDITING_UNREVIEWED = 'unreviewed';
|
||||
const AUDITING_NO_OWNER_AND_UNREVIEWED = 'uninvolved-unreviewed';
|
||||
const AUDITING_ALL = 'all';
|
||||
|
||||
private $key;
|
||||
private $spec;
|
||||
|
@ -86,13 +89,26 @@ final class PhabricatorOwnersAuditRule
|
|||
'0' => '"0"',
|
||||
),
|
||||
),
|
||||
self::AUDITING_AUDIT => array(
|
||||
'name' => pht('Audit Commits'),
|
||||
self::AUDITING_UNREVIEWED => array(
|
||||
'name' => pht('Audit Unreviewed Commits'),
|
||||
'icon.icon' => 'fa-check',
|
||||
),
|
||||
self::AUDITING_NO_OWNER => array(
|
||||
'name' => pht('Audit Commits With No Owner Involvement'),
|
||||
'icon.icon' => 'fa-check',
|
||||
'deprecated' => array(
|
||||
'1' => '"1"',
|
||||
),
|
||||
),
|
||||
self::AUDITING_NO_OWNER_AND_UNREVIEWED => array(
|
||||
'name' => pht(
|
||||
'Audit Unreviewed Commits and Commits With No Owner Involvement'),
|
||||
'icon.icon' => 'fa-check',
|
||||
),
|
||||
self::AUDITING_ALL => array(
|
||||
'name' => pht('Audit All Commits'),
|
||||
'icon.icon' => 'fa-check',
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
@ -132,29 +132,85 @@ final class PhabricatorRepositoryCommitOwnersWorker
|
|||
$author_phid,
|
||||
$revision) {
|
||||
|
||||
// Don't trigger an audit if auditing isn't enabled for the package.
|
||||
$audit_uninvolved = false;
|
||||
$audit_unreviewed = false;
|
||||
|
||||
$rule = $package->newAuditingRule();
|
||||
if ($rule->getKey() === PhabricatorOwnersAuditRule::AUDITING_NONE) {
|
||||
return false;
|
||||
switch ($rule->getKey()) {
|
||||
case PhabricatorOwnersAuditRule::AUDITING_NONE:
|
||||
return false;
|
||||
case PhabricatorOwnersAuditRule::AUDITING_ALL:
|
||||
return true;
|
||||
case PhabricatorOwnersAuditRule::AUDITING_NO_OWNER:
|
||||
$audit_uninvolved = true;
|
||||
break;
|
||||
case PhabricatorOwnersAuditRule::AUDITING_UNREVIEWED:
|
||||
$audit_unreviewed = true;
|
||||
break;
|
||||
case PhabricatorOwnersAuditRule::AUDITING_NO_OWNER_AND_UNREVIEWED:
|
||||
$audit_uninvolved = true;
|
||||
$audit_unreviewed = true;
|
||||
break;
|
||||
}
|
||||
|
||||
// Trigger an audit if we don't recognize the commit's author.
|
||||
if (!$author_phid) {
|
||||
return true;
|
||||
// If auditing is configured to trigger on unreviewed changes, check if
|
||||
// the revision was "Accepted" when it landed. If not, trigger an audit.
|
||||
if ($audit_unreviewed) {
|
||||
$commit_unreviewed = true;
|
||||
if ($revision) {
|
||||
$was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED;
|
||||
if ($revision->isPublished()) {
|
||||
if ($revision->getProperty($was_accepted)) {
|
||||
$commit_unreviewed = false;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if ($commit_unreviewed) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
// If auditing is configured to trigger on changes with no involved owner,
|
||||
// check for an owner. If we don't find one, trigger an audit.
|
||||
if ($audit_uninvolved) {
|
||||
$commit_uninvolved = $this->isOwnerInvolved(
|
||||
$commit,
|
||||
$package,
|
||||
$author_phid,
|
||||
$revision);
|
||||
if ($commit_uninvolved) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
// We can't find any reason to trigger an audit for this commit.
|
||||
return false;
|
||||
}
|
||||
|
||||
private function isOwnerInvolved(
|
||||
PhabricatorRepositoryCommit $commit,
|
||||
PhabricatorOwnersPackage $package,
|
||||
$author_phid,
|
||||
$revision) {
|
||||
|
||||
$owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
|
||||
array(
|
||||
$package->getID(),
|
||||
));
|
||||
$owner_phids = array_fuse($owner_phids);
|
||||
|
||||
// Don't trigger an audit if the author is a package owner.
|
||||
if (isset($owner_phids[$author_phid])) {
|
||||
return false;
|
||||
// If the commit author is identifiable and a package owner, they're
|
||||
// involved.
|
||||
if ($author_phid) {
|
||||
if (isset($owner_phids[$author_phid])) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
// Trigger an audit of there is no corresponding revision.
|
||||
// Otherwise, we need to find an owner as a reviewer.
|
||||
|
||||
// If we don't have a revision, this is hopeless: no owners are involved.
|
||||
if (!$revision) {
|
||||
return true;
|
||||
}
|
||||
|
@ -174,21 +230,19 @@ final class PhabricatorRepositoryCommitOwnersWorker
|
|||
continue;
|
||||
}
|
||||
|
||||
// If this reviewer accepted the revision and owns the package, we're
|
||||
// all clear and do not need to trigger an audit.
|
||||
// If this reviewer accepted the revision and owns the package, we've
|
||||
// found an involved owner.
|
||||
if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) {
|
||||
$found_accept = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
// Don't trigger an audit if a package owner already reviewed the
|
||||
// revision.
|
||||
if ($found_accept) {
|
||||
return false;
|
||||
return true;
|
||||
}
|
||||
|
||||
return true;
|
||||
return false;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -114,16 +114,22 @@ Auditing
|
|||
========
|
||||
|
||||
You can automatically trigger audits on unreviewed code by configuring
|
||||
**Auditing**. The available settings are:
|
||||
**Auditing**. The available settings allow you to select behavior based on
|
||||
these conditions:
|
||||
|
||||
- **Disabled**: Do not trigger audits.
|
||||
- **Enabled**: Trigger audits.
|
||||
- **No Owner Involvement**: Triggers an audit when the commit author is not
|
||||
a package owner, and no package owner reviewed an associated revision in
|
||||
Differential.
|
||||
- **Unreviewed Commits**: Triggers an audit when a commit has no associated
|
||||
revision in Differential, or the associated revision in Differential landed
|
||||
without being "Accepted".
|
||||
|
||||
When enabled, audits are triggered for commits which:
|
||||
For example, the **Audit Commits With No Owner Involvement** option triggers
|
||||
audits for commits which:
|
||||
|
||||
- affect code owned by the package;
|
||||
- were not authored by a package owner; and
|
||||
- were not accepted by a package owner.
|
||||
- were not accepted (in Differential) by a package owner.
|
||||
|
||||
Audits do not trigger if the package has been archived.
|
||||
|
||||
|
|
Loading…
Reference in a new issue