Re: [DISCUSS] Hygene for merging PRs
Hi, Cameron, That's generally the case. Thanks for Xinyu to bring this to our attention! +1 to the stated guidelines. -Yi On Fri, May 17, 2019 at 11:10 AM Cameron Lee wrote: > Thanks Xinyu for starting this thread. > I support the guidelines that you mentioned, with a couple clarifications > regarding "PR Review": If a Samza PR is authored by a committer, then > another second committer should provide an approval before that code is > merged, correct? Once the second committer provides approval, then are we > ok with any committer (including the PR author) doing the merge? > > Cameron > > On Thu, May 16, 2019 at 11:30 AM Xinyu Liu wrote: > > > Hi, all, > > > > I've seen different practices around how PRs are contributed, reviewed > and > > merged for Samza open source. I think it's time to bring up our committer > > guide again to make sure we follow exactly the guidelines. It's also an > > opportunity to talk about future improvement to the flow. > > > > *PR Contribution* > > According to our committer guide [1], a JIRA must be created before > > creating the PR, unless the PR is trivial typo or doc fixes. The PR needs > > to have the JIRA ticket name in the following format: > > > > *SAMZA- : * > > > > As an example: > > SAMZA-2168: Remove redundant SystemAdmin creation in ApplicationMaster > [2]. > > > > *PR Review* > > As discussed before, a Samza PR requires an approval from a committer > > before merging. Contributors are welcome to review the code, but a final > > "LGTM" from a committer is a MUST. > > > > *PR merge* > > As we now use the simple merge flow in github to merge a PR, I think we > > should mostly squash the commits for merging.Otherwise it's hard to roll > > back changes and it generally generates a lot of noise in the commit > > history. > > > > Any further suggestions are highly appreciated. > > > > Thanks, > > Xinyu > > > > [1] > > https://cwiki.apache.org/confluence/display/SAMZA/Contributor%27s+Corner > > [2] https://github.com/apache/samza/pull/1001 > > >
Re: [DISCUSS] Hygene for merging PRs
Thanks Xinyu for starting this thread. I support the guidelines that you mentioned, with a couple clarifications regarding "PR Review": If a Samza PR is authored by a committer, then another second committer should provide an approval before that code is merged, correct? Once the second committer provides approval, then are we ok with any committer (including the PR author) doing the merge? Cameron On Thu, May 16, 2019 at 11:30 AM Xinyu Liu wrote: > Hi, all, > > I've seen different practices around how PRs are contributed, reviewed and > merged for Samza open source. I think it's time to bring up our committer > guide again to make sure we follow exactly the guidelines. It's also an > opportunity to talk about future improvement to the flow. > > *PR Contribution* > According to our committer guide [1], a JIRA must be created before > creating the PR, unless the PR is trivial typo or doc fixes. The PR needs > to have the JIRA ticket name in the following format: > > *SAMZA- : * > > As an example: > SAMZA-2168: Remove redundant SystemAdmin creation in ApplicationMaster [2]. > > *PR Review* > As discussed before, a Samza PR requires an approval from a committer > before merging. Contributors are welcome to review the code, but a final > "LGTM" from a committer is a MUST. > > *PR merge* > As we now use the simple merge flow in github to merge a PR, I think we > should mostly squash the commits for merging.Otherwise it's hard to roll > back changes and it generally generates a lot of noise in the commit > history. > > Any further suggestions are highly appreciated. > > Thanks, > Xinyu > > [1] > https://cwiki.apache.org/confluence/display/SAMZA/Contributor%27s+Corner > [2] https://github.com/apache/samza/pull/1001 >
[DISCUSS] Hygene for merging PRs
Hi, all, I've seen different practices around how PRs are contributed, reviewed and merged for Samza open source. I think it's time to bring up our committer guide again to make sure we follow exactly the guidelines. It's also an opportunity to talk about future improvement to the flow. *PR Contribution* According to our committer guide [1], a JIRA must be created before creating the PR, unless the PR is trivial typo or doc fixes. The PR needs to have the JIRA ticket name in the following format: *SAMZA- : * As an example: SAMZA-2168: Remove redundant SystemAdmin creation in ApplicationMaster [2]. *PR Review* As discussed before, a Samza PR requires an approval from a committer before merging. Contributors are welcome to review the code, but a final "LGTM" from a committer is a MUST. *PR merge* As we now use the simple merge flow in github to merge a PR, I think we should mostly squash the commits for merging.Otherwise it's hard to roll back changes and it generally generates a lot of noise in the commit history. Any further suggestions are highly appreciated. Thanks, Xinyu [1] https://cwiki.apache.org/confluence/display/SAMZA/Contributor%27s+Corner [2] https://github.com/apache/samza/pull/1001
