Skip to content

Pull Request Checklist

Jérémie Bresson edited this page Aug 31, 2018 · 6 revisions

General

  • be polite
  • encourage contributors (especially new ones)
  • always give a lot of contexts

Checks

  • No merge issue
  • Ensure up-to-date scripts
  • Author of the PR is linked to a GitHub account
  • Ensure the Technical Committee is on CC

Target branch

Depending if this is a breaking change, the target branch needs to be different.

TODO: add more details about this.

Review

It should be clear what the PR is changing (link to an issue, explanation, small OpenAPI Spec to reproduce the issue).

Try to have a title that summarize the change. Remember that the PR titles are used in the release notes, so it is important to invest some effort. Good titles:

  • Include the generator name, or the domain in brackets [JAXRS-spec], [JAVA][Rest-assured], [C++][Restbed], [docs], [CI]...
  • Include a short summary of the change

The description is also an important aspect of the PR. It should provide more information about what the change brings:

  • If the change introduce a new option, explain the possible values and what is the default
  • If an issue exists that describes the problem, add a link to it.
  • ...

Remember that the PR description will be the first place where developer look when they want to understand the intention behind a change.

Unit tests are always great.

Test locally if needed.

Do not approve changes you do not understand.

If the change is in the common part (is applied for all languages), be extra careful. Ask for a second review.

If necessary, ask the author about his change.

Never merge your own PR

Somebody else needs to approve

Exception to the rule:

  • The PR is only about updating the samples
  • The PR is only about updating the docs
  • The PR is only about adding a unit-test and CI is green

Merge the Pull Request

Use the "Squash and merge" option:

GitHub usage.

This is enforced with per GitHub settings.

The history looks more clean with this option

After merge tasks

  • Ensure the milestone is set
  • Ensure the labels are set correctly

Common CI Build issues.

Helping contributors to have a green CI is important. This section present some how-to to detect common problems

Shippable: ensure up to date

Shippable run this script: bin/utils/ensure-up-to-date. In the logs it start with:

./bin/utils/ensure-up-to-date
# START SCRIPT: ./bin/utils/ensure-up-to-date

On error case:

UNCOMMITTED CHANGES ERROR
There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'

Then you see the result of git diff (can be use full for debugging) and git status

On error case, the scripts ends with:

Please run 'bin/utils/ensure-up-to-date' locally and commit changes (UNCOMMITTED CHANGES ERROR)

The PR needs to be updated so that the samples corresponds to the changes made to the generator. Here is how this can be done:

  • Run mvn clean verify to compile all modules and openapi-generator-cli in particular.
  • Run the scripts to udpate the outdated samples (bin/______.sh, refer to this table to find it).
  • Add the changes as new commit in the PR

Shippable: generate all samples

If the logs end with:

[ERROR] 5 out of 185 scripts failed.

Grep for ERROR!! FAILED TO RUN to see which scripts are failing.