Pull Request Process

#1

Before opening a PR

  • Provide a description of the changes and link to the Jira issue, filling out the PR comment template
  • For significant changes, point out where the focus of the review should be
  • Provide notes on how the change was tested
  • Provide screenshots if applicable (especially for GUI changes)
  • Choose at least one reviewer for the PR and request their review

After opening a PR

  • Address any feedback received and re-request a review from the originator of the feedback once the changes have been pushed
  • Wait until merge criteria is met, then the author should merge the PR if they are able to merge, otherwise request a reviewer to merge

Merge criteria

  • Open for at least 3 business days
  • PR has been “approved” by all reviewers

Approving reviews

  • A reviewer should use ‘request changes’ for their review if they have feedback they want addressed and then approve once feedback is addressed
  • If all looks good, use ‘approve’
2 Likes
#2

Inline with the PR process, here are a few things I look for when reviewing:

General

  1. Is there a corresponding JIRA issue? (Not required for trivial changes)
  2. Does the target branch match what’s identified as the fix versions in the JIRA issue?
    • People often get this wrong
  3. Weigh risk/reward
    • What could happen if we don’t merge this PR?
    • What could happen if we do merge this PR?

Testing

  1. Does the code have any unit, integration or system test coverage?

Code quality

  1. Indentation and code style should be consistent with existing code when editing.

Security

  1. Are the functions limited to the right roles?
  2. Does the change introduce any new possible attack vectors?
    • When displaying new content via JSP, ensure to test for XSS

Licensing

  1. Do not add any GPL or AGPLv3 licensed libraries code to the build.
1 Like
#3

I am bit torn by the 1 business day limitation. We are spread through various time zones and if timing is bad, people from other time zones don’t get the chance to review things. this may work for trivial changes, but we should increase the minimum amount of days open maybe to 3 days.

In general we will probably want to have this more “dynamic” than “static”. There is always an exception for a rule. The creator may just suggests a certain amount of days, until it will be merged. Where a default is provided by us.

#4

I agree, one day is too short for a lot of the changes we see. Maybe we just state “has been open for a suitable amount of time at the authors discretion (recommended 3+ business days)”

1 Like
#5

I really like the suggestions and if someone does not know who to choose for the review, there is chat and we can always talk to find someone :slight_smile: