Page MenuHomestyx hydra
Files F38216

No OneTemporary
Actions

View Options

diff --git a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
index 1f8fd0b1bd..32a9bb804b 100644
--- a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
+++ b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
@@ -1,101 +1,117 @@
<?php
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;
public static function newFromState($key) {
$specs = self::newSpecifications();
$spec = idx($specs, $key, array());
$rule = new self();
$rule->key = $key;
$rule->spec = $spec;
return $rule;
}
public function getKey() {
return $this->key;
}
public function getDisplayName() {
return idx($this->spec, 'name', $this->key);
}
public function getIconIcon() {
return idx($this->spec, 'icon.icon');
}
public static function newSelectControlMap() {
$specs = self::newSpecifications();
return ipull($specs, 'name');
}
public static function getStorageValueFromAPIValue($value) {
$specs = self::newSpecifications();
$map = array();
foreach ($specs as $key => $spec) {
$deprecated = idx($spec, 'deprecated', array());
if (isset($deprecated[$value])) {
return $key;
}
}
return $value;
}
public static function getModernValueMap() {
$specs = self::newSpecifications();
$map = array();
foreach ($specs as $key => $spec) {
$map[$key] = pht('"%s"', $key);
}
return $map;
}
public static function getDeprecatedValueMap() {
$specs = self::newSpecifications();
$map = array();
foreach ($specs as $key => $spec) {
$deprecated_map = idx($spec, 'deprecated', array());
foreach ($deprecated_map as $deprecated_key => $label) {
$map[$deprecated_key] = $label;
}
}
return $map;
}
private static function newSpecifications() {
return array(
self::AUDITING_NONE => array(
'name' => pht('No Auditing'),
'icon.icon' => 'fa-ban',
'deprecated' => array(
'' => pht('"" (empty string)'),
'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',
+ ),
);
}
}
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index 219314c9d5..65434658ab 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -1,194 +1,248 @@
<?php
final class PhabricatorRepositoryCommitOwnersWorker
extends PhabricatorRepositoryCommitParserWorker {
protected function getImportStepFlag() {
return PhabricatorRepositoryCommit::IMPORTED_OWNERS;
}
protected function parseCommit(
PhabricatorRepository $repository,
PhabricatorRepositoryCommit $commit) {
if (!$this->shouldSkipImportStep()) {
$this->triggerOwnerAudits($repository, $commit);
$commit->writeImportStatusFlag($this->getImportStepFlag());
}
if ($this->shouldQueueFollowupTasks()) {
$this->queueTask(
'PhabricatorRepositoryCommitHeraldWorker',
array(
'commitID' => $commit->getID(),
));
}
}
private function triggerOwnerAudits(
PhabricatorRepository $repository,
PhabricatorRepositoryCommit $commit) {
$viewer = PhabricatorUser::getOmnipotentUser();
if (!$repository->shouldPublish()) {
return;
}
$affected_paths = PhabricatorOwnerPathQuery::loadAffectedPaths(
$repository,
$commit,
PhabricatorUser::getOmnipotentUser());
$affected_packages = PhabricatorOwnersPackage::loadAffectedPackages(
$repository,
$affected_paths);
$commit->writeOwnersEdges(mpull($affected_packages, 'getPHID'));
if (!$affected_packages) {
return;
}
$commit = id(new DiffusionCommitQuery())
->setViewer($viewer)
->withPHIDs(array($commit->getPHID()))
->needCommitData(true)
->needAuditRequests(true)
->executeOne();
if (!$commit) {
return;
}
$data = $commit->getCommitData();
$author_phid = $data->getCommitDetail('authorPHID');
$revision_id = $data->getCommitDetail('differential.revisionID');
if ($revision_id) {
$revision = id(new DifferentialRevisionQuery())
->setViewer($viewer)
->withIDs(array($revision_id))
->needReviewers(true)
->executeOne();
} else {
$revision = null;
}
$requests = $commit->getAudits();
$requests = mpull($requests, null, 'getAuditorPHID');
$auditor_phids = array();
foreach ($affected_packages as $package) {
$request = idx($requests, $package->getPHID());
if ($request) {
// Don't update request if it exists already.
continue;
}
$should_audit = $this->shouldTriggerAudit(
$commit,
$package,
$author_phid,
$revision);
if (!$should_audit) {
continue;
}
$auditor_phids[] = $package->getPHID();
}
// If none of the packages are triggering audits, we're all done.
if (!$auditor_phids) {
return;
}
$audit_type = DiffusionCommitAuditorsTransaction::TRANSACTIONTYPE;
$owners_phid = id(new PhabricatorOwnersApplication())
->getPHID();
$content_source = $this->newContentSource();
$xactions = array();
$xactions[] = $commit->getApplicationTransactionTemplate()
->setTransactionType($audit_type)
->setNewValue(
array(
'+' => array_fuse($auditor_phids),
));
$editor = $commit->getApplicationTransactionEditor()
->setActor($viewer)
->setActingAsPHID($owners_phid)
->setContinueOnNoEffect(true)
->setContinueOnMissingFields(true)
->setContentSource($content_source);
$editor->applyTransactions($commit, $xactions);
}
private function shouldTriggerAudit(
PhabricatorRepositoryCommit $commit,
PhabricatorOwnersPackage $package,
$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;
}
$accepted_statuses = array(
DifferentialReviewerStatus::STATUS_ACCEPTED,
DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER,
);
$accepted_statuses = array_fuse($accepted_statuses);
$found_accept = false;
foreach ($revision->getReviewers() as $reviewer) {
$reviewer_phid = $reviewer->getReviewerPHID();
// If this reviewer isn't a package owner, just ignore them.
if (empty($owner_phids[$reviewer_phid])) {
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;
}
}
diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner
index 95a3882552..df74fc6e83 100644
--- a/src/docs/user/userguide/owners.diviner
+++ b/src/docs/user/userguide/owners.diviner
@@ -1,174 +1,180 @@
@title Owners User Guide
@group userguide
Group files in a codebase into packages and define ownership.
Overview
========
The Owners application allows you to group files in a codebase (or across
codebases) into packages. This can make it easier to reference a module or
subsystem in other applications, like Herald.
Creating a Package
==================
To create a package, choose a name and add some files which belong to the
package. For example, you might define an "iOS Application" package by
including these paths:
/conf/ios/
/src/ios/
/shared/assets/mobile/
Any files in those directories are considered to be part of the package, and
you can now conveniently refer to them (for example, in a Herald rule) by
referring to the package instead of copy/pasting a huge regular expression
into a bunch of places.
If new source files are later added, or the scope of the package otherwise
expands or contracts, you can edit the package definition to keep things
updated.
You can use "exclude" paths to ignore subdirectories which would otherwise
be considered part of the package. For example, you might exclude a path
like this:
/conf/ios/generated/
Perhaps that directory contains some generated configuration which frequently
changes, and which you aren't concerned about.
After creating a package, files the package contains will be identified as
belonging to the package when you look at them in Diffusion, or look at changes
which affect them in Diffusion or Differential.
Dominion
========
The **Dominion** option allows you to control how ownership cascades when
multiple packages own a path. The dominion rules are:
**Strong Dominion.** This is the default. In this mode, the package will always
own all files matching its configured paths, even if another package also owns
them.
For example, if the package owns `a/`, it will always own `a/b/c.z` even if
another package owns `a/b/`. In this case, both packages will own `a/b/c.z`.
This mode prevents users from stealing files away from the package by defining
more narrow ownership rules in new packages, but enforces hierarchical
ownership rules.
**Weak Dominion.** In this mode, the package will only own files which do not
match a more specific path in another package.
For example, if the package owns `a/` but another package owns `a/b/`, the
package will no longer consider `a/b/c.z` to be a file it owns because another
package matches the path with a more specific rule.
This mode lets you to define rules without implicit hierarchical ownership,
but allows users to steal files away from a package by defining a more
specific package.
For more details on files which match multiple packages, see
"Files in Multiple Packages", below.
Auto Review
===========
You can configure **Auto Review** for packages. When a new code review is
created in Differential which affects code in a package, the package can
automatically be added as a subscriber or reviewer.
The available settings allow you to take these actions:
- **Review Changes**: This package will be added to reviews as a reviewer.
Reviews will appear on the dashboards of package owners.
- **Review Changes (Blocking)** This package will be added to reviews as a
blocking reviewer. A package owner will be required to accept changes
before they may land.
- **Subscribe to Changes**: This package will be added to reviews as a
subscriber. Owners will be notified of changes, but not required to act.
If you select the **With Non-Owner Author** option for these actions, the
action will not trigger if the author of the revision is a package owner. This
mode may be helpful if you are using Owners mostly to make sure that someone
who is qualified is involved in each change to a piece of code.
If you select the **All** option for these actions, the action will always
trigger even if the author is a package owner. This mode may be helpful if you
are using Owners mostly to suggest reviewers.
These rules do not trigger if the package has been archived.
The intent of this feature is to make it easy to configure simple, reasonable
behaviors. If you want more tailored or specific triggers, you can write more
powerful rules by using Herald.
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.
The intent of this feature is to make it easy to configure simple auditing
behavior. If you want more powerful auditing behavior, you can use Herald to
write more sophisticated rules.
Ignored Attributes
==================
You can automatically exclude certain types of files, like generated files,
with **Ignored Attributes**.
When a package is marked as ignoring files with a particular attribute, and
a file in a particular change has that attribute, the file will be ignored when
computing ownership.
(This feature is currently rough, only works for Differential revisions, and
may not always compute the correct set of owning packages in some complex
cases where it interacts with dominion rules.)
Files in Multiple Packages
==========================
Multiple packages may own the same file. For example, both the
"Android Application" and the "iOS Application" packages might own a path
like this, containing resources used by both:
/shared/assets/mobile/
If both packages own this directory, files in the directory are considered to
be part of both packages.
Packages do not need to have claims of equal specificity to own files. For
example, if you have a "Design Assets" package which owns this path:
/shared/assets/
...it will //also// own all of the files in the `mobile/` subdirectory. In this
configuration, these files are part of three packages: "iOS Application",
"Android Application", and "Design Assets".
(You can use an "exclude" rule if you want to make a different package with a
more specific claim the owner of a file or subdirectory. You can also change
the **Dominion** setting for a package to let it give up ownership of paths
owned by another package.)

File Metadata

Mime Type
text/x-diff
Expires
Thu, Feb 6, 1:41 PM (1 d, 7 h)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
34029
Default Alt Text
(18 KB)

Event Timeline

styx copyright 2020 sirocyl / ellie (kymkdd) / ncommander / styx contributors
Use of other names and brands is not intended to reflect a claim of, or right to, use the name or brand.
styx is an operating system using the Linux kernel and components, and "styx Linux" as a phrase does not reflect the inclusion of the Linux brand or trademark in the styx project.
Linux® is the registered trademark of Linus Torvalds in the U.S. and other countries.
Terms and Conditions (TODO: fix this!)