[hotfix] Add Review Progress section to PR description template. #6873

fhueske · 2018-10-18T08:20:04Z

What is the purpose of the change

Add the Review Progress section to the PR description template.
This change was discussed on the dev mailing list

Brief change log

Add the Review Progress section to the PR description template.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? yes, see review process description

tisonkun

+1 thanks for the effort! This pull request follows the discussion and it is LGTM.

zentol · 2018-10-18T08:43:14Z

Are we concerned that contributors might modify the review progress section? Committers would have to check through mail notifications whether it was modified or not :/

zentol · 2018-10-18T08:43:40Z

(Should probably also add a note that this section is not to be modified by the contributor)

fhueske · 2018-10-18T09:28:22Z

Thanks for the feedback @zentol.
I've updated the PR and made it very explicit that the progress should only be modified by a committter.
I think with this in place, we should assume that contributors do not modify the progress section.

hequn8128 · 2018-10-18T15:22:59Z

+1 to merge! Looks very neat and the template doesn't grow too much.

sunjincheng121 · 2018-10-19T03:19:11Z

.github/PULL_REQUEST_TEMPLATE.md

+
+* [ ] 1. The contribution is well-described.
+* [ ] 2. There is consensus that the contribution should go into to Flink.
+* [ ] 3. [Does not need specific attention | Needs specific attention for X | Has attention for X by Y]


About [3], If the committer identifies [Needs specific attention for X], what do we need to do with PR?
From the points of my view, the committer who change the review process, have to make sure all the specific attention parts are solved.So the [3] should be described : [ ] 3. All the specific attention parts are well-solved.
What do you think?

I think it is fine as it is. When the PR needs special attention, it should be marked at the point 3..

A committer who is familiar with the code should then resolve the remaining points 4. and 5, architecture and implementation.

In other words, if a PR is reviewed by multiple Committers familiar with different modules, [3] may increase the content continuously. Then a committer who is familiar with the code，change the review process with the value [Does not need specific attention] , right?

Yes, I think in theory, the number of people who should have a look at the PR can be extended. IMO, the committer who finally merges the PR should check that everybody mentioned there commented on the PR or ask them before merging.

Make sense to me. +1 to merged.

twalthr · 2019-01-11T09:28:39Z

.github/PULL_REQUEST_TEMPLATE.md

+**NOTE: THE REVIEW PROGRESS MUST ONLY BE UPDATED BY AN APACHE FLINK COMMITTER!**
+
+* [ ] 1. The contribution is well-described.
+* [ ] 2. There is consensus that the contribution should go into to Flink.


Can we add a point "Is blocked by feature X" or "Make sense to wait after feature Y"?

That's a good point but I'm not sure if this belongs into the template. If the PR author is aware of the dependency, it should be noted in the PR description. If a reviewer find that dependency, it can be noted as a regular comment and the last box should not be ticked until the blocking PR was merged.

Ok, I'm also fine with a comment in the PR.

twalthr · 2019-01-11T09:30:06Z

.github/PULL_REQUEST_TEMPLATE.md

+
+**NOTE: THE REVIEW PROGRESS MUST ONLY BE UPDATED BY AN APACHE FLINK COMMITTER!**
+
+* [ ] 1. The contribution is well-described.


Should we add a field to add the committers name to track who made this decision?

Shouldn't this be tracked by the comments in Github and also posted to the mailing list?

I'm sure it is tracked somewhere. But I thought it might make sense to put a name right next to certain decisions. And it helps if there is a committer that takes ownership.

rmetzger · 2019-01-21T13:53:03Z

@zentol: I'm a bit against this **NOTE: THE REVIEW PROGRESS MUST ONLY BE UPDATED BY AN APACHE FLINK COMMITTER!** line, as it seems not very inviting for new community members.
Maybe as a convention, reviewers put into the comments what they have reviewed, so if a committer who's merging a PR has doubts about the checklist, they can check the comments?

I'm also thinking* about implementing a bot that takes care of tracking the checklist.

*strongly enough to have created a project locally already :)

fhueske · 2019-01-21T14:05:47Z

I think if we modify the review checklist in place (i.e., update it in the PR description) this is almost automatically given.

AFAIK, only the PR author and members of the ASF Github organization (or maybe even registered Flink committers?) can update the PR description. If we ask committers to sign-off changes with their name, we should be good.

A review progress bot would be great to have, IMO.

fhueske · 2019-01-22T14:24:03Z

I've updated the PR.

Before merging I'll post to the dev mailing list to remind everybody about the upcoming change and the new review process.

rmetzger · 2019-01-22T14:34:00Z

Nice. I'm almost done with the bot (famous last words in software engineering).
I might be able to finish it till tomorrow. Should we wait with the announcement till then?

fhueske · 2019-01-22T15:17:44Z

Sorry, did not read your comment before I sent out the mail.
You can just reply to the thread. Maybe, we can even leave out this PR and go directly with the bot.

KurtYoung · 2019-01-23T01:09:50Z

.github/PULL_REQUEST_TEMPLATE.md

+# Review Progress <!-- NOTE: DO NOT REMOVE THIS SECTION! -->
+
+* [ ] 1. The contribution is well-described.
+* [ ] 2. There is consensus that the contribution should go into to Flink.


Can you give some examples to show what makes a consensus, let developers have a good understanding?

Hi @KurtYoung, the consensus aspect is described in the Pull Request Review Guide that is linked below.

Do you think this is sufficient or does it need more clarification?

[hotfix] Add Review Progress section to PR description template.

84c4bb4

tisonkun approved these changes Oct 18, 2018

View reviewed changes

Address comment

4f10763

fhueske force-pushed the review-template branch from 1af4483 to 4f10763 Compare October 18, 2018 09:25

sunjincheng121 reviewed Oct 19, 2018

View reviewed changes

twalthr reviewed Jan 11, 2019

View reviewed changes

Remove line about committers.

8ecc521

KurtYoung reviewed Jan 23, 2019

View reviewed changes

fhueske closed this Feb 25, 2019

fhueske deleted the review-template branch February 25, 2019 22:32


		NOTE: THE REVIEW PROGRESS MUST ONLY BE UPDATED BY AN APACHE FLINK COMMITTER!

		* [ ] 1. The contribution is well-described.

[hotfix] Add Review Progress section to PR description template. #6873

[hotfix] Add Review Progress section to PR description template. #6873

Uh oh!

Conversation

fhueske commented Oct 18, 2018

What is the purpose of the change

Brief change log

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

zentol commented Oct 18, 2018

Uh oh!

zentol commented Oct 18, 2018

Uh oh!

fhueske commented Oct 18, 2018

Uh oh!

hequn8128 commented Oct 18, 2018

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!

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!

rmetzger commented Jan 21, 2019

Uh oh!

fhueske commented Jan 21, 2019

Uh oh!

fhueske commented Jan 22, 2019

Uh oh!

rmetzger commented Jan 22, 2019

Uh oh!

fhueske commented Jan 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants