This doc explains the best practices for submitting a pull request to the Gloo Edge project. It should serve as a reference for all contributors, and be useful especially useful to new and infrequent submitters.
Merging a pull request requires the following steps to be completed before the pull request will be merged automatically.
- Open a pull request
- Pass all automated tests
- Get all necessary approvals from reviewers and code owners
If you want to solicit reviews before the implementation of your pull request is complete, there are a few methods to achieve this:
- Open a Draft PR. This will run the minimum set of tests, but not the Kubernetes e2e tests
- Remove the
Keep PR Updatedlabel. Automation on PRs will attempt to keep feature branches up to date with the target branch. Removing this label will prevent that and avoid CI running unnecessarily on branches that we know are not ready to merge. - Add the
Work In Progresslabel. This will allow all tests to run will prevent the PR from merging automatically, even when all tests are passing and there are two approving reviews.
Even if you have followed these steps, it is good practice to add a # WORK IN PROGRESS section to the PR and outline the work that is still to be done.
Below are some best practices we have found to help PRs get reviewed quickly
All PRs are required to contain a changelog entry. This is enforced by the changelog bot. If you do not include one, the first reviewer should ask for this.
You can use the changelog.sh script to generate a changelog template for submission.
Small PRs are more likely to be reviewed quickly and thoroughly. If the PR takes >45 minutes to review, the review may be less thorough and more likely to miss something important.
Having a series of discrete commits makes it easier to understand the idea of the PR, and break up the review into smaller chunks
When PRs merge in Gloo Edge, they are squashed into a single commit, so it is not necessary to squash your commits before merging.
This can make it difficult to understand the history of the PR, and can make it difficult to understand the changes in the future.
If you encounter cosmetic changes to the project that you wish to improve (e.g. spelling mistakes, formatting, poor names, etc), please submit these as a separate PR.
As always, use your judgment. A few small changes are fine, but if you find yourself making many changes to unrelated files, it is probably best to split them up.
If your changes are large, or you are unsure of the approach, it is best to gather feedback early. This can be done by opening a draft PR, or by asking for feedback in Slack.
In your code, if someone might not understand why you did something, they definitely won't remember later. To avoid this, add comments to your code that express the why, since the code should express the how.
Read up on GoDoc - follow those general rules for comments.
Almost every PR that changes code, should also change or introduce tests. If for some reason this doesn't apply to your PR, please explain why in the PR body.
If you do not know how to test a give feature, please ask, and we'll be happy to suggest appropriate test cases.
The PR body is generally the first place reviewers will look to gather context on the set of proposed changes. As such, we recommend the following:
- Include a description of the change in the PR body, where the why is made clear. This makes it easier to understand the change in the future
- Enumerate all changes, even/especially minor ones, so the reviewer knows they are intentional
- Link any relevant Slack conversations or design documents in the PR body so that they are not lost
When a PR merges into the target branch in Gloo Edge, the changes are squashed into a single commit, whose message is the PR title. As such, it is important to have a clear title that describes the change