You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@samza.apache.org by Xinyu Liu <xi...@gmail.com> on 2019/05/16 18:29:57 UTC

[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-<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

Re: [DISCUSS] Hygene for merging PRs

Posted by Yi Pan <ni...@gmail.com>.

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 <ca...@gmail.com>
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 <xi...@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
> >
>

Re: [DISCUSS] Hygene for merging PRs

Posted by Cameron Lee <ca...@gmail.com>.

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 <xi...@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
>