[ML] PR guidelines #105

hendrikmuhs · 2018-05-28T08:17:41Z

Adding guidelines about PR's as well as discussed backport PR's.

A readable version can be found at: https://github.com/hendrikmuhs/ml-cpp/blob/PR-guidelines/CONTRIBUTING.md#pull-requests

tveasey

I think this looks pretty good; I'm basically happy with the content. All my comments are related to typos.

tveasey · 2018-05-29T08:57:37Z

CONTRIBUTING.md

+
+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.
+
+The followings lists some guide lines 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.


"guide lines" |-> "guidelines" and "Note: Try to follow this guideline as close" |-> "Note, try to follow this guideline as closely"

tveasey · 2018-05-29T09:00:26Z

CONTRIBUTING.md

+
+1. PR title summarizes the change, short and descriptive.
+   1. Use prefixes for quick categorization:
+      1. [X.Y] Branch label if backport PR, if ommited the PR only applies to master. Note: Backport PR's are done after master PR, further explaination can be found below [#backport]


ommited |-> omitted. Plus I'd change "Note: Backport" to "Note, backport" either way no need to capitalise backport here. Also, explaination |-> explanation.

tveasey · 2018-05-29T09:02:30Z

CONTRIBUTING.md

+   1. Use prefixes for quick categorization:
+      1. [X.Y] Branch label if backport PR, if ommited the PR only applies to master. Note: Backport PR's are done after master PR, further explaination can be found below [#backport]
+      1. [ML] mandatory prefix, to be consistent with other repositories
+      1. [FEATURE] If your pull requests targets a feature branch, this prefix helps as a filter


tveasey · 2018-05-29T09:06:19Z

CONTRIBUTING.md

+    1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`.
+    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.
+    1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature
+    1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption.


I've tended to use this for changes which are expected to have a significant impact on results (otherwise, it will likely apply to nearly all PRs). That said, this is difficult to quantify so wouldn't be against repurposing.

No, I am fine with the current rules, no intent to change it. I am re-phrasing 'any' to 'significant'

tveasey · 2018-05-29T09:06:42Z

CONTRIBUTING.md

+    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.
+    1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature
+    1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption.
+    1. `discuss` If your PR suggests a change which you first like to discuss regarding it's functional changes before going deep into actual implementation details (e.g. you change a default)


"If your PR suggests a change which you first like to discuss regarding it's" it's |-> "If your PR suggests a change for which you'd first like to discuss regarding its"

tveasey · 2018-05-29T09:08:37Z

CONTRIBUTING.md

+    1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature
+    1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption.
+    1. `discuss` If your PR suggests a change which you first like to discuss regarding it's functional changes before going deep into actual implementation details (e.g. you change a default)
+    1. `WIP` let's 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.


let's |-> lets

tveasey · 2018-05-29T09:10:36Z

CONTRIBUTING.md

+
+### Backport
+
+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.


, e.g. 6.1

tveasey · 2018-05-29T09:11:40Z

CONTRIBUTING.md

+
+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.
+
+A backport starts right after merging the PR on `master`, please open backport PR's immediatly 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:


immediatly |-> immediately

tveasey · 2018-05-29T11:25:19Z

Actually, I've just encountered another issue: if the original PR was marked as a non-issue it won't have a change log entry and if the backport PR doesn't have a >non-issue label it fails the checks. So this label needs to be added or we need to customise the checks for such PRs. I'd be happy adding the >non-issue label in such cases as well as the target version.

address review comments

Improve affects-results description

hendrikmuhs · 2018-05-29T11:54:21Z

Thanks @tveasey, I made some changes based on your review.

droberts195 · 2018-05-29T12:02:04Z

CONTRIBUTING.md

+    1. `:ml` mandatory label, to be consistent with other repositories.
+    1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`.
+    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.
+    1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature


nit: non-issue should be >non-issue.

Same in two places on line 103

tveasey

Aside from Dave's catch regarding label naming, I'm happy with this now. I don't think I need to review again so I'll approve in advance.

fix the non-issue label

Add guidelines about PR's as well as backport PR's.

Hendrik Muhs added 4 commits May 27, 2018 07:55

Update CONTRIBUTING.md

977372b

Update CONTRIBUTING.md

32e77d0

Update CONTRIBUTING.md

5085698

Update CONTRIBUTING.md

ba5842a

hendrikmuhs added v7.0.0 review >non-issue v6.4.0 labels May 28, 2018

tveasey reviewed May 29, 2018

View reviewed changes

Hendrik Muhs added 2 commits May 29, 2018 13:51

Update CONTRIBUTING.md

1e1986c

address review comments

Update CONTRIBUTING.md

e60bba5

Improve affects-results description

droberts195 reviewed May 29, 2018

View reviewed changes

tveasey approved these changes May 29, 2018

View reviewed changes

Update CONTRIBUTING.md

5ca7a65

fix the non-issue label

hendrikmuhs merged commit ac7eb3f into elastic:master May 30, 2018

hendrikmuhs pushed a commit to hendrikmuhs/ml-cpp that referenced this pull request May 30, 2018

[ML] PR guidelines (elastic#105)

95fa22c

Add guidelines about PR's as well as backport PR's.

hendrikmuhs mentioned this pull request May 30, 2018

[6.x][ML] Backport PR guidelines (#105) #112

Merged

hendrikmuhs pushed a commit that referenced this pull request May 30, 2018

[ML] PR guidelines (#105) (#112)

8cca4ed

Add guidelines about PR's as well as backport PR's.

sophiec20 added :ml and removed :ml labels Jun 12, 2018


		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.

		The followings lists some guide lines 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.


		### Backport

		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.


		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.

		A backport starts right after merging the PR on `master`, please open backport PR's immediatly 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:

[ML] PR guidelines #105

[ML] PR guidelines #105

Uh oh!

Conversation

hendrikmuhs commented May 28, 2018

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tveasey May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tveasey commented May 29, 2018

Uh oh!

hendrikmuhs commented May 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tveasey May 29, 2018 •

edited

Loading