samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cameron Lee <cameronlee...@gmail.com>
Subject Re: [DISCUSS] Hygene for merging PRs
Date Fri, 17 May 2019 18:10:12 GMT
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.us@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message