PCM Development/ReviewProcess

Overview

• Situation: peer groups that assign reviews to each other
• Goal: distribute reviews of pull requests over people in the group to spread knowledge

Requirements

• timely reaction to review requests
  • average time for first reaction should be 2 days / check GitHub regularly for assigned reviews
  • goal: final review within 1 week
  • exception handling allowed e.g. in case of high work balance
• requester asks if review is not done within expected timewindow
• reviewer assignment
  • if you are not very deep into the topic, assign a reviewer that is
  • if you are very knowledgeable about a topic, assign someone that is not
  • pool of people to choose reviewer: PCM_Development/Process

Review Process

• when JIRA task is finished the JIRA task owner
  • triggers pull request and assigns it to a dedicated reviewer (reviever should be an experienced developer in this domain)
  • resolves the JIRA ticket (status: resolved)
• reviewer receives an email notification from github about the review
• reviewer conducts the review within X days; if passed will execute the merge
• github will send an email notification to the JIRA task owner to inform him about the successful pull request
• JIRA task owner can now close the ticket (status: closed)

Reviewing Pull Requests

Sketch of typical stepts for reviewing a pull request (PR) for a change in Palladio (i.e., Eclipse-based). GitHub also has documentation on reviewing proposed changes in a pull request.

The example pull request used here is PR 35 on Palladio-Core-PCM. It is associated with the issue PALLADIO-539.

Prerequisite

• Set up a PCM development environment. It makes sense to install from nightly and only checkout the relevant projects. If you already have an installation, please update from the nightly update site before continuing.
• Clone the repository the PR changes and checkout the associated branch. It is given on the page of the pull request, usually it is named after the issue.
  • In the example PR, the branch is named PALLADIO-539. The command for checking out the branch is then git checkout PALLADIO-539.
  • Note: There might be multiple projects associated with the same issue resulting in multiple pull requests that have to be considered at once (or in a sensible order). The JIRA issue should find and list all pull requests associated with it. For example, PALLADIO-503 lists 10 associated pull requests.
  • Note: You can also check out the pull request into a new branch locally (either via git or GitHub CLI), if this is more convenient for you (for example, if the PR origin is in a fork of the project). See [1] for more information. In this example, this would result in the commands git fetch origin pull/35/head:PR-35 && git checkout PR-35
• Import the (relevant) projects from the project via File -> Import... -> Existing projects into workspace. Usually it makes sense to check out the code projects under bundles/ that are relevant to the PR and all tests under tests/.
  • The relevant/changed projects can be found via the PR.

Trying out changes

This depends on the issue at hand, but it usually makes sense to start an inner Eclipse instance. In this Eclipse, try to create models, use editors, ..., to find out whether the new behavior now matches the described behavior in the issue. In our example, the changes do not directly and obviously manifest in the editors, so it makes sense to examine and run the tests.

Review code

• Look through the changed code and identify bad/incomprehensible code or other possible violations against good coding practice
• Annotate changes in the pull request and decide on accepting or requesting changes.
• Examine and run the added test cases (as JUnit or Plug-In Tests) from your outer Eclipse instance (look at the pull request to identify the added test cases)

Merging the PR

• If you accept the request, you can merge the pull request. There are different options for doing this and all can be sensible depending on the situation. As a rough guideline:
  • Usually, rebasing the changes is the preferred way.
  • If there are a lot of small confusing commits that make more sense as one large commit, it can also make sense to squash the changes.
• When in doubt, talk to the person that made the changes to decide on how to merge the PR.