revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 14:00:56 +01:00
Code Issues Releases Wiki Activity

Update some Audit documentation

Browse source 
Summary:
Ref T10978.

  - Generally refresh this documentation.
  - Use the word "publish", not the word "push", to distinguish between review and audit, echoing the language in the "Write, Review, Merge, Publish" document.
  - Mention the new "Needs Verification" state.

Test Plan: Read documentation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978

Differential Revision: https://secure.phabricator.com/D17253
This commit is contained in:
epriestley 2017-01-26 07:15:42 -08:00
parent 97cac83e9b
commit 3b8e2739fc
2 changed files with 148 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

144
src/docs/user/userguide/audit.diviner
View file

@ -1,71 +1,125 @@
@title Audit User Guide
@group userguide
Guide to the Audit (post-push code review) tool and workflow.
Guide to using Phabricator to audit published commits.
= Overview =
Phabricator supports two code review workflows, "review" (pre-push) and
"audit" (post-push). To understand the differences between the two, see
Overview
========
Phabricator supports two code review workflows, "review" (pre-publish) and
"audit" (post-publish). To understand the differences between the two, see
@{article:User Guide: Review vs Audit}.
This document summarizes the post-push "audit" workflow implemented by the
creatively-named //Audit// tool.
= How Audit Works =
How Audit Works
===============
Using auditing allows you to push and deploy code without waiting for code
review, while still doing code review eventually. The Audit tool primarily keeps
track of two things:
The audit workflow occurs after changes have been published. It provides ways
to track, discuss, and resolve issues with commits that are discovered after
they go through whatever review process you have in place (if you have one).
Two examples of how you might use audit are:
**Fix Issues**: If a problem is discovered after a change has already been
published, users can find the commit which introduced the problem and raise a
concern on it. This notifies the author of the commit and prompts them to
remedy the issue.
**Watch Changes**: In some cases, you may want to passively look over changes
that satisfy some criteria as they are published. For example, you may want to
review all Javascript changes at the end of the week to keep an eye on things,
or make sure that code which impacts a subsystem is looked at by someone on
that team, eventually.
Developers may also want other developers to take a second look at things if
they realize they aren't sure about something after a change has been published,
or just want to provide a heads-up.
You can configure Herald rules and Owners packages to automatically trigger
audits of commits that satisfy particular criteria.
Audit States and Actions
========================
The audit workflow primarily keeps track of two things:
- **Commits** and their audit state (like "Not Audited", "Approved", or
"Concern Raised").
- **Audit Requests** which ask a user (or some other entity) to audit a
commit. These can be triggered in a number of ways (see below).
- **Audit Requests** which ask a user (or some other entity, like a project
or package) to audit a commit. These can be triggered in a number of ways
(see below).
In the Audit tool's home screen and on the homepage you can see commits and
requests that require your action:
Users interact with commits by leaving comments and applying actions, like
accepting the changes or raising a concern. These actions change the state of
their own audit and the overall audit state of the commit. Here's an example of
a typical audit workflow:
- **Required Audits** are open audit requests that require you, a project
you are a member of, or a package you own to audit a commit. An audit
request is closed when you approve the associated commit.
- **Problem Commits** are commits you authored which someone has raised a
concern about in audit. Problem commits go away when you satisfy all the
auditors and get them to "Approve" the commit.
- Alice publishes a commit containing some Javascript.
- This triggers an audit request to Bailey, the Javascript technical
lead on the project (see below for a description of trigger mechanisms).
- Later, Bailey logs into Phabrictor and sees the audit request. She ignores
it for the moment, since it isn't blocking anything. At the end of the
week she looks through her open requests to see what the team has been
up to.
- Bailey notices a few minor problems with Alice's commit. She leaves
comments describing improvements and uses "Raise Concern" to send the
commit back into Alice's queue.
- Later, Alice logs into Phabricator and sees that Bailey has raised a
concern (usually, Alice will also get an email). She resolves the issue
somehow, maybe by making a followup commit with fixes.
- After the issues have been dealt with, she uses "Request Verification" to
return the change to Bailey so Bailey can verify that the concerns have
been addressed.
- Bailey uses "Accept Commit" to close the audit.
For example:
In {nav Diffusion > Browse Commits}, you can review commits and query for
commits with certain audit states. The default "Active Audits" view shows
all of the commits which are relevant to you given their audit state, divided
into buckets:
- Evan creates commit `abcdef1234` and pushes it to the remote.
- This triggers an audit request to Bob through some mechanism (see below for
a description of trigger mechanisms).
- Later, Bob logs into Phabricator and sees the audit request on his homepage.
- Bob clicks through and examines the commit. He notices a problem, so he
selects "Raise Concern" and describes the issue in a comment.
- Evan receives an email that Bob has raised a concern about his commit. He
opts not to deal with it immediately.
- Later, Evan logs into Phabricator and sees the commit on his homepage
under "Problem Commits".
- Evan resolves the issue somehow (e.g., by discussing it with Bob, or fixing
it in another commit).
- Now satisfied, Bob "Accepts" the original commit.
- This causes the request to disappear from Bob's queue, and the commit to
disappear from Evan's queue.
- **Needs Attention**: These are commits which you authored that another
user has raised a concern about: for example, maybe they believe they have
found a bug or some other problem. You should address the concerns.
- **Needs Verification**: These are commits which someone else authored
that you previously raised a concern about. The author has indicated that
they believe the concern has been addressed. You should verify that the
remedy is satisfactory and accept the change, or raise a further concern.
- **Ready to Audit**: These are commits which someone else authored that you
have been asked to audit, either by a user or by a system rule. You should
look over the changes and either accept them or raise concerns.
- **Waiting on Authors**: These are commits which someone else authored that
you previously raised a concern about. The author has not responded to the
concern yet. You may want to follow up.
- **Waiting on Auditors**: These are commits which you authored that someone
else needs to audit.
= Audit Triggers =
You can use the query constraints to filter this list or find commits that
match certain criteria.
Audit Triggers
==============
Audit requests can be triggered in a number of ways:
- You can add auditors explicitly from the web UI, using either "Edit Commit"
or the "Change Auditors" action. You might do this if you realize you are
not sure about something that you recently published and want a second
opinion.
- If you put `Auditors: username1, username2` in your commit message, it will
trigger an audit request to those users when you push it to a tracked
branch.
- You can create rules in Herald that trigger audits based on properties
of the commit -- like the files it touches, the text of the change, the
author, etc.
- You can create an audit request for yourself by commenting on any commit.
- You can create an Owners package and select "Enable Auditing" (this is an
advanced feature which is only likely to be useful for very large teams).
- You can create an Owners package and enable automatic auditing for the
package.
= Audits in Small Teams =
Audits in Small Teams
=====================
If you have a small team and don't need complicated trigger rules, you can set
up a simple audit workflow like this:
@ -84,7 +138,9 @@ commit should have //someone// look at it".
Once your team gets bigger, you can refine this ruleset so that developers see
only changes that are relevant to them.
= Audit Tips =
Audit Tips
==========
- When viewing a commit, audit requests you are responsible for are
highlighted. You are responsible for a request if it's a user request
@ -99,6 +155,8 @@ only changes that are relevant to them.
you submit a comment at the bottom of the page.
- Press "?" to view keyboard shortcuts.
= Next Steps =
Next Steps
==========
- Learn more about Herald at @{article:Herald User Guide}.

