Skip to content
This repository has been archived by the owner on Dec 31, 2019. It is now read-only.

Latest commit

 

History

History
619 lines (463 loc) · 28.5 KB

README.md

File metadata and controls

619 lines (463 loc) · 28.5 KB
Raw

cr.joyent.us new user instructions

cr.joyent.us is the Joyent Code Review server, running the Gerrit Code Review tool.

Quick overview

Gerrit is a combination of two pieces:

  • a Git server. You submit code for review using git(1), just like you're used to. The main difference is that instead of pushing changes directly to master, you push them to a special reference that creates a new Change inside Gerrit.
  • a web interface. You typically add reviewers, submit feedback, vote on changes, approve changes, and finally integrate changes into master all through the web UI.

There's also a REST API and an ssh-based CLI that support many of the same operations as the web interface.

There are a few other things to know about Gerrit:

  • Gerrit formalizes the notion of a Change and patchset. See the quick start below.
  • The process of "integrating" (or "landing") a change (either by merge, rebase, fast-forward, or cherry-pick) is called submitting it. This might be confusing if you think of "submitting a change for review". In Gerrit, submitting a change refers to integrating it into master.
  • Gerrit enforces that each patchset consists of exactly one commit. This matches our policy for pushes, although we do sometimes review changes that aren't yet squashed. You can still do this with Gerrit! See the FAQ for details.
  • Gerrit also formalizes the notion of approval with a voting system. It uses numeric values, but these don't get added together. See the FAQ for details.

About these docs

In this document:

For more help with the mechanics of using Gerrit, including specific steps or configuration, check out the Gerrit User Guide. We have one major exception to the standard Gerrit workflow: rather than putting Change-Id into commit messages, we have people push new changes to the special refs/changes/... references. This is shown in the examples below.

There's a separate cr.joyent.us operator guide for those interested in helping run cr.joyent.us.

If you want to reach out to a person, contact an administrator. You can list the administrators in Gerrit. The new user instructions below will mention a few places where you need an administrator to take action to get your account set up.

New user instructions

  1. Navigate to https://cr.joyent.us.

  2. Click the "Sign in" link and grant access to our Gerrit GitHub application. This is currently only used for authentication to the Gerrit web UI.

  3. Next, add a public key for ssh access to the Gerrit repositories. Back in the Gerrit web interface, click your name in the top-right, then click Settings. On the left, click "SSH Public Keys" and add an ssh public key.

  4. Next, register and confirm your email address so that you can receive email notifications. From the same screen where you added public keys, click "Contact Information" on the left to register your email address. Follow the instructions to verify your address.

  5. Test out ssh access. Your ssh username should be the same as your GitHub account name. You can confirm this in the Gerrit web UI by clicking the "Profile" link on the left (from the same screen where you added your email address). The "username" field should match your GitHub account name.

    You should be able to log in like this (with your username):

    $ ssh davepacheco@cr.joyent.us

  ****    Welcome to Gerrit Code Review    ****

  Hi David Pacheco, you have successfully connected over SSH.

  Unfortunately, interactive shells are disabled.
  To clone a hosted Git repository, use:

  git clone ssh://davepacheco@cr.joyent.us/REPOSITORY_NAME.git

Connection to cr.joyent.us closed.

    If you have trouble with this, contact one of these administrators.

  6. Have one of these administrators add your account to the appropriate groups inside Gerrit.

Now that you're ready to start using Gerrit, check out the Gerrit User Guide.

Quick start: using Gerrit for code review

These instructions assume you have already followed the "New user instructions" above.

Let's walk through an example. I've got a repository on cr.joyent.us called my-playground. First, I clone it:

dap@sharptooth ~ $ git clone --origin=cr ssh://davepacheco@cr.joyent.us/my-playground.git
Cloning into 'my-playground'...
remote: Counting objects: 2, done
remote: Finding sources: 100% (2/2)
remote: Total 2 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (2/2), done.

Now, let me add a README and commit that:

dap@sharptooth my-playground $ vim README.md
dap@sharptooth my-playground $ git add README.md
dap@sharptooth my-playground $ git commit -m "add initial README"
[master 95a7c80] add initial README
 1 file changed, 1 insertion(+)
 create mode 100644 README.md

