mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-23 15:22:41 +01:00
Provide documentation about Audit, Differential, and audit vs review
Summary: Try to explain how this stuff works a little better. Let me know what's unclear / missing / not good. Test Plan: Generated documentation, read documentation. Reviewers: btrahan, gschmidt Reviewed By: btrahan CC: aran, epriestley, davidreuss Maniphest Tasks: T904 Differential Revision: https://secure.phabricator.com/D1969
This commit is contained in:
parent
6d631577f2
commit
821f580c3a
5 changed files with 377 additions and 0 deletions
59
src/docs/feedback.diviner
Normal file
59
src/docs/feedback.diviner
Normal file
|
@ -0,0 +1,59 @@
|
||||||
|
@title Give Feedback! Get Support!
|
||||||
|
@group intro
|
||||||
|
|
||||||
|
How to give us feedback, report bugs, and request features, and get support for
|
||||||
|
problems with Phabricator.
|
||||||
|
|
||||||
|
= Overview =
|
||||||
|
|
||||||
|
We'd love to hear your feedback about Phabricator, whether it's good or bad. We
|
||||||
|
stay on top of bug reports and fix many of them within a day or two (and
|
||||||
|
sometimes within hours). The Phabricator roadmap is determined in large part by
|
||||||
|
user feedback and feature requests. Your feedback matters, will often have an
|
||||||
|
immediate short-term impact, and the project leads are actively listening to it.
|
||||||
|
|
||||||
|
We also try to provide a very high level of free support. If you have trouble
|
||||||
|
with anything or just don't understand how something works, ask us! We're happy
|
||||||
|
to help, and it's usually valuable for us because we can prevent the problem
|
||||||
|
in the code (or document it better) so future users don't hit it.
|
||||||
|
|
||||||
|
Some day we will no doubt grow callous and distant, but for now the community
|
||||||
|
is small enough that we can provide a high level of service and support to
|
||||||
|
everyone and still have plenty of time to write code.
|
||||||
|
|
||||||
|
If you're in the SF bay area, we're also happy to come onsite and help you set
|
||||||
|
things up, answer any questions you might have, or just hang out and tell
|
||||||
|
Facebook war stories.
|
||||||
|
|
||||||
|
The best ways to provide feedback are:
|
||||||
|
|
||||||
|
= Maniphest =
|
||||||
|
|
||||||
|
The best way to report bugs and request features is through
|
||||||
|
[[http://secure.phabricator.com/maniphest/task/create/ | Maniphest]]. Just file
|
||||||
|
the bug/request and we'll handle everything else. (If it's time-sensitive or
|
||||||
|
blocking you, feel free to assign it to `epriestley`.) Feel free to file support
|
||||||
|
requests, general questions, or random feedback this way, too.
|
||||||
|
|
||||||
|
= GitHub Issues =
|
||||||
|
|
||||||
|
You can also use
|
||||||
|
[[https://github.com/facebook/phabricator/issues/new | GitHub Issues]] if you
|
||||||
|
prefer.
|
||||||
|
|
||||||
|
= IRC =
|
||||||
|
|
||||||
|
We're active in #phabricator on FreeNode, and it's the best place to ask
|
||||||
|
questions and get support.
|
||||||
|
|
||||||
|
= Email =
|
||||||
|
|
||||||
|
You can email us at `btrahan@phacility.com` and `epriestley@phacility.com`.
|
||||||
|
|
||||||
|
= Next Steps =
|
||||||
|
|
||||||
|
Continue by:
|
||||||
|
|
||||||
|
- Filing a bug of feature request in
|
||||||
|
[[http://secure.phabricator.com/maniphest/task/create/ | Maniphest]]; or
|
||||||
|
- contributing to Phabricator with @{article:Contibutor Introduction}.
|
107
src/docs/userguide/audit.diviner
Normal file
107
src/docs/userguide/audit.diviner
Normal file
|
@ -0,0 +1,107 @@
|
||||||
|
@title Audit User Guide
|
||||||
|
@group userguide
|
||||||
|
|
||||||
|
Guide to the Audit (post-push code review) tool and workflow.
|
||||||
|
|
||||||
|
= Overview =
|
||||||
|
|
||||||
|
Phabricator supports two code review workflows, "review" (pre-push) and
|
||||||
|
"audit" (post-push). 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.
|
||||||
|
|
||||||
|
NOTE: The audit workflow is new, give us feedback about it! See
|
||||||
|
@{article:Give Feedback! Get Support!}.
|
||||||
|
|
||||||
|
= 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:
|
||||||
|
|
||||||
|
- **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).
|
||||||
|
|
||||||
|
In the Audit tool's home screen (at `/audit/`) and on the homepage you can see
|
||||||
|
commits and requests that require your action:
|
||||||
|
|
||||||
|
- **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.
|
||||||
|
|
||||||
|
For example:
|
||||||
|
|
||||||
|
- 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.
|
||||||
|
|
||||||
|
= Audit Triggers =
|
||||||
|
|
||||||
|
Audit requests can be triggered in a number of ways:
|
||||||
|
|
||||||
|
- 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).
|
||||||
|
|
||||||
|
= 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:
|
||||||
|
|
||||||
|
- Create a new Project, "Code Audits".
|
||||||
|
- Create a new global Herald rule for Commits, which triggers an audit by
|
||||||
|
the "Code Audits" project for every commit where "Differential Revision"
|
||||||
|
"does not exist" (this will allow you to transition partly or fully to
|
||||||
|
review later if you want).
|
||||||
|
- Have every engineer join the "Code Audits" project.
|
||||||
|
|
||||||
|
This way, everyone will see an audit request for every commit, but it will be
|
||||||
|
dismissed if anyone approves it. Effectively, this enforces the rule "every
|
||||||
|
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 =
|
||||||
|
|
||||||
|
- When viewing a commit, audit requests you are responsible for are
|
||||||
|
highlighted. You are responsible for a request if it's a user request
|
||||||
|
and you're that user, or if it's a project request and you're a member
|
||||||
|
of the project, or if it's a package request and you're a package owner.
|
||||||
|
Any action you take will update the state of all the requests you're
|
||||||
|
responsible for.
|
||||||
|
- You can leave inline comments by clicking the line numbers in the diff.
|
||||||
|
- You can leave a comment across multiple lines by dragging across the line
|
||||||
|
numbers.
|
||||||
|
- Inline comments are initially saved as drafts. They are not submitted until
|
||||||
|
you submit a comment at the bottom of the page.
|
||||||
|
- Press "?" to view keyboard shortcuts.
|
||||||
|
|
||||||
|
= Next Steps =
|
||||||
|
|
||||||
|
- Learn more about Herald at @{article:Herald User Guide}.
|
68
src/docs/userguide/differential.diviner
Normal file
68
src/docs/userguide/differential.diviner
Normal file
|
@ -0,0 +1,68 @@
|
||||||
|
@title Differential User Guide
|
||||||
|
@group userguide
|
||||||
|
|
||||||
|
Guide to the Differential (pre-push code review) tool and workflow.
|
||||||
|
|
||||||
|
= Overview =
|
||||||
|
|
||||||
|
Phabricator supports two code review workflows, "review" (pre-push) and
|
||||||
|
"audit" (post-push). To understand the differences between the two, see
|
||||||
|
@{article:User Guide: Review vs Audit}.
|
||||||
|
|
||||||
|
This document summarizes the pre-push "review" workflow implemented by the tool
|
||||||
|
//Differential//.
|
||||||
|
|
||||||
|
= How Review Works =
|
||||||
|
|
||||||
|
Code review in Phabricator is a lightweight, asynchronous web-based process. If
|
||||||
|
you are familiar with GitHub, it is similar to how pull requests work:
|
||||||
|
|
||||||
|
- An author prepares a change to a codebase, then sends it for review. They
|
||||||
|
specify who they want to review it (additional users may be notified as
|
||||||
|
well, see below). The change itself is called a "Differential Revision".
|
||||||
|
- The reviewers receive an email asking them to review the change.
|
||||||
|
- The reviewers inspect the change and either discuss it, approve it, or
|
||||||
|
request changes (e.g., if they identify problems or bugs).
|
||||||
|
- In response to feedback, the author may update the change (e.g., fixing
|
||||||
|
the bugs or addressing the problems).
|
||||||
|
- Once everything is satisfied, some reviewer accepts the change and the
|
||||||
|
author pushes it to the upstream.
|
||||||
|
|
||||||
|
The Differential home screen shows two sets of revisions:
|
||||||
|
|
||||||
|
- **Action Required** is revisions you are the author of or a reviewer for,
|
||||||
|
which you need to review, revise, or push.
|
||||||
|
- **Waiting on Others** is revisions you are the author of or a reviewer for,
|
||||||
|
which someone else needs to review, revise, or push.
|
||||||
|
|
||||||
|
= Creating Revisions =
|
||||||
|
|
||||||
|
The preferred way to create revisions in Differential is with `arc`
|
||||||
|
(see @{article:Arcanist User Guide}). You can also create revisions from the
|
||||||
|
web interface, by navigating to Differential, pressing the "Create Revision"
|
||||||
|
button, and pasting a diff in.
|
||||||
|
|
||||||
|
= Herald Rules =
|
||||||
|
|
||||||
|
If you're interested in keeping track of changes to certain parts of a codebase
|
||||||
|
(e.g., maybe changes to a feature or changes in a certain language, or there's
|
||||||
|
just some intern who you don't trust) you can write a Herald rule to
|
||||||
|
automatically CC you on any revisions which match rules (like content, author,
|
||||||
|
files affected, etc.)
|
||||||
|
|
||||||
|
= Differential Tips =
|
||||||
|
|
||||||
|
- You can leave inline comments by clicking the line numbers in the diff.
|
||||||
|
- You can leave a comment across multiple lines by dragging across the line
|
||||||
|
numbers.
|
||||||
|
- Inline comments are initially saved as drafts. They are not submitted until
|
||||||
|
you submit a comment at the bottom of the page.
|
||||||
|
- Press "?" to view keyboard shortcuts.
|
||||||
|
|
||||||
|
= Next Steps =
|
||||||
|
|
||||||
|
- Read the FAQ at @{article:Differential User Guide: FAQ}; or
|
||||||
|
- learn about handling large changesets at
|
||||||
|
@{article:Differential User Guide: Large Changes}; or
|
||||||
|
- learn about test plans at @{article:Differential User Guide: Test Plans}; or
|
||||||
|
- learn more about Herald at @{article:Herald User Guide}.
|
|
@ -40,3 +40,23 @@ If authors are being jerks about this (making sweeping changes as soon as they
|
||||||
get an accept), solve the problem socially by telling them to stop being jerks.
|
get an accept), solve the problem socially by telling them to stop being jerks.
|
||||||
Unless you've configured additional layers of enforcement, there's nothing
|
Unless you've configured additional layers of enforcement, there's nothing
|
||||||
stopping them from silently changing the code before pushing it, anyway.
|
stopping them from silently changing the code before pushing it, anyway.
|
||||||
|
|
||||||
|
= How can I enable syntax highlighting? =
|
||||||
|
|
||||||
|
You need to install and configure **Pygments**. Consult the configuration file
|
||||||
|
for instructions.
|
||||||
|
|
||||||
|
= What do the whitespace options mean? =
|
||||||
|
|
||||||
|
Most of these are pretty straightforward, but "Ignore Most" is not:
|
||||||
|
|
||||||
|
- **Show All**: Show all whitespace.
|
||||||
|
- **Ignore Trailing**: Ignore changes which only affect trailing whitespace.
|
||||||
|
- **Ignore Most**: Ignore changes which only affect leading or trailing
|
||||||
|
whitespace (but not whitespace changes between non-whitespace characters)
|
||||||
|
in files which are not marked as having significant whitespace.
|
||||||
|
In those files, show whitespace changes. By default, Python (.py) and
|
||||||
|
Haskell (.lhs, .hs) are marked as having significant whitespace, but this
|
||||||
|
can be changed in the `differential.whitespace-matters` configuration
|
||||||
|
setting.
|
||||||
|
- **Ignore All**: Ignore all whitespace changes in all files.
|
||||||
|
|
123
src/docs/userguide/reviews_vs_audit.diviner
Normal file
123
src/docs/userguide/reviews_vs_audit.diviner
Normal file
|
@ -0,0 +1,123 @@
|
||||||
|
@title User Guide: Review vs Audit
|
||||||
|
@group userguide
|
||||||
|
|
||||||
|
Discusses the differences between Review and Audit workflows.
|
||||||
|
|
||||||
|
= Overview =
|
||||||
|
|
||||||
|
Phabricator supports two similar but separate code review workflows:
|
||||||
|
|
||||||
|
- **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}.
|
||||||
|
|
||||||
|
(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.)
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
= Advantages of Review =
|
||||||
|
|
||||||
|
Pre-push review is significantly more powerful than post-push auditing. You
|
||||||
|
gain these advantages by requiring review //before// changes may be pushed:
|
||||||
|
|
||||||
|
- Authors have a strong incentive to craft small, well-formed changes that
|
||||||
|
will be readily understood, to explain them adequately, and to provide
|
||||||
|
appropriate test plans, test coverage and context.
|
||||||
|
- 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.
|
||||||
|
- 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.
|
||||||
|
- 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
|
||||||
|
production, having already had a chance to become familiar with and reason
|
||||||
|
through the code.
|
||||||
|
- Reviewers are able to catch problems which automated tests may have
|
||||||
|
difficulty detecting. For example, human reviewers are able to reason about
|
||||||
|
performance problems that tests can easily miss because they run on
|
||||||
|
small datasets and stub out service calls.
|
||||||
|
- Communicating about changes //before// they happen generally leads to better
|
||||||
|
preparation for their effects.
|
||||||
|
|
||||||
|
The theoretical cost of review is that it slows down development by introducing
|
||||||
|
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
|
||||||
|
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
|
||||||
|
pursue dependent changes easily. Authors should rarely if ever be blocked on
|
||||||
|
review, even though an individual change is blocked until it is approved.
|
||||||
|
- The workflow as a whole is lightweight and, with skillful reviewers,
|
||||||
|
effective at identifying bugs. It is generally faster to fix bugs in review
|
||||||
|
than in production.
|
||||||
|
- More importantly, it is effective at identifying problems with architecture
|
||||||
|
and approach. These are free to fix in review ("don't do this, it is a bad
|
||||||
|
idea") and may be very time consuming to fix in production. No matter how
|
||||||
|
good your test suite is, it can't identify solutions which are poor because
|
||||||
|
of missing context, or miscommunication, or which are simply bad ideas.
|
||||||
|
- Changes which are too large or too complicated to be reviewed quickly are
|
||||||
|
often //too large and too complicated, period//. Nearly all large changes
|
||||||
|
can be split apart into small, independent pieces which are easier to
|
||||||
|
understand and test. Review tends to encourage smaller and better-factored
|
||||||
|
changes.
|
||||||
|
- Review can be integrated with static analysis which can detect (and,
|
||||||
|
in many cases, correct) mechanical problems with code like syntax,
|
||||||
|
formatting, naming conventions, style problems, misspellings, and some
|
||||||
|
program errors. This reduces the amount of time it takes to review code,
|
||||||
|
and means reviewers can focus on actual problems with the code rather than
|
||||||
|
minor stylistic issues.
|
||||||
|
- Review creates a permanent record of context and intent which explains why
|
||||||
|
a change was made, generally with much more information than commit messages
|
||||||
|
alone (authors have an incentive to properly explain a change when sending
|
||||||
|
it for review). This makes it easier to understand code later, and to
|
||||||
|
respond appropriately when it breaks.
|
||||||
|
- 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 =
|
||||||
|
|
||||||
|
Post-push review is significantly better than nothing. 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
|
||||||
|
`arc`.
|
||||||
|
- Audits require little adjustment to existing workflows and little training.
|
||||||
|
- Audits are completely nonblocking, and send fewer notifications than review.
|
||||||
|
- Even if you have review, audits can be useful as a supplement to keep tabs
|
||||||
|
on lower-importance changes or raise issues that are discovered after
|
||||||
|
review.
|
||||||
|
|
||||||
|
= Recommendations =
|
||||||
|
|
||||||
|
Here are super biased recommendations from developers of code review software:
|
||||||
|
|
||||||
|
- If you can do review, do it. Supplement it with audits for less important
|
||||||
|
changes as your organization scales.
|
||||||
|
- If you can't do review immediately, set up audits and try to transition
|
||||||
|
toward review. Some types of changes (like tentative changes or requests
|
||||||
|
for feedback about code) are a naturally good fit for review and can serve
|
||||||
|
as a stepping stone toward broader acceptance. Greater familiarity with the
|
||||||
|
toolset may also foster more acceptance toward review, and the value of
|
||||||
|
review may become more obvious as the organization scales (e.g., once you
|
||||||
|
get interns).
|
||||||
|
- 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 =
|
||||||
|
|
||||||
|
- Learn more about reviews in @{article:Differential User Guide}; or
|
||||||
|
- learn more about audits in @{article:Audit User Guide}.
|
Loading…
Reference in a new issue