User Tools

Site Tools


reviewing_patches

Peer-reveiwing is enforced in IoTivity. That means each and every change to the source code must be reviewed by another person and must be approved by the appropriate authoritative person. Exceptions are must be rare and must be justified.

The tool for reviewing changes is Gerrit, which you can access by going to https://gerrit.iotivity.org.

Code review process

Set up

In order to review changes, you need to log in to https://gerrit.iotivity.org with your Linux Foundation account. Anonymous users are allowed to view changes but may not provide comments or reviews.

To submit changes for review, please consult the setup documentation to create your SSH keys, upload them to the Gerrit server, create Git commits and push them to the review tool.

Guidelines for reviewing

The IoTivity Project governance outlines the general guidelines on reviewing. All reviewers are required to follow those guidelines when providing comments.

In addition to the general guidelines, reviewers must keep in mind a few more details:

Reviewing a patch

A patch is reviewed on the Gerrit Web UI by providing comments and by providing review scores.

Comments may be inserted for the whole changeset, per file or even on a specific line. To do reviews on a file or a line, click the file name on the display or the type of diff you prefer to read, then double-click the line or the file header. An edit box will open up.

All comments done per file or on a given line are in draft state until you provide a review to the full changeset. Once you click Publish, the comment is made public.

A review may also contain a Code Review score and a Verified score. The possible Code Review levels are:

  • -2: this patch, as it stands, cannot be accepted
  • -1: this patch could use improvements, but it is ok if it is merged as is
  • 0: neutral opinion, just making comments
  • +1: this patch looks good to me, but someone else needs to approve it
  • +2: this patch is approved

Whenever giving negative scores, it's important to provide constructive feedback on what needs improvement or why a given change cannot be accepted. Contributors cannot learn from reviews unless they are given this information.

Note that only IoTivity Project Maintainers and their sub-maintainers can give reviews of -2 and +2. All other developers may give reviews of -1 and +1.

In addition to the Code Review, every patch can receive the following Verified scores:

  • -1: verified not to work (breaks the build, fails unit test, etc.)
  • 0: not verified yet
  • +1: verified to work

The Verified review is provided automatically by the infrastructure. You should not need to provide a Verified vote, unless you have strong reason to revert the automatic score. If you need to do that, please leave a comment with the reason why it was necessary.

A patch will be merged on the following conditions:

  • The patch got at least one “+2” score and no “-2” score in the Code Review category.
  • The patch got at least one “+1” score in the Verified category.

When a patch meets all the criteria above, maintainers and sub-maintainers can to click the Submit and Merge button to merge the patch into the Gerrit repository.

Approving a change

The IoTivity Project Governance allows any person to provide feedback. All that is necessary is that you create an account. Any person with an account can provide constructive comments and provide +1 and -1 Code Review scores. Those reviews are non-binding.

The binding review for a given change must come from the appropriate IoTivity Project Maintainer or a delegated sub-maintainer. The list of Maintainers and sub-maintainers can be found on the IoTivity Projects and Functions page.

In case a commit contains changes that affect multiple Projects, all of the Maintainers, delegated sub-maintainers or known experts need to review the change. In that case, the commit needs at least the +1 from the experts if a +2 could not be obtained for all affected Projects.

Review times

The review time does not remove the need to obtain the approval from the correct Maintainer (see above): a change stays open at least until the Maintainer approves it. Maintainers are encouraged to leave a contribution open for review until others have had the chance to provide feedback.

There's no hard requirement on how long a review must be open for review, so long as enough time is given to the reviewers to act. The time required should be proportional to the complexity of the change.

That is to say small, trivial changes can be approved and submitted in just a couple of hours or even minutes. Medium-sized changes should wait for a day or two so other reviewers have the time to look at the contribution. Large reviews may end up open for months as the discussion on the implementation happens in Gerrit, in JIRA and in the IoTivity mailing lists.

Contributors are advised to take the review time into account when preparing their submissions.

Reverting changes

Once a change has been submitted to the source code, the review is closed, but Gerrit still allows the use of the “Revert” button. When that is used, Gerrit will create a change containing the negation of the previously accepted commit.

Reversals should be rare. Even if a change got accepted by mistake or in violation of the rules, IoTivity contributors should instead seek to provide a fix for any problems introduced. Reverting a change should happen only in cases where the commit is irrevocably incorrect or, in the opinion of the Maintainer, is harmful to the Project.

Note that violations of the rules may be sanctioned in other ways.

Exceptions

The IoTivity review rules should be followed whenever possible. The following is a list of expected conditions under which the rules may be bent:

  • Self-approval: Maintainers and sub-maintainers may only approve their own changes if the relevant Maintainer or another sub-maintainer could not be located in time. Even then, the author of the change should seek at least a +1 from an experienced developer.
  • Approval from Maintainer from another Project: similar to the above, the Maintainer or a sub-maintainer from a different Project may approve a contribution outside of his/her area if the relevant Maintainer cannot be located (e.g. vacation). This should only be done for small or trivial changes; anything with complexity needs to wait for the correct Maintainer for however long it takes.

If in doubt, please ask the IoTivity main development mailing list for more information. Further exceptions may be granted on a case-by-case basis by the development community.

reviewing_patches.txt · Last modified: 2014/11/26 00:19 by Thiago Macieira