Now I'm ready to send this out for review. Gerrit first-classes the idea of a single logical change to the repository, and that's called a Change (of course). In Gerrit, a Change encapsulates multiple revisions of the work as well as the review and automated verification process around the change. I can create a new change from my commit by pushing to the magic branch refs/for/master:

dap@sharptooth my-playground $ git push cr HEAD:refs/for/master
Counting objects: 4, done.
Writing objects: 100% (3/3), 266 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: Processing changes: new: 1, refs: 1, done
remote:
remote: New Changes:
remote:   https://cr.joyent.us/12 add initial README
remote:
To ssh://davepacheco@cr.joyent.us/my-playground.git
 * [new branch]      HEAD -> refs/for/master

Gerrit gave me back a URL that I can use to refer to this Change. It also assigns it a ChangeId. (There's a long ChangeId and a short one. We can generally use the short one. In this case, that's 12.)

Reviewers can be automatically notified and they can leave their feedback in the UI. We won't go through that process here.

Once I've got feedback, I might make some changes to the README:

dap@sharptooth my-playground $ vim README.md
dap@sharptooth my-playground $ git add README.md
dap@sharptooth my-playground $ git commit -m "forgot to add content"
[master f6eb4be] forgot to add content
 1 file changed, 2 insertions(+)

At this point, I want to upload a new revision of the same Change. Gerrit calls each revision a PatchSet. In order to submit it, I need to squash my change:

dap@sharptooth my-playground $ git rebase -i HEAD^^
[detached HEAD 920280c] add initial README; forgot to add content
 1 file changed, 3 insertions(+)
 create mode 100644 README.md
Successfully rebased and updated refs/heads/master.

This will open a file that looks something like this:

pick 95a7c80 add initial README
pick f6eb4be forgot to add content

Change pick to fixup for the second commit like so:

pick 95a7c80 add initial README
fixup f6eb4be forgot to add content

(You can use squash instead if you really do want to keep the second commit message.)

Save and exit. Then I push that to the magical reference refs/changes/12 (because this is a new PatchSet for Change 12):

dap@sharptooth my-playground $ git push cr HEAD:refs/changes/12
Counting objects: 4, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 316 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: Processing changes: updated: 1, refs: 1, done
remote:
remote: Updated Changes:
remote:   https://cr.joyent.us/12 add initial README
remote:
To ssh://davepacheco@cr.joyent.us/my-playground.git
 * [new branch]      HEAD -> refs/changes/12

The Change's URL is the same as it was. Now if you visit it, you'll see the latest patchset by default. You can also view diffs against the previous patchset (or any previous patchset).

When we're satisfied with everything, we're ready to submit the change, which is (somewhat confusingly) the Gerrit terminology for integrating the change into "master". To enable this, the change needs a "+1" vote in the "Integration Approval" category. This is separate from any number of "+1" votes under "Code Review".

Even with the requisite "+1" votes, Gerrit will only allow changes to be integrated that can be fast-forwarded onto "master". If someone else has changed "master" in the meantime, we'll need to update the change to apply cleanly to master: this can be done in Gerrit via "Rebase".

Prior to integration, you will need to update the commit message with the appropriate "Reviewed by" and "Approved by" lines (without any blank lines). The cr-update-reviewers tool can help with this, as can the more comprehensive grr. Alternatively, this can be done with a git commit --amend, but be careful to re-push only this change, or Gerrit will drop the +1 I-A.

Once your change has +1s for both Code Review and Integration Approval, and the commit message is updated, assuming you have the appropriate permissions to integrate the change (see the New User Instructions), then you should see the "Submit change" button show up. Clicking that button will integrate the change into master. The change should be replicated to GitHub within about 30 seconds.

Using cr.joyent.us with an existing local workspace

Suppose you've been working on a project from the GitHub copy, and someone has imported that project into cr.joyent.us, and you want to start using Gerrit for code review. All you need to do is add a new Git remote:

git remote add cr ssh://YOURUSERNAME@cr.joyent.us/PROJECT_GITHUB_ACCOUNT/PROJECT_NAME.git

PROJECT_GITHUB_ACCOUNT is usually joyent. For example, if davepacheco had a local copy of https://github.com/joyent/illumos-joyent, he'd do this:

git remote add cr ssh://davepacheco@cr.joyent.us/joyent/illumos-joyent.git

