Skip to content

Pull Requests

Jump to bottom
kyuksel edited this page Oct 31, 2022 · 17 revisions

Required Approvals

Pull requests must be approved by at least two people. You can request review from or tag the DataBiosphere/terra-ui-working-group GitHub team on PRs for any feedback, but it is no longer required.

Tests

Integration tests against the dev environment will automatically be run when a branch is pushed. Currently, branch protection rules do not require tests to pass in order for a PR to be merged. However, the general expectation is that tests should pass before merging. In the case of flaky test failures that are apparently unrelated to the PR, re-running the tests a few times is preferred over merging with failing tests.

Occasionally, a change in dev causes one or more tests to consistently fail. Usually these are announced in the terra-ui Slack channel. In these cases, it's ok to merge a PR with those tests failing. If an apparently unrelated test consistently fails on a PR after a few re-runs, post in the terra-ui Slack channel.

Other Requirements and Guidelines

For Authors

Required

  • Prefix JIRA ticket (for compliance reasons) on PR title, followed by a brief reason for the PR; e.g. [IA-1234] Display Spark UIs
  • If you are changing an existing file's name and/or path, make sure CODEOWNERS are updated accordingly if applicable.
  • If you are adding a MixPanel event, make sure to go through the MixPanel Documentation Guide. For any questions, utilize the Slack channel #mixpanel-help.

Strongly Recommended

  • Have clear JIRA ticket descriptions and acceptance criteria.
  • Have clear PR descriptions:
    • Include screenshots and/or videos on PR description when feasible.
    • Show the old vs. new.
    • Include logs of manual testing completed.
  • Do not squash commits until your PR is ready to merge. This helps when reviewers check out code and pull new changes incrementally as they review.
  • If you’re adding/modifying an integration test or suspect a non-test change is likely to affect test behaviour, make sure to
  • Write unit tests, especially for new code.

Tips

  • Use the draft PR feature.
    • If you’d like to open a PR that isn’t ready for review yet, open it as a draft PR so it’s easier for folks to be aware.
    • Also put [DRAFT] in the title of the PR. This makes it easier to immediately spot and ignore draft PRs from the notification email body.
  • How to help PR makers and reviewers communicate at which stage they are?
    • 👍🏼 comments when you agree with a comment and feel no need to elaborate.
    • Re-request review when you’re done addressing comments so reviewers get notified.
    • Conversations should be resolved only by the person who first commented when they believe the comment was addressed.
  • Remember that smaller PRs are reviewed faster, larger PRs are reviewed exponentially slower.
  • Keep the scope tight.
  • Review and comment your own code, adding comments that are only meant to be read at review-time (See an example here. Of course, if the comment makes sense to future readers, consider adding the comment in the actual committed code itself). This should reduce the round trips (from teammate to reviewer back to teammate) which are expensive. It also lets you leverage your knowledge of your teammates and indicates that you are considerate of the concerns they would typically raise. This increases team trust and shipping velocity.

For Reviewers

Strongly Recommended

  • Mention how you manually tested the PR on a locally deployed UI or a PR site. All team reviews are expected to include such testing.
  • While submitting a review, clarify any expectation around what must be changed, and whether the change is required to merge the PR or it can be done after merging the PR.

Tips

  • It’s okay to approve a PR without waiting for a comment to be addressed, to speed things up, when one or both of the following apply:
    • You didn't consider not addressing the comment a blocker to merge the PR.
    • You trust that the author will address the comment prior to merging, and if they happen to not, it wouldn't be a huge deal.

References

Terra UI Wiki.

Developing

Testing

UX Considerations

Using Terra

Deploying

Security and Maintenance

Clone this wiki locally