Update the PR guidelines in Contributing docs
This commit is contained in:
tfny
GitHub
parent
ff2911d070
commit
14eb56cf30
1 changed files with 222 additions and 6 deletions
|
|
@ -3,12 +3,228 @@ title: "Traefik Pull Requests Documentation"
|
|||
description: "Looking to contribute to Traefik Proxy? This guide will show you the guidelines for submitting a PR in our contributors guide repository."
|
||||
---
|
||||
|
||||
# Submitting Pull Requests
|
||||
# Before You Submit a Pull Request
|
||||
|
||||
A Quick Guide for Efficient Contributions
|
||||
{: .subtitle }
|
||||
This guide is for contributors who already have a pull request to submit.
|
||||
If you are looking for information on setting up your developer environment
|
||||
and creating code to contribute to Traefik Proxy or related projects,
|
||||
see the [development guide](https://docs.traefik.io/contributing/building-testing/).
|
||||
|
||||
So you've decided to improve Traefik?
|
||||
Thank You!
|
||||
Looking for a way to contribute to Traefik Proxy?
|
||||
Check out this list of [Priority Issues](https://github.com/traefik/traefik/labels/contributor%2Fwanted),
|
||||
the [Good First Issue](https://github.com/traefik/traefik/labels/contributor%2Fgood-first-issue) list,
|
||||
or the list of [confirmed bugs](https://github.com/traefik/traefik/labels/kind%2Fbug%2Fconfirmed) waiting to be remedied.
|
||||
|
||||
Please review the [guidelines on creating PRs](https://github.com/traefik/contributors-guide/blob/master/pr_guidelines.md) for Traefik in our [contributors guide repository](https://github.com/traefik/contributors-guide).
|
||||
## How We Prioritize
|
||||
|
||||
We wish we could review every pull request right away.
|
||||
Unfortunately, our team has to prioritize pull requests (PRs) for review
|
||||
(but we are welcoming new [maintainers](https://github.com/traefik/traefik/blob/master/docs/content/contributing/maintainers-guidelines.md) to speed this up,
|
||||
so if you are interested, check it out and apply).
|
||||
|
||||
The PRs we are able to handle fastest are:
|
||||
|
||||
* Documentation updates.
|
||||
* Bug fixes.
|
||||
* Enhancements and Features with a `contributor/wanted` tag.
|
||||
|
||||
PRs that take more time to address include:
|
||||
|
||||
* Enhancements or Features without the `contributor/wanted` tag.
|
||||
|
||||
If you have an idea for an enhancement or feature that you would like to build,
|
||||
[create an issue](https://github.com/traefik/traefik/issues/new/choose) for it first
|
||||
and tell us you are interested in writing the PR.
|
||||
If an issue already exists, definitely comment on it to tell us you are interested in creating a PR.
|
||||
|
||||
This will allow us to communicate directly and let you know if it is something we would accept.
|
||||
It also allows us to make sure you have all the information you need during the design phase
|
||||
so that it can be reviewed and merged quickly.
|
||||
|
||||
If you have questions about the Triage process,
|
||||
[read more here](https://github.com/traefik/contributors-guide/blob/master/issue_triage.md).
|
||||
|
||||
## The Pull Request Submit Process
|
||||
|
||||
Merging a PR requires the following steps to be completed before it is merged automatically.
|
||||
|
||||
* Make sure your pull request adheres to our best practices. These include:
|
||||
* [Following project conventions](https://github.com/traefik/traefik/blob/master/docs/content/contributing/maintainers-guidelines.md); including using the PR Template.
|
||||
* Make small pull requests.
|
||||
* Solve only one problem at a time.
|
||||
* Comment thoroughly.
|
||||
* Do not open the PR from an organization repository.
|
||||
* Keep "allows edit from maintainer" checked.
|
||||
* Use semantic line breaks for documentation.
|
||||
* Pass the validation check.
|
||||
* Pass all tests.
|
||||
* Receive 3 approving reviews maintainers.
|
||||
|
||||
## Pull Request Review Cycle
|
||||
|
||||
You can read about our Triage Process [here](https://github.com/traefik/contributors-guide/blob/master/issue_triage.md),
|
||||
but in short, it looks like this:
|
||||
|
||||
* We triage every new PR or comment before entering it into the review process.
|
||||
* We ensure that all prerequisites for review have been met.
|
||||
* We check to make sure the use case meets our needs.
|
||||
* We assign reviewers.
|
||||
* Design Review.
|
||||
* This takes longer than other parts of the process.
|
||||
* We review that there are no obvious conflicts with our codebase.
|
||||
* Code Review.
|
||||
* We review the code in-depth and run tests.
|
||||
* We may ask for changes here.
|
||||
* During code review, we ask that you be reasonably responsive,
|
||||
if a PR languishes in code review it is at risk of rejection,
|
||||
or we may take ownership of the PR and the contributor will become a co-author.
|
||||
* Merge.
|
||||
* Success!
|
||||
|
||||
!!! note
|
||||
|
||||
Occasionally, we may freeze our codebase when working towards a specific feature or goal that could impact other development.
|
||||
During this time, your pull request could remain unmerged while the release work is completed.
|
||||
|
||||
## Run Local Verifications
|
||||
|
||||
You must run these local verifications before you submit your pull request to predict the pass or failure of continuous integration.
|
||||
Your PR will not be reviewed until these are green on the CI.
|
||||
|
||||
* `make validate`
|
||||
* `make pull-images`
|
||||
* `make test`
|
||||
|
||||
## The Testing and Merge Workflow
|
||||
|
||||
Pull Requests are managed by the bot [Myrmica Lobicornis](https://github.com/traefik/lobicornis).
|
||||
This bot is responsible for verifying GitHub Checks (CI, Tests, etc), mergability, and minimum reviews.
|
||||
In addition, it rebases or merges with the base PR branch if needed.
|
||||
It performs several other housekeeping items
|
||||
and you can read more about those on the [README](https://github.com/traefik/lobicornis) for Lobicornis.
|
||||
|
||||
The maintainer giving the final LGTM must add the `status/3-needs-merge` label to trigger the merge bot.
|
||||
|
||||
By default, a squash-rebase merge will be carried out.
|
||||
|
||||
The status `status/4-merge-in-progress` is only used by the bot.
|
||||
|
||||
If the bot is not able to perform the merge, the label `bot/need-human-merge` is added.
|
||||
In such a situation, solve the conflicts/CI/... and then remove the label `bot/need-human-merge`.
|
||||
|
||||
To prevent the bot from automatically merging a PR, add the label `bot/no-merge`.
|
||||
|
||||
The label `bot/light-review` decreases the number of required LGTM from 3 to 1.
|
||||
|
||||
This label can be used when:
|
||||
|
||||
* Updating a dependency.
|
||||
* Merging branches back into the next version branch.
|
||||
* Submitting minor documentation changes.
|
||||
* Submitting changelog PRs.
|
||||
|
||||
## Why Was My Pull Request Closed?
|
||||
|
||||
Traefik Proxy is made by the community for the community,
|
||||
as such the goal is to engage the community to make Traefik the best reverse proxy available.
|
||||
Part of this goal is maintaining a lean codebase and ensuring code velocity.
|
||||
unfortunately, this means that sometimes we will not be able to merge a pull request.
|
||||
|
||||
Because we respect the work you did, you will always be told why we are closing your pull request.
|
||||
If you do not agree with our decision, do not worry; closed pull requests are easy to recreate,
|
||||
and little work is lost by closing a pull request that subsequently needs to be reopened.
|
||||
|
||||
Your pull request might be closed if:
|
||||
|
||||
* Your PR's design conflicts with our existing codebase in such a way that Merging is not an option
|
||||
and the work needed to make your pull request usable is too high.
|
||||
* To prevent this, make sure you created an issue first
|
||||
and think about including Traefik Proxy maintainers in your design phase to minimize conflicts.
|
||||
* Your PR is for an enhancement or feature that we will not use.
|
||||
* Please remember to create an issue for any pull request **before** you create a PR
|
||||
to ensure that your goal is something we can merge and that you have any design insight you might need from the team.
|
||||
* Your PR has been waiting for feedback from the contributor for over 90 days.
|
||||
|
||||
## Why is My Pull Request Not Getting Reviewed
|
||||
|
||||
A few factors affect how long your pull request might wait for review.
|
||||
|
||||
We must prioritize which PRs we focus on.
|
||||
Our first priority is PRs we have identified as having high community engagement and broad applicability.
|
||||
We put our top priorities on our roadmap and you can identify them by the `contributor/wanted` tag.
|
||||
These PRs will enter our review process the fastest.
|
||||
|
||||
Our second priority is bug fixes.
|
||||
Especially for bugs that have already been tagged with `bug/confirmed`.
|
||||
These reviews enter the process quickly.
|
||||
|
||||
If your PR does not meet the criteria above,
|
||||
it will take longer for us to review as any PRs that do meet the criteria above will be prioritized.
|
||||
|
||||
Additionally, during the last few weeks of a milestone, we stop reviewing PRs to reduce churn and stabilize.
|
||||
We will resume after the release.
|
||||
|
||||
The second major reason that we deprioritize your PR is that you are not following best practices.
|
||||
|
||||
The most common failures to follow best practices are:
|
||||
|
||||
* You did not create an issue for the PR you wish to make.
|
||||
If you do not create an issue before submitting your PR,
|
||||
we will not be able to answer any design questions and let you know how likely your PR is to be merged.
|
||||
* You created pull requests that are too large to review.
|
||||
* Break your pull requests up.
|
||||
If you can extract whole ideas from your pull request and send those as pull requests of their own,
|
||||
you should do that instead.
|
||||
It is better to have many pull requests addressing one thing than one pull request addressing many things.
|
||||
* Traefik Proxy is a fast-moving codebase — lock in your changes ASAP with your small pull request,
|
||||
and make merges be someone else's problem.
|
||||
We want every pull request to be useful on its own,
|
||||
so use your best judgment on what should be a pull request vs. a commit.
|
||||
* You did not comment well.
|
||||
* Comment everything.
|
||||
|
||||
Please remember that we are working internationally, cross-culturally, and with different use-cases.
|
||||
Your reviewer will not intuitively understand the problem the same way you do or solve it the same way you would.
|
||||
This is why every change you make must be explained and your strategy for coding must also be explained.
|
||||
|
||||
* Your tests were inadequate or absent.
|
||||
* If you do not know how to test your PR, please ask!
|
||||
We will be happy to help you or suggest appropriate test cases.
|
||||
|
||||
If you have already followed the best practices and your PR still has not received a response,
|
||||
here are some things you can do to move the process along:
|
||||
|
||||
* If you have fixed all the issues from a review,
|
||||
remember to re-request a review (using the designated button) to let your reviewer know that you are ready.
|
||||
You can choose to comment with the changes you made.
|
||||
* Ping `@tfny` if you have not been assigned to a reviewer.
|
||||
|
||||
For more information on best practices, try these links:
|
||||
|
||||
* [How to Write a Git Commit Message - Chris Beams](https://chris.beams.io/posts/git-commit/)
|
||||
* [Distributed Git - Contributing to a Project (Commit Guidelines)](https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project)
|
||||
* [What’s with the 50/72 rule? - Preslav Rachev](https://preslav.me/2015/02/21/what-s-with-the-50-72-rule/)
|
||||
* [A Note About Git Commit Messages - Tim Pope](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html)
|
||||
|
||||
## It's OK to Push Back
|
||||
|
||||
Sometimes reviewers make mistakes.
|
||||
It is OK to push back on changes your reviewer requested.
|
||||
If you have a good reason for doing something a certain way, you are absolutely allowed to debate the merits of a requested change.
|
||||
Both the reviewer and reviewee should strive to discuss these issues in a polite and respectful manner.
|
||||
|
||||
You might be overruled, but you might also prevail.
|
||||
We are pretty reasonable people.
|
||||
|
||||
Another phenomenon of open-source projects (where anyone can comment on any issue) is the dog-pile -
|
||||
your pull request gets so many comments from so many people it becomes hard to follow.
|
||||
In this situation, you can ask the primary reviewer (assignee) whether they want you to fork a new pull request
|
||||
to clear out all the comments.
|
||||
You do not have to fix every issue raised by every person who feels like commenting,
|
||||
but you should answer reasonable comments with an explanation.
|
||||
|
||||
## Common Sense and Courtesy
|
||||
|
||||
No document can take the place of common sense and good taste.
|
||||
Use your best judgment, while you put a bit of thought into how your work can be made easier to review.
|
||||
If you do these things your pull requests will get merged with less friction.
|
||||
|
|
|
|||
Loading…
Reference in a new issue