Now you can use the same instructions from the "quick start" above. Briefly:

# Submit code for review as a NEW change in Gerrit.
git push cr HEAD:refs/for/master

# Submit a new patchset for existing change XYZ in Gerrit
git push cr HEAD:refs/changes/XYZ

See the Gerrit docs for more information on special references like refs/for/... and refs/changes/... rather than refs/for/master.

Importing an existing GitHub repository

Unfortunately, only administrators can usefully create projects, so the easiest thing is to have one of them do this for you.

Once a repository has been imported:

  • All subsequent pushes should go through Gerrit. People should stop pushing to the GitHub copy. Please notify the internal teams by emailing "devs" and notify the external developer community by updating the repo contribution guidelines (boilerplate). Ideally, we would turn off the ability to push to GitHub, but we have not written any tooling or documentation for this yet.
  • Any changes pushed to "master" in Gerrit will be replicated to GitHub.
  • GitHub remains the repository of record for automated builds and other tooling.

Note that while we don't want to allow pushes to GitHub and Gerrit simultaneously, we can easily go back to a Gerrit-less world if we decide to by simply removing the project from Gerrit and re-enabling GitHub push access.

Creating a new repository

By far, the easiest way to create new repositories is to:

  1. Create the repository on GitHub.
  2. Push a commit to the GitHub repository (master branch). This can be the first substantive commit you want in the repo or an empty commit if you want to use code review for the first substantive commit.
  3. Have an administrator import the repository from GitHub.

At this point, the project should be fully functional on Gerrit. Changes that go through code review on Gerrit and are ultimately submitted (integrated into master) will be replicated to GitHub. Changes pushed directly to master on Gerrit (bypassing code review) will also be replicated to GitHub.

Creating Gerrit reviews against a branch

It can be useful to create Gerrit reviews with a base branch other than 'master'. For this to work, the branch has to be pushed into cr.joyent.us. You will need to be a Gerrit admin, or ask one to do the following for you.

If our base branch is my-dev-branch:

  1. Make sure your local check-out of the base branch is identical to that hosted on github (same SHA for HEAD).

  2. Add yourself to "Temporary Git Superusers" via the gerrit admin interface.

  3. Push that base branch directly to cr.joyent.us:

    $ git push cr HEAD:refs/heads/my-dev-branch

  4. Remove yourself from "Temporary Git Superusers".

  5. Now, you can create Gerrit reviews against that branch as needed, by using the magical refs/for/<branchname>:

    $ git checkout my-latest-dev-work
$ git push cr HEAD:refs/for/my-dev-branch

Merging from upstream repositories

The illumos-joyent repository currently merges changes from upstream without review. To do this, the merge process should look exactly the same as before, except that users should push directly to "cr.joyent.us". If you've used the above instructions to add a remote for cr.joyent.us, then it looks like this:

$ git push cr master

Or, to configure "git push" of master to always push to cr.joyent.us:

$ git push -u cr master

and then subsequent pushes can just use git push.

In order to do this, your cr.joyent.us user has to have an additional privilege, which is the ability to push merge commits to this repository. Talk to an administrator to set this up.

If you need to do this for any repository other than illumos-joyent, talk to dap to make sure your use-case is covered.

FAQ

What do I have to do right now?

If you work on any Triton or Manta repository:

  1. If you have not yet done so, please read RFD 45.
  2. Follow the new user instructions at the top of this document.
  3. For any Triton or Manta repository that you work on, pay attention for any notice that it's been imported into Gerrit. Once that's happened, stop pushing to GitHub and follow the above docs for "Using Gerrit with an existing workspace".
  4. For any Triton or Manta repository that you are responsible for, when you have some time, please follow the instructions above for importing that repository into Gerrit. Feel free to pick up other repos as well, but if there's someone else that ought to be involved (i.e., someone else that tends to be responsible for that repo), please coordinate with them.

Which repositories are going to use cr.joyent.us?

We're planning on using cr.joyent.us for code review on all repositories that directly make up Triton and Manta. That would at least include the repositories on github.com/joyent that start with "sdc-" or "manta-", plus node modules and other repositories that are obviously tied to Triton and Manta (e.g., "node-sdc-clients", "moray").

We can easily add support for other repositories, including components like ContainerPilot and general-purpose modules that happen to be included in Triton or Manta (e.g., node-jsprim). That's up to project owners at this point.

