Skip to content

Commit 8cca4ed

Browse files
author
Hendrik Muhs
authored
[ML] PR guidelines (elastic#105) (elastic#112)
Add guidelines about PR's as well as backport PR's.
1 parent 91bd889 commit 8cca4ed

File tree

1 file changed

+40
-1
lines changed

1 file changed

+40
-1
lines changed

CONTRIBUTING.md

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ Once your changes and tests are ready to submit for review:
5050

5151
1. Submit a pull request
5252

53-
Push your local changes to your forked copy of the repository and [submit a pull request](https://help.github.com/articles/about-pull-requests/). In the pull request, choose a title which sums up the changes that you have made, and in the body provide more details about what your changes do. Also mention the number of the issue where discussion has taken place, eg `Closes #123`.
53+
Push your local changes to your forked copy of the repository and [submit a pull request](https://help.github.com/articles/about-pull-requests/). In the pull request, choose a title which sums up the changes that you have made, and in the body provide more details about what your changes do. Also mention the number of the issue where discussion has taken place, eg `Closes #123`. See more information about pull requests [below](#pull-requests)
5454

5555
Then sit back and wait. There will probably be discussion about the pull request and, if any changes are needed, we would love to work with you to get your pull request merged into ml-cpp.
5656

@@ -64,3 +64,42 @@ Please adhere to the general guideline that you should never force push to a pub
6464
1. If you change code, follow the existing [coding style](STYLEGUIDE.md).
6565
1. Write a test, unit tests are located under `lib/{module}/unittest`
6666
1. Test your changes (`make test`)
67+
68+
## Pull Requests
69+
70+
Every change made to ml-cpp must be held to a high standard, Pull Requests are equally important as they document changes and decissions that have been made. `You Know, for Search` - a descriptive and relevant summary of the change helps people to find your PR later on.
71+
72+
The followings lists some guidelines when authoring pull requests. Note, try to follow this guideline as close as possible but do not feel bad if you are unsure, team members can help you.
73+
74+
1. PR title summarizes the change, short and descriptive.
75+
1. Use prefixes for quick categorization:
76+
1. [X.Y] Branch label if backport PR, if omited the PR only applies to master. Note, backport PR's are done after master PR, further explanation can be found below [#backport]
77+
1. [ML] mandatory prefix, to be consistent with other repositories
78+
1. [FEATURE] If your pull requests targets a feature branch, this prefix helps as a filter
79+
1. A detailed summary of what changed. Some hints:
80+
1. Keep it short but do not ommit important details. Usually we squash and merge, write what you would write as commit message of the squashed commit, later you can reuse what you wrote.
81+
1. A link to each issue that is closed by the PR (e.g. Closes #123) or related (Relates #456)
82+
1. Further information if necessary, maybe you want to share a screenshot, list open Todo's, describe your thought process, etc.
83+
1. Optional: List of backport PR's, other related PR's
84+
1. Label the PR, not all might apply:
85+
1. `:ml` mandatory label, to be consistent with other repositories.
86+
1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`.
87+
1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version.
88+
1. `>non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature
89+
1. `affects-results` If the PR is expected to have a significant impact on results(QA test suite), e.g. changes of scoring. Be aware that changes in terms of memory consumption can indirectly lead to significant result changes.
90+
1. `discuss` If your PR suggests a change for which you'd first like to discuss regarding its functional changes before going deep into actual implementation details (e.g. you change a default)
91+
1. `WIP` lets potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR.
92+
1. `review` Try to find reviewers for your change, github has some suggestions, however if unsure or you know the reviewer is very busy, `review` marks the PR as free to grab for review. Still, PR's are open to anyone to comment.
93+
94+
### Backport
95+
96+
Any development usually starts with an implementation on `master`, therefore any PR starts with a PR against `master`. Depending on the type we then decide about backport branches, features usually get backported to the active release branch, e.g. `6.x`, bugfixes might get backported to concrete release branches, e.g. `6.1`. If unsure about the versions to backport to, it can be discussed on the master PR.
97+
98+
A backport starts right after merging the PR on `master`, please open backport PR's immediately after merging to `master`, even if you intend to merge backports a little later (e.g. to wait/analyse QA results). This helps to ensure that backports do not get lost. Rules for Backport PR's:
99+
100+
1. Prefix the title with `[X.Y]` and the mandatory `[ML]`, e.g. `[6.3][ML] Store expanding window bucket values in a compressed format`.
101+
1. Link to the originating PR in the description, you do not need to repeat the description as discussions should exclusively happen on the master PR.
102+
1. Backports do not need a review, however if you had to do further changes (merge conflicts, compiler specialities) it's advised to let the adjustments get quickly reviewed.
103+
1. Label the PR with `>backport` and the corresponding version of this backport, the full set of labels remain on the master PR. If the master PR is classified as `>non-issue` also add the `>non-issue` label due to CI checks.
104+
105+
Although it might look simple, always wait for CI to confirm that changes are sound to avoid unnecessary build breaks.

0 commit comments

Comments
 (0)