Page MenuHomePhabricator

Audit approval flow needs more actions
Closed, DuplicatePublic

Description

When a commit is published, we have a Herald rule that creates an Audit task and assigns relevant group of people to be auditors.
For the commit to be approved, all auditors need to approve it.
Sometimes it's enough for one member of auditors group to make a code review and approve the commit.
So I suggest twp separate actions:

  1. "Approve commit" - will close the audit task.
  2. "Looks good, someone else need to approve" - will keep the audit open.

Event Timeline

alnet82 raised the priority of this task from to Needs Triage.
alnet82 updated the task description. (Show Details)
alnet82 added a project: Audit.
alnet82 added a subscriber: alnet82.

This looks identical to T731 except that it refers to audits, not revisions.

@alnet82 do you only use Audit, or is there a pre-commit workflow you use as well?

@eadler - yes T731 looks similar.
@chad, we use only Audits at this point. The reason we don't use pre-commit workflow is mainly due the fact that we didn't want to introduce a new Source Code Control system, besides what we already use - mercurial. Our team works on many platforms and each developer uses its own IDE, so at this point we didn't want to convert everyone to Arcanist.
In our current flow everyone that want his commits to be reviewed, uploads them to a "review" branch. After a successful review, the branch is merged with the master branch.
Additional thing I find lacking is a way to compare ("diff") a series of commits. Think of a following flow:

  1. Alice pushes commit A and sets Bob as Auditor.
  2. Bob comments on commit A and rejects the commit.
  3. Alice pushes commit B to address Bob's comments.

At this point to help Bob reviewing a code several things are missing:

  1. There is no connection between commits A and B. It could be nice if some reserved keyword in commit B would refer to commit A, so when you look into one of the commits you can see relation to the other.
  2. A way to see not only the current commit diff, but a diff between a series of commits: A-->B, Original_Head-->B, etc'.
  3. When approving commit B, also approving commits that relate to it.

Basically everything you want is in Differential.

Just a few quick pointers:

  • You are not required to use Arcanist to use Differential (you can manually upload diffs)
  • It is not hard to use Arcanist (arc branch, arc diff, arc land is basically all I use)
  • Arcanist has a lint/unit pipeline which reduces code review time (catching errors, formatting nits)

We're feel pretty strongly that Arcanist is a superior workflow and engineering marvel. It sounds like some people are using it?

@chad, thanks for the comments. There is no doubt that Differential mode is a proffered way to do a code review.
However it has some issues:

  1. Using manually uploaded diffs is nice, but not an option for a daily routine.
  2. While Arcanist is not hard to use, it requires though some magic to make it work with PyCharm, Eclispe or any other IDE being used. It should be convenient for people to use and hidden as much as possible. If you could just replace "arc" with "hg" that would be a perfect match.

T5000 has a bit of info in providing "review" workflows without arc (specifically, building a review from a branch in diffusion). Stuff like that though is far off our timeline. It's just a feature very few installs need.

Well its closer now than it was a year ago.

eadler added a project: Restricted Project.Jan 8 2016, 10:15 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jan 8 2016, 10:18 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Feb 18 2016, 6:32 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 13 2016, 9:18 PM