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 <xinyuliu...@gmail.com> 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-<JiraNumber> : <JiraTitle>* > > 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 >
