Emily Jiang
Sep 20, 2017, 5:42:40 AM9/20/17
to Eclipse MicroProfile
I think there are some confusion on when to merge PR and how many people need to review a PR. I would like to use this Q&A to clarify the situation so that everyone is on the same page. I provided the following questions and the possible answers or further thoughts. Please add more.
I think the ultimate goal is to move fast and control the quality. We should eliminate bottle necks. We should not create hierarchy on who should have a final say. Our MP train should be moving all the time. If one person is busy on other things, the train should not be stopped.
Q: If I have a PR, how many people do I need to send for review?
A: Most cases will be just one.
Q: If that reviewer does not respond within a day or so, do we automatically remove it and send to a different person for review?
A: I think yes. More thoughts?
Q: Some PR has very tiny changes, I could send to quite a few people for review and just want to get one approval and then merge. Is it ok?
A: ok. If any other reviewer has issues with the PR, it is recommended for that reviewer to reopen the discussion.
Q: If I just corrected one typo, do I still need a review?
A: I think yes. Open to discussion.
Emily
I think the ultimate goal is to move fast and control the quality. We should eliminate bottle necks. We should not create hierarchy on who should have a final say. Our MP train should be moving all the time. If one person is busy on other things, the train should not be stopped.
Q: If I have a PR, how many people do I need to send for review?
A: Most cases will be just one.
Q: If that reviewer does not respond within a day or so, do we automatically remove it and send to a different person for review?
A: I think yes. More thoughts?
Q: Some PR has very tiny changes, I could send to quite a few people for review and just want to get one approval and then merge. Is it ok?
A: ok. If any other reviewer has issues with the PR, it is recommended for that reviewer to reopen the discussion.
Q: If I just corrected one typo, do I still need a review?
A: I think yes. Open to discussion.
Emily
Heiko Rupp
Sep 20, 2017, 7:21:32 AM9/20/17
to Eclipse MicroProfile
GitHub has this "designated reviewer" field, where unfortunately only MP committers can be selected.
I would make it so that everyone of the active folks on the project can send a 'LGTM' and not only folks in that field on GitHub.
I guess that was your intention anyway.
Emily Jiang
Sep 20, 2017, 8:43:03 AM9/20/17
to Eclipse MicroProfile
+1 Heiko!
I found this github limitation is pretty unfortunate. Maybe we can use @xx in the PR for them to review. Any other thoughts?
Emily
Kevin Sutter
Sep 20, 2017, 8:54:05 AM9/20/17
to Eclipse MicroProfile
I invite multiple people to review a PR since I don't know everybody's schedule and availability. But, I usually only wait for a single review to continue with the merge (unless I have to make updates, of course). There may be times that I definitely want a particular person to review something due to their expertise and experience. In that case, I will probably ping them directly to ensure that they saw the review request.
I think every PR should have at least one more set of eyes take a look, even for a typo. The requirement for a review/approver was discussed previously. And, I did find out it's doable. We just need to file a bug tracker with Eclipse to turn it on. I was going to wait until after 1.2 was done before pursuing this again, but I think we're close enough to 1.2 to move ahead.
The other half of the question is who should merge these reviewed/approved changes? My preference would be to let the submitter of the PR do the merge. But, if there is only one reviewer requested, then either the approver or the submitter can do the merge. If there are multiple reviewers, then the merge should probably be left up to the submitter since we don't know the intent of having multiple reviewers. And, of course, the submitter may not have privileges where s/he has to rely on somebody else to do the merge.
Or, to summarize, just use common sense... :-)
-- Kevin
I think every PR should have at least one more set of eyes take a look, even for a typo. The requirement for a review/approver was discussed previously. And, I did find out it's doable. We just need to file a bug tracker with Eclipse to turn it on. I was going to wait until after 1.2 was done before pursuing this again, but I think we're close enough to 1.2 to move ahead.
The other half of the question is who should merge these reviewed/approved changes? My preference would be to let the submitter of the PR do the merge. But, if there is only one reviewer requested, then either the approver or the submitter can do the merge. If there are multiple reviewers, then the merge should probably be left up to the submitter since we don't know the intent of having multiple reviewers. And, of course, the submitter may not have privileges where s/he has to rely on somebody else to do the merge.
Or, to summarize, just use common sense... :-)
-- Kevin
Reply all
Reply to author
Forward
0 new messages