Which repositories have already been transitioned to cr.joyent.us?

See the full list of repositories that live on cr.joyent.us.

After a repository has been imported into cr.joyent.us, where's the repository of record? Is it Gerrit or GitHub?

GitHub remains the repository of record. Automated builds and other tooling (e.g., searching the code base) can still use GitHub. You can clone repositories from either Gerrit or GitHub. They should always be in sync. (The nice thing about this is that if we decide to abandon Gerrit for whatever reason, we can do that very easily.)

All pushes should go through the Gerrit server, even when bypassing code review. Remember, Gerrit is a full-fledged Git server, and even if we decide to skip code review (e.g., for very early projects or for merges from upstream), we still want to push to Gerrit so that the changes are reflected there and then replicated to GitHub.

What do we do if someone accidentally pushes to GitHub instead of Gerrit?

If someone pushes to GitHub, changes cannot be replicated from Gerrit to GitHub until the Gerrit copy is updated with that change.

Gerrit will need to be updated with whatever was pushed to GitHub. As long as nobody has integrated a change from inside Gerrit or pushed to Gerrit's "master" directly, this is easy: just push the current GitHub "master" to cr.joyent.us. Currently, only administrators have privileges to do this, but that's just to minimize accidental pushes to "master".

If GitHub and Gerrit have explicitly diverged (i.e., there are commits in both places that are not present in the other), we'll likely have to force-push the Gerrit master to match the GitHub master and then re-apply whatever changes had been integrated via Gerrit. Obviously, we'd rather avoid this.

Can we enforce that people don't accidentally push to GitHub instead of Gerrit?

Help is needed here. GitHub appears to support restricting pushes, but most of the engineering team are effectively GitHub superusers for the Joyent organization, which bypasses all such restrictions. It would be great if we could investigate how to manage GitHub access control so that people still have the permissions they need to do everyday work (including creating new repos), but we can still enforce push restrictions to avoid the above problem.

What are the exact policies around code review and voting?

To encourage discussion about test plan, risk analysis, and other non-code considerations in additional to regular code review, our Gerrit is configured to provide separate scores for code review and change approval.

These two scores come in the form of Gerrit "labels", which can be likened to dimensions along which the change is measured. Reviewers may vote in each dimension separately as allowed by their user rights. Note that this is different to the standard Gerrit configuration as described on much of the Internet - particularly, we do not have any "+2" scores.

The first of these two labels is Code-Review. This dimension is used to record your satisfaction with the quality of the code and design in the change, and your confidence in its correctness. Votes under this label can take the values -1, 0 or +1:

  • A "-1" vote (Should not integrate as-is) is a vote against a change as it stands: without amendment you are not happy to see this change integrated.
  • A "0" vote (No score yet) can either mean that you have not yet reviewed the change, or can be used for minor questions or suggestions that you have which should not block integration.
  • A "+1" vote (Looks good to me) indicates that the change has your full confidence and is ready to be considered for approval as-is.

The second label is named Integration-Approval. Only users who have been added to the Change Approver Role may vote on this label (though at present this includes all Joyent staff). It is used to indicate that you have reviewed the process and risk around this change and assessed it as ready to integrate. In particular, some of the factors that may be involved in deciding on an appropriate Integration-Approval vote:

  • Are there any outstanding -1 Code-Review scores whose concerns have not been dealt with? Any reviewers who were added and never replied?
  • Did the proposer or reviewers of the change test it to your satisfaction? (This may not be applicable to all changes)
  • Does the list of reviewers include everyone you think should be consulted on this change? (Is there an established owner or expert in this subsystem?)
  • Is this change too great a risk to a critical subsystem for the improvement it embodies?
  • Anything else that is not considered directly in Code-Review itself (this is not an exhaustive list), especially if it's a concern from the maintainer's perspective rather than the developer's.

Votes under the Integration-Approval label can take the values -1, 0, or +1:

  • A "-1" vote (Rejected/Veto) should be rarely used, but constitutes a maintainer veto on the change. Such a vote blocks integration of the change permanently unless the voter later rescinds their decision.
  • A "0" vote (Not assessed yet) is used either to indicate that the change has not yet been assessed for Integration-Approval, or used by a potential Approver to ask a question or provide minor comments.
  • A "+1" vote (Approved, ready to integrate) approves the change and marks it ready for integration.