74
src/docs/user/userguide/reviews_vs_audit.diviner
View file

@ -1,30 +1,45 @@
@title User Guide: Review vs Audit
@group userguide
Discusses the differences between Review and Audit workflows.
Discusses the differences between "review" and "audit" workflows.
= Overview =
Overview
========
Phabricator supports two similar but separate code review workflows:
Phabricator supports two similar but separate code review workflows: "review"
and "audit".
- **Differential** is used for pre-push code review, called "reviews"
elsewhere in the documentation. You can learn more in
@{article:Differential User Guide}.
- **Audit** is used for post-push code reviews, called "audits" elsewhere in
the documentation. You can learn more in @{article:Audit User Guide}.
Review occurs in **Differential**, before changes are published. You can learn
more in @{article:Differential User Guide}.
(By "pre-push", this document means review which blocks deployment of changes,
while "post-push" means review which happens after changes are deployed or
en route to deployment.)
Audit occurs in **Diffusion**, after changes are published. You can learn more
in @{article:Audit User Guide}.
Both are lightweight, asynchronous web-based workflows where reviewers/auditors
inspect code independently, from their own machines -- not synchronous review
sessions where authors and reviewers meet in person to discuss changes.
When this documentation discusses "unpublished changes", it refers to changes
which are still subject to being reworked in response to feedback. In many
workflows, these changes will only exist locally on the developer's machine,
but some workflows push tentative or temporary changes into remotes. The step
that "publishes" changes might be either pushing or merging them, depending on
your workflow.
= Advantages of Review =
Both the audit and review workflows are lightweight, asynchronous web-based
workflows where reviewers or auditors inspect code independently, from their
own machines -- not synchronous review sessions where authors and reviewers
meet in person to discuss changes.
Pre-push review is significantly more powerful than post-push auditing. You
gain these advantages by requiring review //before// changes may be pushed:
Broadly, review is normally a //blocking// workflow: in review workflows,
authors usually can not publish changes until review completes and reviewers
are satisfied.
In contrast, audit is normally a //nonblocking// workflow: in audit workflows,
changes usually move forward by default.
Advantages of Review
====================
Pre-publish review is significantly more powerful than post-publish auditing.
You gain these advantages by requiring review //before// changes may be
published:
- Authors have a strong incentive to craft small, well-formed changes that
will be readily understood, to explain them adequately, and to provide
@ -32,11 +47,12 @@ gain these advantages by requiring review //before// changes may be pushed:
- Reviewers have a real opportunity to make significant suggestions about
architecture or approach in review. These suggestions are less attractive
to adopt from audit, and may be much more difficult to adopt if significant
time has passed between push and audit.
time has passed between publish and audit.
- Authors have a strong incentive to fix problems and respond to feedback
received during review, because it blocks them. Authors have a much weaker
incentive to address problems raised during audit.
- Authors can ask reviewers to apply and verify fixes before they are pushed.
received during review because it blocks them. Authors have a much weaker
incentive to promptly address problems raised during audit.
- Authors can ask reviewers to apply and verify fixes before they are
published.
- Authors can easily pursue feedback early, and get course corrections on
approach or direction.
- Reviewers are better prepared to support a given change once it is in
@ -54,7 +70,7 @@ a blocking step into the process and generally wastes developer time that could
be better spent developing. This is less true than it appears, because the costs
are low and pay for themselves in other ways:
- Differential is fast and provides a very lightweight process for submitting
- Differential is fast and provides a lightweight process for submitting
code for review and for performing review.
- Authors are free to pursue other changes while code is being reviewed. With
appropriate change management (like local branching in Git) they can even
@ -87,13 +103,15 @@ are low and pay for themselves in other ways:
- With `arc patch`, it is roughly as easy to pull a change out of Differential
as it is to pull it out of the remote.
= Advantages of Audit =
Advantages of Audit
===================
Post-push review is significantly better than nothing. If you are unpersuaded
Post-publish audit is a less powerful workflow than pre-publish review, but can
supplement review and is better than nothing on its own. If you are unpersuaded
by the arguments above (or work on a team that is unswayed), audits provide
some of the benefits of review with less friction:
- Audits are driven entirely by Phabricator, users do not need to install
- Audits are driven entirely by Phabricator: users do not need to install
`arc`.
- Audits require little adjustment to existing workflows and little training.
- Audits are completely nonblocking, and send fewer notifications than review.
@ -101,7 +119,8 @@ some of the benefits of review with less friction:
on lower-importance changes or raise issues that are discovered after
review.
= Recommendations =
Recommendations
===============
Here are super biased recommendations from developers of code review software:
@ -117,7 +136,8 @@ Here are super biased recommendations from developers of code review software:
- If you aren't interested in review, just do audits. You can always
change your mind later. But consider review! It's really good, we promise!
= Next Steps =
Next Steps
==========
- Learn more about reviews in @{article:Differential User Guide}; or
- learn more about audits in @{article:Audit User Guide}.