Skip to content

Pull Requests: Review guideline

Armel Soro edited this page Jun 19, 2023 · 2 revisions

Each PR requires one lgtm label. After a reviewer is satisfied with the changes, they will add /lgtm (looks good to me) as a comment to the PR that applies the lgtm label. Optionally, approving the PR via GitHub review also adds lgtm label to the PR. A reviewer is responsible for checking the code and functional parts of the changes.

Once the PR has the lgtm label and the required tests pass, an internal bot will automatically merge the PR.

Following are some tips you can take into account while reviewing a pull request:

  • Refer to our coding conventions guide documented on this page
  • Look for holistic acceptance criteria, including dependencies with other features, forward and backward compatibility, API and flag definitions, etc. In essence, the high levels of design.
  • Look for general code quality, correctness, sane software engineering, style, etc. In essence, the quality of the actual code itself.
  • Ensure that necessary tests have been added.
  • Ensure that the feature or fix works locally.
  • Check if the code is understandable, and has comments been added to it.
  • Check if the PR passes all the pre-submit tests, and all the requested changes are resolved.
  • As a reviewer, if you apply the /lgtm label before it meets all the necessary criteria, you can put it on hold with the /hold label immediately. You can use /lgtm cancel to cancel your /lgtm and use /hold cancel once you are ready to approve it. This especially applies to draft PRs.
  • Avoid merging the PR manually unless it is an emergency, and you have the required permissions. Prow’s tide component automatically merges the PR once all the conditions are met.

Prow

odo uses both the IBM Cloud infrastructure and Prow infrastructure for CI testing. Use the command-help to see the list of possible bot commands.

Prow uses OWNERS files to determine who can approve and lgtm a PR. It also ensures that post-submit tests (tests that run before merge) validate the PR.