[hotfix] Add Review Progress section to PR description template. #6873
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,3 +70,15 @@ This change added tests and can be verified as follows: | |
|
|
||
| - Does this pull request introduce a new feature? (yes / no) | ||
| - If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) | ||
|
|
||
| ---- | ||
|
|
||
| # 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. | ||
|
Contributor
There was a problem hiding this comment. Can we add a point "Is blocked by feature X" or "Make sense to wait after feature Y"?
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. Ok, I'm also fine with a comment in the PR.
Contributor
There was a problem hiding this comment. Can you give some examples to show what makes a consensus, let developers have a good understanding?
Contributor
Author
There was a problem hiding this comment. 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? |
||
| * [ ] 3. [Does not need specific attention | Needs specific attention for X | Has attention for X by Y] | ||
|
Member
There was a problem hiding this comment. About [3], If the committer identifies [Needs specific attention for X], what do we need to do with PR?
Contributor
Author
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. 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?
Contributor
Author
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. Make sense to me. +1 to merged. |
||
| * [ ] 4. The architectural approach is sound. | ||
| * [ ] 5. Overall code quality is good. | ||
|
|
||
| Please see the [Pull Request Review Guide](https://flink.apache.org/reviewing-prs.html) if you have questions about the review process. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a field to add the committers name to track who made this decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be tracked by the comments in Github and also posted to the mailing list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.