Note that actually integrating the change is a separate step to the +1 Integration-Approval vote. This step can be taken by any member of the Change Approver Role. It is considered acceptable in many cases for the approver to also be a code reviewer for the change.

Any "-1" votes in either dimension are "sticky" - i.e. if the change is amended (a new patchset is uploaded), the vote still affects the new patchset until revisited by the original voter.

Any "+1" votes are not sticky (except where the amendment is trivial), and must be reapplied after changes. Trivial amendments include a change to the commit message or a clean rebase.

For a change to be able to be integrated, it must receive one current "+1" vote in both the Code-Review and Integration-Approval dimensions, and no "-1" votes in the Integration-Approval dimension. A "-1" vote in Code-Review does not block an otherwise approved change from being integrated, but any "-1" in Integration-Approval does (even if, for example, there is a later "+1" in Integration-Approval).

In general, we consider this policy to be the formalization of what is existing best practice at Joyent. We're not looking to make significant changes to how review is done. The basic expectation is that all new code changes for Triton and Manta get reviewed by at least one other person. Any member of the engineering team (and any community members to whom we delegate access) can act in the role of Change Approver - and Approving your own change is justified in some circumstances. But the expectation is that people will seek out feedback from others (even if there's nobody else who's very familiar with the code).

How will we handle changes that need to be kept secret (e.g., security fixes)?

This case is extremely rare, but it does come up. We have not tried this out yet, but there are two options:

  1. Gerrit supports draft reviews, which are not visible by default. We can keep these hidden until they're finally integrated.
  2. For these rare cases only, people could use some other code review mechanism (e.g., webrevs in Manta) and then push the change directly to master when it's ready to be opened up.

If Gerrit requires a single commmit per change, how can I maintain multiple local commits?

If you want to keep local commits solely to keep track of what you've sent out for review, then use patchsets for that. Reviewers can diff any pair of patchsets, and people can also "git checkout" any patchset.

It's pretty common that people want to keep lots of local commits separately from what they submit to Gerrit. You can do this, too. It's just that in order to push a new patchset, you have to create a single, squashed commit and push that to cr.joyent.us. (You can keep all the other commits, though!)

The most robust way to do this is to:

  1. Make sure all of your changes are committed to your working branch.
  2. Checkout a new, temporary branch based on the upstream master.
  3. Cherry-pick all of your commits onto this branch.
  4. Squash all of these commits.
  5. Push the squashed commit to Gerrit.
  6. Checkout your working branch again.

At the end of this, you'll have pushed a single, squashed commit representing all of your work, but your working directory will still be on the branch that has all of your smaller commits.

Let's say you're keeping all of your commits on a local branch called WORKING_BRANCH. (This can be "master" or any other branch.) When you want to submit a new patchset:

# Make sure that you're on WORKING_BRANCH and that you've got no
# uncommitted changes.
$ git status

# Check out a temporary branch to store the squashed commit.  This branch
# should be based on the upstream master.  You can call this branch
# whatever you like.
$ git checkout -b TEMP_BRANCH cr/master

# Apply your local commits to this branch.
$ git cherry-pick TEMP_BRANCH..WORKING_BRANCH

# Squash all of the changes with "git rebase".
$ git rebase -i cr/master

# Push the new patchset for review (see above for details)
$ git push cr HEAD:refs/changes/XYZ

# Switch back to your working branch.
$ git checkout WORKING_BRANCH

grr makes this approach very simple.

As long as you started with no uncommitted changes, this procedure will never lose any data, and you'll wind up in the same state you started: on your working branch with all of your local commits.

At this point, you can delete TEMP_BRANCH if you want, or you can use it for the next patchset by resetting it with git reset --hard cr/master.

Who can become an administrator?

Anyone can sign up to help run cr.joyent.us, but you should not need administrative privileges to do anything that's part of the normal workflow except for importing a new project. We've set up relatively fine-grained access control for most other operations. As an administrator, the safeties are off, and you can easily end up doing the wrong thing (e.g., pushing to master and bypassing code review). For that reason, we're trying to keep the group as small as we can.

If you do want to sign up to help run cr.joyent.us as an administrator, check out the operator guide first and then talk to an existing administrator to be added to the group.