[DISCUSS] Start new Review Process

[Fabian Hueske-2]

Hi everyone,

A few months ago the community discussed and agreed to improve the PR
review process [1-4].
The idea is to follow a checklist to avoid in-depth reviews on
contributions that might not be accepted for other reasons. Thereby,
reviewers and contributors do not spend their time on PRs that will not be
merged.
The checklist consists of five points:

1. The contribution is well-described.
2. There is consensus that the contribution should go into to Flink.
3. [Does not need specific attention | Needs specific attention for X | Has
attention for X by Y]
4. The architectural approach is sound.
5. Overall code quality is good.

Back then we added a review guide to the website [5] but did not put the
new process in place yet. I would like to start this now.
There is a PR [6] that adds the review checklist to the PR template.
Committers who review add PR should follow the checklist and tick and sign
off the boxes by updating the PR description. For that committers need to
be members of the ASF Github organization.

If nobody has concerns, I'll merge the PR in a few days.
Once the PR is merged, the reviews of all new PRs should follow the
checklist.

Best,
Fabian

[1]
https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
[2]
https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
[3]
https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
[4]
https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
[5] https://flink.apache.org/reviewing-prs.html
[6] https://github.com/apache/flink/pull/6873

[Robert Metzger]

Hey,

as I've mentioned already in the pull request, I have started implementing
a little bot for GitHub that tracks the checklist [1]
The bot is monitoring incoming pull requests. It creates a comment with the
checklist.
Reviewers can write a message to the bot (such as "@flinkbot approve
contribution"), then the bot will update the checklist comment.

As an upcoming feature, I also plan to add a label to the pull request when
all the checklist conditions have been met.

I hope to finish the bot today. After some initial testing, we can deploy
it with Flink (if there are no objections by the community).


[1] https://github.com/rmetzger/flink-community-tools


On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]> wrote:

> Hi everyone,
>
> A few months ago the community discussed and agreed to improve the PR
> review process [1-4].
> The idea is to follow a checklist to avoid in-depth reviews on
> contributions that might not be accepted for other reasons. Thereby,
> reviewers and contributors do not spend their time on PRs that will not be
> merged.
> The checklist consists of five points:
>
> 1. The contribution is well-described.
> 2. There is consensus that the contribution should go into to Flink.
> 3. [Does not need specific attention | Needs specific attention for X | Has
> attention for X by Y]
> 4. The architectural approach is sound.
> 5. Overall code quality is good.
>
> Back then we added a review guide to the website [5] but did not put the
> new process in place yet. I would like to start this now.
> There is a PR [6] that adds the review checklist to the PR template.
> Committers who review add PR should follow the checklist and tick and sign
> off the boxes by updating the PR description. For that committers need to
> be members of the ASF Github organization.
>
> If nobody has concerns, I'll merge the PR in a few days.
> Once the PR is merged, the reviews of all new PRs should follow the
> checklist.
>
> Best,
> Fabian
>
> [1]
>
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> [2]
>
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> [3]
>
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> [4]
>
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> [5] https://flink.apache.org/reviewing-prs.html
> [6] https://github.com/apache/flink/pull/6873
>

[Fabian Hueske-2]

Oh, that's great news!
In that case we can just close the PR and start with the bot right away.
I think it would be good to extend the PR Review guide [1] with a section
about the bot and how to use it.

Fabian

[1] https://flink.apache.org/reviewing-prs.html

Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
[hidden email]>:

> Hey,
>
> as I've mentioned already in the pull request, I have started implementing
> a little bot for GitHub that tracks the checklist [1]
> The bot is monitoring incoming pull requests. It creates a comment with the
> checklist.
> Reviewers can write a message to the bot (such as "@flinkbot approve
> contribution"), then the bot will update the checklist comment.
>
> As an upcoming feature, I also plan to add a label to the pull request when
> all the checklist conditions have been met.
>
> I hope to finish the bot today. After some initial testing, we can deploy
> it with Flink (if there are no objections by the community).
>
>
> [1] https://github.com/rmetzger/flink-community-tools
>
>
> On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]> wrote:
>
> > Hi everyone,
> >
> > A few months ago the community discussed and agreed to improve the PR
> > review process [1-4].
> > The idea is to follow a checklist to avoid in-depth reviews on
> > contributions that might not be accepted for other reasons. Thereby,
> > reviewers and contributors do not spend their time on PRs that will not
> be
> > merged.
> > The checklist consists of five points:
> >
> > 1. The contribution is well-described.
> > 2. There is consensus that the contribution should go into to Flink.
> > 3. [Does not need specific attention | Needs specific attention for X |
> Has
> > attention for X by Y]
> > 4. The architectural approach is sound.
> > 5. Overall code quality is good.
> >
> > Back then we added a review guide to the website [5] but did not put the
> > new process in place yet. I would like to start this now.
> > There is a PR [6] that adds the review checklist to the PR template.
> > Committers who review add PR should follow the checklist and tick and
> sign
> > off the boxes by updating the PR description. For that committers need to
> > be members of the ASF Github organization.
> >
> > If nobody has concerns, I'll merge the PR in a few days.
> > Once the PR is merged, the reviews of all new PRs should follow the
> > checklist.
> >
> > Best,
> > Fabian
> >
> > [1]
> >
> >
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > [2]
> >
> >
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > [3]
> >
> >
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > [4]
> >
> >
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > [5] https://flink.apache.org/reviewing-prs.html
> > [6] https://github.com/apache/flink/pull/6873
> >
>

[Robert Metzger]

Okay, cool! I'll let you know when the bot is ready in a test repo.
While you (and others) are testing it, I'll open a PR for the docs.

On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <[hidden email]> wrote:

> Oh, that's great news!
> In that case we can just close the PR and start with the bot right away.
> I think it would be good to extend the PR Review guide [1] with a section
> about the bot and how to use it.
>
> Fabian
>
> [1] https://flink.apache.org/reviewing-prs.html
>
> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> [hidden email]>:
>
> > Hey,
> >
> > as I've mentioned already in the pull request, I have started
> implementing
> > a little bot for GitHub that tracks the checklist [1]
> > The bot is monitoring incoming pull requests. It creates a comment with
> the
> > checklist.
> > Reviewers can write a message to the bot (such as "@flinkbot approve
> > contribution"), then the bot will update the checklist comment.
> >
> > As an upcoming feature, I also plan to add a label to the pull request
> when
> > all the checklist conditions have been met.
> >
> > I hope to finish the bot today. After some initial testing, we can deploy
> > it with Flink (if there are no objections by the community).
> >
> >
> > [1] https://github.com/rmetzger/flink-community-tools
> >
> >
> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]> wrote:
> >
> > > Hi everyone,
> > >
> > > A few months ago the community discussed and agreed to improve the PR
> > > review process [1-4].
> > > The idea is to follow a checklist to avoid in-depth reviews on
> > > contributions that might not be accepted for other reasons. Thereby,
> > > reviewers and contributors do not spend their time on PRs that will not
> > be
> > > merged.
> > > The checklist consists of five points:
> > >
> > > 1. The contribution is well-described.
> > > 2. There is consensus that the contribution should go into to Flink.
> > > 3. [Does not need specific attention | Needs specific attention for X |
> > Has
> > > attention for X by Y]
> > > 4. The architectural approach is sound.
> > > 5. Overall code quality is good.
> > >
> > > Back then we added a review guide to the website [5] but did not put
> the
> > > new process in place yet. I would like to start this now.
> > > There is a PR [6] that adds the review checklist to the PR template.
> > > Committers who review add PR should follow the checklist and tick and
> > sign
> > > off the boxes by updating the PR description. For that committers need
> to
> > > be members of the ASF Github organization.
> > >
> > > If nobody has concerns, I'll merge the PR in a few days.
> > > Once the PR is merged, the reviews of all new PRs should follow the
> > > checklist.
> > >
> > > Best,
> > > Fabian
> > >
> > > [1]
> > >
> > >
> >
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > > [2]
> > >
> > >
> >
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > > [3]
> > >
> > >
> >
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > > [4]
> > >
> > >
> >
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > > [5] https://flink.apache.org/reviewing-prs.html
> > > [6] https://github.com/apache/flink/pull/6873
> > >
> >
>

[Robert Metzger]

I have the bot now running in https://github.com/flinkbot/test-repo/pulls
Feel free to play with it.

On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <[hidden email]> wrote:

> Okay, cool! I'll let you know when the bot is ready in a test repo.
> While you (and others) are testing it, I'll open a PR for the docs.
>
> On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <[hidden email]> wrote:
>
>> Oh, that's great news!
>> In that case we can just close the PR and start with the bot right away.
>> I think it would be good to extend the PR Review guide [1] with a section
>> about the bot and how to use it.
>>
>> Fabian
>>
>> [1] https://flink.apache.org/reviewing-prs.html
>>
>> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
>> [hidden email]>:
>>
>> > Hey,
>> >
>> > as I've mentioned already in the pull request, I have started
>> implementing
>> > a little bot for GitHub that tracks the checklist [1]
>> > The bot is monitoring incoming pull requests. It creates a comment with
>> the
>> > checklist.
>> > Reviewers can write a message to the bot (such as "@flinkbot approve
>> > contribution"), then the bot will update the checklist comment.
>> >
>> > As an upcoming feature, I also plan to add a label to the pull request
>> when
>> > all the checklist conditions have been met.
>> >
>> > I hope to finish the bot today. After some initial testing, we can
>> deploy
>> > it with Flink (if there are no objections by the community).
>> >
>> >
>> > [1] https://github.com/rmetzger/flink-community-tools
>> >
>> >
>> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]>
>> wrote:
>> >
>> > > Hi everyone,
>> > >
>> > > A few months ago the community discussed and agreed to improve the PR
>> > > review process [1-4].
>> > > The idea is to follow a checklist to avoid in-depth reviews on
>> > > contributions that might not be accepted for other reasons. Thereby,
>> > > reviewers and contributors do not spend their time on PRs that will
>> not
>> > be
>> > > merged.
>> > > The checklist consists of five points:
>> > >
>> > > 1. The contribution is well-described.
>> > > 2. There is consensus that the contribution should go into to Flink.
>> > > 3. [Does not need specific attention | Needs specific attention for X
>> |
>> > Has
>> > > attention for X by Y]
>> > > 4. The architectural approach is sound.
>> > > 5. Overall code quality is good.
>> > >
>> > > Back then we added a review guide to the website [5] but did not put
>> the
>> > > new process in place yet. I would like to start this now.
>> > > There is a PR [6] that adds the review checklist to the PR template.
>> > > Committers who review add PR should follow the checklist and tick and
>> > sign
>> > > off the boxes by updating the PR description. For that committers
>> need to
>> > > be members of the ASF Github organization.
>> > >
>> > > If nobody has concerns, I'll merge the PR in a few days.
>> > > Once the PR is merged, the reviews of all new PRs should follow the
>> > > checklist.
>> > >
>> > > Best,
>> > > Fabian
>> > >
>> > > [1]
>> > >
>> > >
>> >
>> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
>> > > [2]
>> > >
>> > >
>> >
>> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
>> > > [3]
>> > >
>> > >
>> >
>> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
>> > > [4]
>> > >
>> > >
>> >
>> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
>> > > [5] https://flink.apache.org/reviewing-prs.html
>> > > [6] https://github.com/apache/flink/pull/6873
>> > >
>> >
>>
>

[Fabian Hueske-2]

Hi Robert,

Thanks for working on the bot!
I have a few suggestions / questions:

Suggestions:
1) It would be great to approve multiple boxes in one comment. Either as
> @flinkbot approve contribution consensus
or by
> @flinkbot approve contribution
> @flinkbot approve consensus

2) Extend the last line of the description by adding something like "See
Pull Request Review Guide for how to use the Flink bot."?
3) Rephrase the first line to include "[description]" instead of
"[contribution]", as it is about approving the description

Questions:
1) Can anybody use the bot or just committers?
2) Does it make sense to enforce the order in which approvals are given?

Best,
Fabian

Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
[hidden email]>:

> I have the bot now running in https://github.com/flinkbot/test-repo/pulls
> Feel free to play with it.
>
> On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <[hidden email]>
> wrote:
>
> > Okay, cool! I'll let you know when the bot is ready in a test repo.
> > While you (and others) are testing it, I'll open a PR for the docs.
> >
> > On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <[hidden email]>
> wrote:
> >
> >> Oh, that's great news!
> >> In that case we can just close the PR and start with the bot right away.
> >> I think it would be good to extend the PR Review guide [1] with a
> section
> >> about the bot and how to use it.
> >>
> >> Fabian
> >>
> >> [1] https://flink.apache.org/reviewing-prs.html
> >>
> >> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> >> [hidden email]>:
> >>
> >> > Hey,
> >> >
> >> > as I've mentioned already in the pull request, I have started
> >> implementing
> >> > a little bot for GitHub that tracks the checklist [1]
> >> > The bot is monitoring incoming pull requests. It creates a comment
> with
> >> the
> >> > checklist.
> >> > Reviewers can write a message to the bot (such as "@flinkbot approve
> >> > contribution"), then the bot will update the checklist comment.
> >> >
> >> > As an upcoming feature, I also plan to add a label to the pull request
> >> when
> >> > all the checklist conditions have been met.
> >> >
> >> > I hope to finish the bot today. After some initial testing, we can
> >> deploy
> >> > it with Flink (if there are no objections by the community).
> >> >
> >> >
> >> > [1] https://github.com/rmetzger/flink-community-tools
> >> >
> >> >
> >> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]>
> >> wrote:
> >> >
> >> > > Hi everyone,
> >> > >
> >> > > A few months ago the community discussed and agreed to improve the
> PR
> >> > > review process [1-4].
> >> > > The idea is to follow a checklist to avoid in-depth reviews on
> >> > > contributions that might not be accepted for other reasons. Thereby,
> >> > > reviewers and contributors do not spend their time on PRs that will
> >> not
> >> > be
> >> > > merged.
> >> > > The checklist consists of five points:
> >> > >
> >> > > 1. The contribution is well-described.
> >> > > 2. There is consensus that the contribution should go into to Flink.
> >> > > 3. [Does not need specific attention | Needs specific attention for
> X
> >> |
> >> > Has
> >> > > attention for X by Y]
> >> > > 4. The architectural approach is sound.
> >> > > 5. Overall code quality is good.
> >> > >
> >> > > Back then we added a review guide to the website [5] but did not put
> >> the
> >> > > new process in place yet. I would like to start this now.
> >> > > There is a PR [6] that adds the review checklist to the PR template.
> >> > > Committers who review add PR should follow the checklist and tick
> and
> >> > sign
> >> > > off the boxes by updating the PR description. For that committers
> >> need to
> >> > > be members of the ASF Github organization.
> >> > >
> >> > > If nobody has concerns, I'll merge the PR in a few days.
> >> > > Once the PR is merged, the reviews of all new PRs should follow the
> >> > > checklist.
> >> > >
> >> > > Best,
> >> > > Fabian
> >> > >
> >> > > [1]
> >> > >
> >> > >
> >> >
> >>
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> >> > > [2]
> >> > >
> >> > >
> >> >
> >>
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> >> > > [3]
> >> > >
> >> > >
> >> >
> >>
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> >> > > [4]
> >> > >
> >> > >
> >> >
> >>
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> >> > > [5] https://flink.apache.org/reviewing-prs.html
> >> > > [6] https://github.com/apache/flink/pull/6873
> >> > >
> >> >
> >>
> >
>

[Ufuk Celebi-2]

I played around with the bot and it works pretty well. :-) @Robert:
Are there any plans to contribute the code for the bot to Apache
(potentially in another repository)?

I like Fabians suggestions. Regarding the questions:
1) I would make that dependent on whether you expected the review
guideline to only apply to committers or also regular contributors.
Since the bot is not merging PRs, I don't see a down side in having it
open for all contributors (except potential noise).
2) What do you mean with "order of approvals"?

Since there is another lively discussion going on about a bot for
stale PRs, I'm wondering what the future plans for @flinkbot are. Do
we want to have multiple bots for the project? Or do you plan to
support staleness checks in the future? What about merging of PRs?

– Ufuk

On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]> wrote:

>
> Hi Robert,
>
> Thanks for working on the bot!
> I have a few suggestions / questions:
>
> Suggestions:
> 1) It would be great to approve multiple boxes in one comment. Either as
> > @flinkbot approve contribution consensus
> or by
> > @flinkbot approve contribution
> > @flinkbot approve consensus
>
> 2) Extend the last line of the description by adding something like "See
> Pull Request Review Guide for how to use the Flink bot."?
> 3) Rephrase the first line to include "[description]" instead of
> "[contribution]", as it is about approving the description
>
> Questions:
> 1) Can anybody use the bot or just committers?
> 2) Does it make sense to enforce the order in which approvals are given?
>
> Best,
> Fabian
>
> Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
> [hidden email]>:
>
> > I have the bot now running in https://github.com/flinkbot/test-repo/pulls
> > Feel free to play with it.
> >
> > On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <[hidden email]>
> > wrote:
> >
> > > Okay, cool! I'll let you know when the bot is ready in a test repo.
> > > While you (and others) are testing it, I'll open a PR for the docs.
> > >
> > > On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <[hidden email]>
> > wrote:
> > >
> > >> Oh, that's great news!
> > >> In that case we can just close the PR and start with the bot right away.
> > >> I think it would be good to extend the PR Review guide [1] with a
> > section
> > >> about the bot and how to use it.
> > >>
> > >> Fabian
> > >>
> > >> [1] https://flink.apache.org/reviewing-prs.html
> > >>
> > >> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> > >> [hidden email]>:
> > >>
> > >> > Hey,
> > >> >
> > >> > as I've mentioned already in the pull request, I have started
> > >> implementing
> > >> > a little bot for GitHub that tracks the checklist [1]
> > >> > The bot is monitoring incoming pull requests. It creates a comment
> > with
> > >> the
> > >> > checklist.
> > >> > Reviewers can write a message to the bot (such as "@flinkbot approve
> > >> > contribution"), then the bot will update the checklist comment.
> > >> >
> > >> > As an upcoming feature, I also plan to add a label to the pull request
> > >> when
> > >> > all the checklist conditions have been met.
> > >> >
> > >> > I hope to finish the bot today. After some initial testing, we can
> > >> deploy
> > >> > it with Flink (if there are no objections by the community).
> > >> >
> > >> >
> > >> > [1] https://github.com/rmetzger/flink-community-tools
> > >> >
> > >> >
> > >> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]>
> > >> wrote:
> > >> >
> > >> > > Hi everyone,
> > >> > >
> > >> > > A few months ago the community discussed and agreed to improve the
> > PR
> > >> > > review process [1-4].
> > >> > > The idea is to follow a checklist to avoid in-depth reviews on
> > >> > > contributions that might not be accepted for other reasons. Thereby,
> > >> > > reviewers and contributors do not spend their time on PRs that will
> > >> not
> > >> > be
> > >> > > merged.
> > >> > > The checklist consists of five points:
> > >> > >
> > >> > > 1. The contribution is well-described.
> > >> > > 2. There is consensus that the contribution should go into to Flink.
> > >> > > 3. [Does not need specific attention | Needs specific attention for
> > X
> > >> |
> > >> > Has
> > >> > > attention for X by Y]
> > >> > > 4. The architectural approach is sound.
> > >> > > 5. Overall code quality is good.
> > >> > >
> > >> > > Back then we added a review guide to the website [5] but did not put
> > >> the
> > >> > > new process in place yet. I would like to start this now.
> > >> > > There is a PR [6] that adds the review checklist to the PR template.
> > >> > > Committers who review add PR should follow the checklist and tick
> > and
> > >> > sign
> > >> > > off the boxes by updating the PR description. For that committers
> > >> need to
> > >> > > be members of the ASF Github organization.
> > >> > >
> > >> > > If nobody has concerns, I'll merge the PR in a few days.
> > >> > > Once the PR is merged, the reviews of all new PRs should follow the
> > >> > > checklist.
> > >> > >
> > >> > > Best,
> > >> > > Fabian
> > >> > >
> > >> > > [1]
> > >> > >
> > >> > >
> > >> >
> > >>
> > https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > >> > > [2]
> > >> > >
> > >> > >
> > >> >
> > >>
> > https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > >> > > [3]
> > >> > >
> > >> > >
> > >> >
> > >>
> > https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > >> > > [4]
> > >> > >
> > >> > >
> > >> >
> > >>
> > https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > >> > > [5] https://flink.apache.org/reviewing-prs.html
> > >> > > [6] https://github.com/apache/flink/pull/6873
> > >> > >
> > >> >
> > >>
> > >
> >

[Fabian Hueske-2]

The points in the review template are in the order in which they should be
checked, i.e., first checking the description, then consensus and finally
checking the code.
Currently, it is possible to tick off the code box before checking the
description.
One motivation for the process was to do the steps in exactly the proposed
order for example to to avoid detailed code reviews before there was
consensus whether the contribution was welcome or not.

Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <[hidden email]>:

> I played around with the bot and it works pretty well. :-) @Robert:
> Are there any plans to contribute the code for the bot to Apache
> (potentially in another repository)?
>
> I like Fabians suggestions. Regarding the questions:
> 1) I would make that dependent on whether you expected the review
> guideline to only apply to committers or also regular contributors.
> Since the bot is not merging PRs, I don't see a down side in having it
> open for all contributors (except potential noise).
> 2) What do you mean with "order of approvals"?
>
> Since there is another lively discussion going on about a bot for
> stale PRs, I'm wondering what the future plans for @flinkbot are. Do
> we want to have multiple bots for the project? Or do you plan to
> support staleness checks in the future? What about merging of PRs?
>
> – Ufuk
>
> On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]> wrote:
> >
> > Hi Robert,
> >
> > Thanks for working on the bot!
> > I have a few suggestions / questions:
> >
> > Suggestions:
> > 1) It would be great to approve multiple boxes in one comment. Either as
> > > @flinkbot approve contribution consensus
> > or by
> > > @flinkbot approve contribution
> > > @flinkbot approve consensus
> >
> > 2) Extend the last line of the description by adding something like "See
> > Pull Request Review Guide for how to use the Flink bot."?
> > 3) Rephrase the first line to include "[description]" instead of
> > "[contribution]", as it is about approving the description
> >
> > Questions:
> > 1) Can anybody use the bot or just committers?
> > 2) Does it make sense to enforce the order in which approvals are given?
> >
> > Best,
> > Fabian
> >
> > Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
> > [hidden email]>:
> >
> > > I have the bot now running in
> https://github.com/flinkbot/test-repo/pulls
> > > Feel free to play with it.
> > >
> > > On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <[hidden email]>
> > > wrote:
> > >
> > > > Okay, cool! I'll let you know when the bot is ready in a test repo.
> > > > While you (and others) are testing it, I'll open a PR for the docs.
> > > >
> > > > On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <[hidden email]>
> > > wrote:
> > > >
> > > >> Oh, that's great news!
> > > >> In that case we can just close the PR and start with the bot right
> away.
> > > >> I think it would be good to extend the PR Review guide [1] with a
> > > section
> > > >> about the bot and how to use it.
> > > >>
> > > >> Fabian
> > > >>
> > > >> [1] https://flink.apache.org/reviewing-prs.html
> > > >>
> > > >> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> > > >> [hidden email]>:
> > > >>
> > > >> > Hey,
> > > >> >
> > > >> > as I've mentioned already in the pull request, I have started
> > > >> implementing
> > > >> > a little bot for GitHub that tracks the checklist [1]
> > > >> > The bot is monitoring incoming pull requests. It creates a comment
> > > with
> > > >> the
> > > >> > checklist.
> > > >> > Reviewers can write a message to the bot (such as "@flinkbot
> approve
> > > >> > contribution"), then the bot will update the checklist comment.
> > > >> >
> > > >> > As an upcoming feature, I also plan to add a label to the pull
> request
> > > >> when
> > > >> > all the checklist conditions have been met.
> > > >> >
> > > >> > I hope to finish the bot today. After some initial testing, we can
> > > >> deploy
> > > >> > it with Flink (if there are no objections by the community).
> > > >> >
> > > >> >
> > > >> > [1] https://github.com/rmetzger/flink-community-tools
> > > >> >
> > > >> >
> > > >> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]>
> > > >> wrote:
> > > >> >
> > > >> > > Hi everyone,
> > > >> > >
> > > >> > > A few months ago the community discussed and agreed to improve
> the
> > > PR
> > > >> > > review process [1-4].
> > > >> > > The idea is to follow a checklist to avoid in-depth reviews on
> > > >> > > contributions that might not be accepted for other reasons.
> Thereby,
> > > >> > > reviewers and contributors do not spend their time on PRs that
> will
> > > >> not
> > > >> > be
> > > >> > > merged.
> > > >> > > The checklist consists of five points:
> > > >> > >
> > > >> > > 1. The contribution is well-described.
> > > >> > > 2. There is consensus that the contribution should go into to
> Flink.
> > > >> > > 3. [Does not need specific attention | Needs specific attention
> for
> > > X
> > > >> |
> > > >> > Has
> > > >> > > attention for X by Y]
> > > >> > > 4. The architectural approach is sound.
> > > >> > > 5. Overall code quality is good.
> > > >> > >
> > > >> > > Back then we added a review guide to the website [5] but did
> not put
> > > >> the
> > > >> > > new process in place yet. I would like to start this now.
> > > >> > > There is a PR [6] that adds the review checklist to the PR
> template.
> > > >> > > Committers who review add PR should follow the checklist and
> tick
> > > and
> > > >> > sign
> > > >> > > off the boxes by updating the PR description. For that
> committers
> > > >> need to
> > > >> > > be members of the ASF Github organization.
> > > >> > >
> > > >> > > If nobody has concerns, I'll merge the PR in a few days.
> > > >> > > Once the PR is merged, the reviews of all new PRs should follow
> the
> > > >> > > checklist.
> > > >> > >
> > > >> > > Best,
> > > >> > > Fabian
> > > >> > >
> > > >> > > [1]
> > > >> > >
> > > >> > >
> > > >> >
> > > >>
> > >
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > > >> > > [2]
> > > >> > >
> > > >> > >
> > > >> >
> > > >>
> > >
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > > >> > > [3]
> > > >> > >
> > > >> > >
> > > >> >
> > > >>
> > >
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > > >> > > [4]
> > > >> > >
> > > >> > >
> > > >> >
> > > >>
> > >
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > > >> > > [5] https://flink.apache.org/reviewing-prs.html
> > > >> > > [6] https://github.com/apache/flink/pull/6873
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
>

[Ufuk Celebi-2]

Thanks for the clarification. I agree that it only makes sense to
check the points in order. +1 to add this if we can think of a nice
way to do it. I'm not sure how we would enforce the order with the bot
since there is only indirect feedback to a bot command. The only thing
I can think of at the moment is to add a note to a check in case
earlier steps where not executed. Just ignoring a bot command because
other commands have not been executed before feels not helpful to me
(since we can't prevent reviewers to jump to later steps if they wish
to do so).

I'd rather add a bold note to the bot template that makes clear that
all points should be followed in order to avoid potentially redundant
work.

– Ufuk

On Mon, Jan 28, 2019 at 5:01 PM Fabian Hueske <[hidden email]> wrote:

>
> The points in the review template are in the order in which they should be
> checked, i.e., first checking the description, then consensus and finally
> checking the code.
> Currently, it is possible to tick off the code box before checking the
> description.
> One motivation for the process was to do the steps in exactly the proposed
> order for example to to avoid detailed code reviews before there was
> consensus whether the contribution was welcome or not.
>
> Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <[hidden email]>:
>
> > I played around with the bot and it works pretty well. :-) @Robert:
> > Are there any plans to contribute the code for the bot to Apache
> > (potentially in another repository)?
> >
> > I like Fabians suggestions. Regarding the questions:
> > 1) I would make that dependent on whether you expected the review
> > guideline to only apply to committers or also regular contributors.
> > Since the bot is not merging PRs, I don't see a down side in having it
> > open for all contributors (except potential noise).
> > 2) What do you mean with "order of approvals"?
> >
> > Since there is another lively discussion going on about a bot for
> > stale PRs, I'm wondering what the future plans for @flinkbot are. Do
> > we want to have multiple bots for the project? Or do you plan to
> > support staleness checks in the future? What about merging of PRs?
> >
> > – Ufuk
> >
> > On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]> wrote:
> > >
> > > Hi Robert,
> > >
> > > Thanks for working on the bot!
> > > I have a few suggestions / questions:
> > >
> > > Suggestions:
> > > 1) It would be great to approve multiple boxes in one comment. Either as
> > > > @flinkbot approve contribution consensus
> > > or by
> > > > @flinkbot approve contribution
> > > > @flinkbot approve consensus
> > >
> > > 2) Extend the last line of the description by adding something like "See
> > > Pull Request Review Guide for how to use the Flink bot."?
> > > 3) Rephrase the first line to include "[description]" instead of
> > > "[contribution]", as it is about approving the description
> > >
> > > Questions:
> > > 1) Can anybody use the bot or just committers?
> > > 2) Does it make sense to enforce the order in which approvals are given?
> > >
> > > Best,
> > > Fabian
> > >
> > > Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
> > > [hidden email]>:
> > >
> > > > I have the bot now running in
> > https://github.com/flinkbot/test-repo/pulls
> > > > Feel free to play with it.
> > > >
> > > > On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <[hidden email]>
> > > > wrote:
> > > >
> > > > > Okay, cool! I'll let you know when the bot is ready in a test repo.
> > > > > While you (and others) are testing it, I'll open a PR for the docs.
> > > > >
> > > > > On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <[hidden email]>
> > > > wrote:
> > > > >
> > > > >> Oh, that's great news!
> > > > >> In that case we can just close the PR and start with the bot right
> > away.
> > > > >> I think it would be good to extend the PR Review guide [1] with a
> > > > section
> > > > >> about the bot and how to use it.
> > > > >>
> > > > >> Fabian
> > > > >>
> > > > >> [1] https://flink.apache.org/reviewing-prs.html
> > > > >>
> > > > >> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> > > > >> [hidden email]>:
> > > > >>
> > > > >> > Hey,
> > > > >> >
> > > > >> > as I've mentioned already in the pull request, I have started
> > > > >> implementing
> > > > >> > a little bot for GitHub that tracks the checklist [1]
> > > > >> > The bot is monitoring incoming pull requests. It creates a comment
> > > > with
> > > > >> the
> > > > >> > checklist.
> > > > >> > Reviewers can write a message to the bot (such as "@flinkbot
> > approve
> > > > >> > contribution"), then the bot will update the checklist comment.
> > > > >> >
> > > > >> > As an upcoming feature, I also plan to add a label to the pull
> > request
> > > > >> when
> > > > >> > all the checklist conditions have been met.
> > > > >> >
> > > > >> > I hope to finish the bot today. After some initial testing, we can
> > > > >> deploy
> > > > >> > it with Flink (if there are no objections by the community).
> > > > >> >
> > > > >> >
> > > > >> > [1] https://github.com/rmetzger/flink-community-tools
> > > > >> >
> > > > >> >
> > > > >> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <[hidden email]>
> > > > >> wrote:
> > > > >> >
> > > > >> > > Hi everyone,
> > > > >> > >
> > > > >> > > A few months ago the community discussed and agreed to improve
> > the
> > > > PR
> > > > >> > > review process [1-4].
> > > > >> > > The idea is to follow a checklist to avoid in-depth reviews on
> > > > >> > > contributions that might not be accepted for other reasons.
> > Thereby,
> > > > >> > > reviewers and contributors do not spend their time on PRs that
> > will
> > > > >> not
> > > > >> > be
> > > > >> > > merged.
> > > > >> > > The checklist consists of five points:
> > > > >> > >
> > > > >> > > 1. The contribution is well-described.
> > > > >> > > 2. There is consensus that the contribution should go into to
> > Flink.
> > > > >> > > 3. [Does not need specific attention | Needs specific attention
> > for
> > > > X
> > > > >> |
> > > > >> > Has
> > > > >> > > attention for X by Y]
> > > > >> > > 4. The architectural approach is sound.
> > > > >> > > 5. Overall code quality is good.
> > > > >> > >
> > > > >> > > Back then we added a review guide to the website [5] but did
> > not put
> > > > >> the
> > > > >> > > new process in place yet. I would like to start this now.
> > > > >> > > There is a PR [6] that adds the review checklist to the PR
> > template.
> > > > >> > > Committers who review add PR should follow the checklist and
> > tick
> > > > and
> > > > >> > sign
> > > > >> > > off the boxes by updating the PR description. For that
> > committers
> > > > >> need to
> > > > >> > > be members of the ASF Github organization.
> > > > >> > >
> > > > >> > > If nobody has concerns, I'll merge the PR in a few days.
> > > > >> > > Once the PR is merged, the reviews of all new PRs should follow
> > the
> > > > >> > > checklist.
> > > > >> > >
> > > > >> > > Best,
> > > > >> > > Fabian
> > > > >> > >
> > > > >> > > [1]
> > > > >> > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > > > >> > > [2]
> > > > >> > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > > > >> > > [3]
> > > > >> > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > > > >> > > [4]
> > > > >> > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > > > >> > > [5] https://flink.apache.org/reviewing-prs.html
> > > > >> > > [6] https://github.com/apache/flink/pull/6873
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > >
> >

[Robert Metzger]

@Fabian: Thank you for your suggestions. Multiple approvals in one comment
are already possible.
I will see if I can easily add multiple approvals in one line as well.
I will also address 2) and 3).

Regarding usage of the bot: Anyone can use it! It is up to the committer
who's merging in the end whether they are happy with the approver.
One of the feature ideas I have is to indicate whether somebody is PMC or
committer.

I'm against enforcing the order of approvals for now. I fear that this will
make the tool too restrictive. I like Ufuk's idea of putting a note into
the tracking comment for now.
Once it's active and we are using it day to day, we'll probably learn what
features we need the most.


@Ufuk: I think we should put it into a Apache repo at some point. But I'm
not sure if it's worth going through the effort of setting up a new repo
now, given that the bot is not even active yet, and we are not sure if it's
going to be useful.
Once it is active for a month or two, I will move it.

Regarding the bots in general: I don't see a problem with having multiple
bots in place, as long as they get along with each other well.
We should try not to reinvent the wheel, if there's already a good bot
implementation, I don't see a reason to not use it.
The problem in our case is that we have limited access to our GitHub page,
and merging can not happen through the GitHub user interface -- so I guess
many "off the shelf" bots will not work in our environment.
I'm thinking already about approaches how to automatically merge pull
requests with the bot. But this will be a separate mailing list thread :)

Thanks for the feedback!




On Mon, Jan 28, 2019 at 5:20 PM Ufuk Celebi <[hidden email]> wrote:

> Thanks for the clarification. I agree that it only makes sense to
> check the points in order. +1 to add this if we can think of a nice
> way to do it. I'm not sure how we would enforce the order with the bot
> since there is only indirect feedback to a bot command. The only thing
> I can think of at the moment is to add a note to a check in case
> earlier steps where not executed. Just ignoring a bot command because
> other commands have not been executed before feels not helpful to me
> (since we can't prevent reviewers to jump to later steps if they wish
> to do so).
>
> I'd rather add a bold note to the bot template that makes clear that
> all points should be followed in order to avoid potentially redundant
> work.
>
> – Ufuk
>
> On Mon, Jan 28, 2019 at 5:01 PM Fabian Hueske <[hidden email]> wrote:
> >
> > The points in the review template are in the order in which they should
> be
> > checked, i.e., first checking the description, then consensus and finally
> > checking the code.
> > Currently, it is possible to tick off the code box before checking the
> > description.
> > One motivation for the process was to do the steps in exactly the
> proposed
> > order for example to to avoid detailed code reviews before there was
> > consensus whether the contribution was welcome or not.
> >
> > Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <[hidden email]>:
> >
> > > I played around with the bot and it works pretty well. :-) @Robert:
> > > Are there any plans to contribute the code for the bot to Apache
> > > (potentially in another repository)?
> > >
> > > I like Fabians suggestions. Regarding the questions:
> > > 1) I would make that dependent on whether you expected the review
> > > guideline to only apply to committers or also regular contributors.
> > > Since the bot is not merging PRs, I don't see a down side in having it
> > > open for all contributors (except potential noise).
> > > 2) What do you mean with "order of approvals"?
> > >
> > > Since there is another lively discussion going on about a bot for
> > > stale PRs, I'm wondering what the future plans for @flinkbot are. Do
> > > we want to have multiple bots for the project? Or do you plan to
> > > support staleness checks in the future? What about merging of PRs?
> > >
> > > – Ufuk
> > >
> > > On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]>
> wrote:
> > > >
> > > > Hi Robert,
> > > >
> > > > Thanks for working on the bot!
> > > > I have a few suggestions / questions:
> > > >
> > > > Suggestions:
> > > > 1) It would be great to approve multiple boxes in one comment.
> Either as
> > > > > @flinkbot approve contribution consensus
> > > > or by
> > > > > @flinkbot approve contribution
> > > > > @flinkbot approve consensus
> > > >
> > > > 2) Extend the last line of the description by adding something like
> "See
> > > > Pull Request Review Guide for how to use the Flink bot."?
> > > > 3) Rephrase the first line to include "[description]" instead of
> > > > "[contribution]", as it is about approving the description
> > > >
> > > > Questions:
> > > > 1) Can anybody use the bot or just committers?
> > > > 2) Does it make sense to enforce the order in which approvals are
> given?
> > > >
> > > > Best,
> > > > Fabian
> > > >
> > > > Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
> > > > [hidden email]>:
> > > >
> > > > > I have the bot now running in
> > > https://github.com/flinkbot/test-repo/pulls
> > > > > Feel free to play with it.
> > > > >
> > > > > On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <
> [hidden email]>
> > > > > wrote:
> > > > >
> > > > > > Okay, cool! I'll let you know when the bot is ready in a test
> repo.
> > > > > > While you (and others) are testing it, I'll open a PR for the
> docs.
> > > > > >
> > > > > > On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <
> [hidden email]>
> > > > > wrote:
> > > > > >
> > > > > >> Oh, that's great news!
> > > > > >> In that case we can just close the PR and start with the bot
> right
> > > away.
> > > > > >> I think it would be good to extend the PR Review guide [1] with
> a
> > > > > section
> > > > > >> about the bot and how to use it.
> > > > > >>
> > > > > >> Fabian
> > > > > >>
> > > > > >> [1] https://flink.apache.org/reviewing-prs.html
> > > > > >>
> > > > > >> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> > > > > >> [hidden email]>:
> > > > > >>
> > > > > >> > Hey,
> > > > > >> >
> > > > > >> > as I've mentioned already in the pull request, I have started
> > > > > >> implementing
> > > > > >> > a little bot for GitHub that tracks the checklist [1]
> > > > > >> > The bot is monitoring incoming pull requests. It creates a
> comment
> > > > > with
> > > > > >> the
> > > > > >> > checklist.
> > > > > >> > Reviewers can write a message to the bot (such as "@flinkbot
> > > approve
> > > > > >> > contribution"), then the bot will update the checklist
> comment.
> > > > > >> >
> > > > > >> > As an upcoming feature, I also plan to add a label to the pull
> > > request
> > > > > >> when
> > > > > >> > all the checklist conditions have been met.
> > > > > >> >
> > > > > >> > I hope to finish the bot today. After some initial testing,
> we can
> > > > > >> deploy
> > > > > >> > it with Flink (if there are no objections by the community).
> > > > > >> >
> > > > > >> >
> > > > > >> > [1] https://github.com/rmetzger/flink-community-tools
> > > > > >> >
> > > > > >> >
> > > > > >> > On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <
> [hidden email]>
> > > > > >> wrote:
> > > > > >> >
> > > > > >> > > Hi everyone,
> > > > > >> > >
> > > > > >> > > A few months ago the community discussed and agreed to
> improve
> > > the
> > > > > PR
> > > > > >> > > review process [1-4].
> > > > > >> > > The idea is to follow a checklist to avoid in-depth reviews
> on
> > > > > >> > > contributions that might not be accepted for other reasons.
> > > Thereby,
> > > > > >> > > reviewers and contributors do not spend their time on PRs
> that
> > > will
> > > > > >> not
> > > > > >> > be
> > > > > >> > > merged.
> > > > > >> > > The checklist consists of five points:
> > > > > >> > >
> > > > > >> > > 1. The contribution is well-described.
> > > > > >> > > 2. There is consensus that the contribution should go into
> to
> > > Flink.
> > > > > >> > > 3. [Does not need specific attention | Needs specific
> attention
> > > for
> > > > > X
> > > > > >> |
> > > > > >> > Has
> > > > > >> > > attention for X by Y]
> > > > > >> > > 4. The architectural approach is sound.
> > > > > >> > > 5. Overall code quality is good.
> > > > > >> > >
> > > > > >> > > Back then we added a review guide to the website [5] but did
> > > not put
> > > > > >> the
> > > > > >> > > new process in place yet. I would like to start this now.
> > > > > >> > > There is a PR [6] that adds the review checklist to the PR
> > > template.
> > > > > >> > > Committers who review add PR should follow the checklist and
> > > tick
> > > > > and
> > > > > >> > sign
> > > > > >> > > off the boxes by updating the PR description. For that
> > > committers
> > > > > >> need to
> > > > > >> > > be members of the ASF Github organization.
> > > > > >> > >
> > > > > >> > > If nobody has concerns, I'll merge the PR in a few days.
> > > > > >> > > Once the PR is merged, the reviews of all new PRs should
> follow
> > > the
> > > > > >> > > checklist.
> > > > > >> > >
> > > > > >> > > Best,
> > > > > >> > > Fabian
> > > > > >> > >
> > > > > >> > > [1]
> > > > > >> > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > >
> > >
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > > > > >> > > [2]
> > > > > >> > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > >
> > >
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > > > > >> > > [3]
> > > > > >> > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > >
> > >
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > > > > >> > > [4]
> > > > > >> > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > >
> > >
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > > > > >> > > [5] https://flink.apache.org/reviewing-prs.html
> > > > > >> > > [6] https://github.com/apache/flink/pull/6873
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >
> > > > >
> > >
>

[Aljoscha Krettek-2]

What do you mean by “merging cannot happen through the GitHub user interface”? You can in fact merge PRs by clicking on the merge button, or “rebase and merge”.

Aljoscha

> On 29. Jan 2019, at 11:58, Robert Metzger <[hidden email]> wrote:
>
> @Fabian: Thank you for your suggestions. Multiple approvals in one comment
> are already possible.
> I will see if I can easily add multiple approvals in one line as well.
> I will also address 2) and 3).
>
> Regarding usage of the bot: Anyone can use it! It is up to the committer
> who's merging in the end whether they are happy with the approver.
> One of the feature ideas I have is to indicate whether somebody is PMC or
> committer.
>
> I'm against enforcing the order of approvals for now. I fear that this will
> make the tool too restrictive. I like Ufuk's idea of putting a note into
> the tracking comment for now.
> Once it's active and we are using it day to day, we'll probably learn what
> features we need the most.
>
>
> @Ufuk: I think we should put it into a Apache repo at some point. But I'm
> not sure if it's worth going through the effort of setting up a new repo
> now, given that the bot is not even active yet, and we are not sure if it's
> going to be useful.
> Once it is active for a month or two, I will move it.
>
> Regarding the bots in general: I don't see a problem with having multiple
> bots in place, as long as they get along with each other well.
> We should try not to reinvent the wheel, if there's already a good bot
> implementation, I don't see a reason to not use it.
> The problem in our case is that we have limited access to our GitHub page,
> and merging can not happen through the GitHub user interface -- so I guess
> many "off the shelf" bots will not work in our environment.
> I'm thinking already about approaches how to automatically merge pull
> requests with the bot. But this will be a separate mailing list thread :)
>
> Thanks for the feedback!
>
>
>
>
> On Mon, Jan 28, 2019 at 5:20 PM Ufuk Celebi <[hidden email]> wrote:
>
>> Thanks for the clarification. I agree that it only makes sense to
>> check the points in order. +1 to add this if we can think of a nice
>> way to do it. I'm not sure how we would enforce the order with the bot
>> since there is only indirect feedback to a bot command. The only thing
>> I can think of at the moment is to add a note to a check in case
>> earlier steps where not executed. Just ignoring a bot command because
>> other commands have not been executed before feels not helpful to me
>> (since we can't prevent reviewers to jump to later steps if they wish
>> to do so).
>>
>> I'd rather add a bold note to the bot template that makes clear that
>> all points should be followed in order to avoid potentially redundant
>> work.
>>
>> – Ufuk
>>
>> On Mon, Jan 28, 2019 at 5:01 PM Fabian Hueske <[hidden email]> wrote:
>>>
>>> The points in the review template are in the order in which they should
>> be
>>> checked, i.e., first checking the description, then consensus and finally
>>> checking the code.
>>> Currently, it is possible to tick off the code box before checking the
>>> description.
>>> One motivation for the process was to do the steps in exactly the
>> proposed
>>> order for example to to avoid detailed code reviews before there was
>>> consensus whether the contribution was welcome or not.
>>>
>>> Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <[hidden email]>:
>>>
>>>> I played around with the bot and it works pretty well. :-) @Robert:
>>>> Are there any plans to contribute the code for the bot to Apache
>>>> (potentially in another repository)?
>>>>
>>>> I like Fabians suggestions. Regarding the questions:
>>>> 1) I would make that dependent on whether you expected the review
>>>> guideline to only apply to committers or also regular contributors.
>>>> Since the bot is not merging PRs, I don't see a down side in having it
>>>> open for all contributors (except potential noise).
>>>> 2) What do you mean with "order of approvals"?
>>>>
>>>> Since there is another lively discussion going on about a bot for
>>>> stale PRs, I'm wondering what the future plans for @flinkbot are. Do
>>>> we want to have multiple bots for the project? Or do you plan to
>>>> support staleness checks in the future? What about merging of PRs?
>>>>
>>>> – Ufuk
>>>>
>>>> On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]>
>> wrote:
>>>>>
>>>>> Hi Robert,
>>>>>
>>>>> Thanks for working on the bot!
>>>>> I have a few suggestions / questions:
>>>>>
>>>>> Suggestions:
>>>>> 1) It would be great to approve multiple boxes in one comment.
>> Either as
>>>>>> @flinkbot approve contribution consensus
>>>>> or by
>>>>>> @flinkbot approve contribution
>>>>>> @flinkbot approve consensus
>>>>>
>>>>> 2) Extend the last line of the description by adding something like
>> "See
>>>>> Pull Request Review Guide for how to use the Flink bot."?
>>>>> 3) Rephrase the first line to include "[description]" instead of
>>>>> "[contribution]", as it is about approving the description
>>>>>
>>>>> Questions:
>>>>> 1) Can anybody use the bot or just committers?
>>>>> 2) Does it make sense to enforce the order in which approvals are
>> given?
>>>>>
>>>>> Best,
>>>>> Fabian
>>>>>
>>>>> Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
>>>>> [hidden email]>:
>>>>>
>>>>>> I have the bot now running in
>>>> https://github.com/flinkbot/test-repo/pulls
>>>>>> Feel free to play with it.
>>>>>>
>>>>>> On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <
>> [hidden email]>
>>>>>> wrote:
>>>>>>
>>>>>>> Okay, cool! I'll let you know when the bot is ready in a test
>> repo.
>>>>>>> While you (and others) are testing it, I'll open a PR for the
>> docs.
>>>>>>>
>>>>>>> On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <
>> [hidden email]>
>>>>>> wrote:
>>>>>>>
>>>>>>>> Oh, that's great news!
>>>>>>>> In that case we can just close the PR and start with the bot
>> right
>>>> away.
>>>>>>>> I think it would be good to extend the PR Review guide [1] with
>> a
>>>>>> section
>>>>>>>> about the bot and how to use it.
>>>>>>>>
>>>>>>>> Fabian
>>>>>>>>
>>>>>>>> [1] https://flink.apache.org/reviewing-prs.html
>>>>>>>>
>>>>>>>> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
>>>>>>>> [hidden email]>:
>>>>>>>>
>>>>>>>>> Hey,
>>>>>>>>>
>>>>>>>>> as I've mentioned already in the pull request, I have started
>>>>>>>> implementing
>>>>>>>>> a little bot for GitHub that tracks the checklist [1]
>>>>>>>>> The bot is monitoring incoming pull requests. It creates a
>> comment
>>>>>> with
>>>>>>>> the
>>>>>>>>> checklist.
>>>>>>>>> Reviewers can write a message to the bot (such as "@flinkbot
>>>> approve
>>>>>>>>> contribution"), then the bot will update the checklist
>> comment.
>>>>>>>>>
>>>>>>>>> As an upcoming feature, I also plan to add a label to the pull
>>>> request
>>>>>>>> when
>>>>>>>>> all the checklist conditions have been met.
>>>>>>>>>
>>>>>>>>> I hope to finish the bot today. After some initial testing,
>> we can
>>>>>>>> deploy
>>>>>>>>> it with Flink (if there are no objections by the community).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [1] https://github.com/rmetzger/flink-community-tools
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <
>> [hidden email]>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi everyone,
>>>>>>>>>>
>>>>>>>>>> A few months ago the community discussed and agreed to
>> improve
>>>> the
>>>>>> PR
>>>>>>>>>> review process [1-4].
>>>>>>>>>> The idea is to follow a checklist to avoid in-depth reviews
>> on
>>>>>>>>>> contributions that might not be accepted for other reasons.
>>>> Thereby,
>>>>>>>>>> reviewers and contributors do not spend their time on PRs
>> that
>>>> will
>>>>>>>> not
>>>>>>>>> be
>>>>>>>>>> merged.
>>>>>>>>>> The checklist consists of five points:
>>>>>>>>>>
>>>>>>>>>> 1. The contribution is well-described.
>>>>>>>>>> 2. There is consensus that the contribution should go into
>> to
>>>> Flink.
>>>>>>>>>> 3. [Does not need specific attention | Needs specific
>> attention
>>>> for
>>>>>> X
>>>>>>>> |
>>>>>>>>> Has
>>>>>>>>>> attention for X by Y]
>>>>>>>>>> 4. The architectural approach is sound.
>>>>>>>>>> 5. Overall code quality is good.
>>>>>>>>>>
>>>>>>>>>> Back then we added a review guide to the website [5] but did
>>>> not put
>>>>>>>> the
>>>>>>>>>> new process in place yet. I would like to start this now.
>>>>>>>>>> There is a PR [6] that adds the review checklist to the PR
>>>> template.
>>>>>>>>>> Committers who review add PR should follow the checklist and
>>>> tick
>>>>>> and
>>>>>>>>> sign
>>>>>>>>>> off the boxes by updating the PR description. For that
>>>> committers
>>>>>>>> need to
>>>>>>>>>> be members of the ASF Github organization.
>>>>>>>>>>
>>>>>>>>>> If nobody has concerns, I'll merge the PR in a few days.
>>>>>>>>>> Once the PR is merged, the reviews of all new PRs should
>> follow
>>>> the
>>>>>>>>>> checklist.
>>>>>>>>>>
>>>>>>>>>> Best,
>>>>>>>>>> Fabian
>>>>>>>>>>
>>>>>>>>>> [1]
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
>>>>>>>>>> [2]
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
>>>>>>>>>> [3]
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
>>>>>>>>>> [4]
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
>>>>>>>>>> [5] https://flink.apache.org/reviewing-prs.html
>>>>>>>>>> [6] https://github.com/apache/flink/pull/6873
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

[Chesnay Schepler-3]

I assume he means that bots cannot do it, as they'd require committer
permissions. Same with assigning reviewers and such.

On 29.01.2019 13:09, Aljoscha Krettek wrote:

> What do you mean by “merging cannot happen through the GitHub user interface”? You can in fact merge PRs by clicking on the merge button, or “rebase and merge”.
>
> Aljoscha
>
>> On 29. Jan 2019, at 11:58, Robert Metzger <[hidden email]> wrote:
>>
>> @Fabian: Thank you for your suggestions. Multiple approvals in one comment
>> are already possible.
>> I will see if I can easily add multiple approvals in one line as well.
>> I will also address 2) and 3).
>>
>> Regarding usage of the bot: Anyone can use it! It is up to the committer
>> who's merging in the end whether they are happy with the approver.
>> One of the feature ideas I have is to indicate whether somebody is PMC or
>> committer.
>>
>> I'm against enforcing the order of approvals for now. I fear that this will
>> make the tool too restrictive. I like Ufuk's idea of putting a note into
>> the tracking comment for now.
>> Once it's active and we are using it day to day, we'll probably learn what
>> features we need the most.
>>
>>
>> @Ufuk: I think we should put it into a Apache repo at some point. But I'm
>> not sure if it's worth going through the effort of setting up a new repo
>> now, given that the bot is not even active yet, and we are not sure if it's
>> going to be useful.
>> Once it is active for a month or two, I will move it.
>>
>> Regarding the bots in general: I don't see a problem with having multiple
>> bots in place, as long as they get along with each other well.
>> We should try not to reinvent the wheel, if there's already a good bot
>> implementation, I don't see a reason to not use it.
>> The problem in our case is that we have limited access to our GitHub page,
>> and merging can not happen through the GitHub user interface -- so I guess
>> many "off the shelf" bots will not work in our environment.
>> I'm thinking already about approaches how to automatically merge pull
>> requests with the bot. But this will be a separate mailing list thread :)
>>
>> Thanks for the feedback!
>>
>>
>>
>>
>> On Mon, Jan 28, 2019 at 5:20 PM Ufuk Celebi <[hidden email]> wrote:
>>
>>> Thanks for the clarification. I agree that it only makes sense to
>>> check the points in order. +1 to add this if we can think of a nice
>>> way to do it. I'm not sure how we would enforce the order with the bot
>>> since there is only indirect feedback to a bot command. The only thing
>>> I can think of at the moment is to add a note to a check in case
>>> earlier steps where not executed. Just ignoring a bot command because
>>> other commands have not been executed before feels not helpful to me
>>> (since we can't prevent reviewers to jump to later steps if they wish
>>> to do so).
>>>
>>> I'd rather add a bold note to the bot template that makes clear that
>>> all points should be followed in order to avoid potentially redundant
>>> work.
>>>
>>> – Ufuk
>>>
>>> On Mon, Jan 28, 2019 at 5:01 PM Fabian Hueske <[hidden email]> wrote:
>>>> The points in the review template are in the order in which they should
>>> be
>>>> checked, i.e., first checking the description, then consensus and finally
>>>> checking the code.
>>>> Currently, it is possible to tick off the code box before checking the
>>>> description.
>>>> One motivation for the process was to do the steps in exactly the
>>> proposed
>>>> order for example to to avoid detailed code reviews before there was
>>>> consensus whether the contribution was welcome or not.
>>>>
>>>> Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <[hidden email]>:
>>>>
>>>>> I played around with the bot and it works pretty well. :-) @Robert:
>>>>> Are there any plans to contribute the code for the bot to Apache
>>>>> (potentially in another repository)?
>>>>>
>>>>> I like Fabians suggestions. Regarding the questions:
>>>>> 1) I would make that dependent on whether you expected the review
>>>>> guideline to only apply to committers or also regular contributors.
>>>>> Since the bot is not merging PRs, I don't see a down side in having it
>>>>> open for all contributors (except potential noise).
>>>>> 2) What do you mean with "order of approvals"?
>>>>>
>>>>> Since there is another lively discussion going on about a bot for
>>>>> stale PRs, I'm wondering what the future plans for @flinkbot are. Do
>>>>> we want to have multiple bots for the project? Or do you plan to
>>>>> support staleness checks in the future? What about merging of PRs?
>>>>>
>>>>> – Ufuk
>>>>>
>>>>> On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]>
>>> wrote:
>>>>>> Hi Robert,
>>>>>>
>>>>>> Thanks for working on the bot!
>>>>>> I have a few suggestions / questions:
>>>>>>
>>>>>> Suggestions:
>>>>>> 1) It would be great to approve multiple boxes in one comment.
>>> Either as
>>>>>>> @flinkbot approve contribution consensus
>>>>>> or by
>>>>>>> @flinkbot approve contribution
>>>>>>> @flinkbot approve consensus
>>>>>> 2) Extend the last line of the description by adding something like
>>> "See
>>>>>> Pull Request Review Guide for how to use the Flink bot."?
>>>>>> 3) Rephrase the first line to include "[description]" instead of
>>>>>> "[contribution]", as it is about approving the description
>>>>>>
>>>>>> Questions:
>>>>>> 1) Can anybody use the bot or just committers?
>>>>>> 2) Does it make sense to enforce the order in which approvals are
>>> given?
>>>>>> Best,
>>>>>> Fabian
>>>>>>
>>>>>> Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
>>>>>> [hidden email]>:
>>>>>>
>>>>>>> I have the bot now running in
>>>>> https://github.com/flinkbot/test-repo/pulls
>>>>>>> Feel free to play with it.
>>>>>>>
>>>>>>> On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <
>>> [hidden email]>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Okay, cool! I'll let you know when the bot is ready in a test
>>> repo.
>>>>>>>> While you (and others) are testing it, I'll open a PR for the
>>> docs.
>>>>>>>> On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <
>>> [hidden email]>
>>>>>>> wrote:
>>>>>>>>> Oh, that's great news!
>>>>>>>>> In that case we can just close the PR and start with the bot
>>> right
>>>>> away.
>>>>>>>>> I think it would be good to extend the PR Review guide [1] with
>>> a
>>>>>>> section
>>>>>>>>> about the bot and how to use it.
>>>>>>>>>
>>>>>>>>> Fabian
>>>>>>>>>
>>>>>>>>> [1] https://flink.apache.org/reviewing-prs.html
>>>>>>>>>
>>>>>>>>> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
>>>>>>>>> [hidden email]>:
>>>>>>>>>
>>>>>>>>>> Hey,
>>>>>>>>>>
>>>>>>>>>> as I've mentioned already in the pull request, I have started
>>>>>>>>> implementing
>>>>>>>>>> a little bot for GitHub that tracks the checklist [1]
>>>>>>>>>> The bot is monitoring incoming pull requests. It creates a
>>> comment
>>>>>>> with
>>>>>>>>> the
>>>>>>>>>> checklist.
>>>>>>>>>> Reviewers can write a message to the bot (such as "@flinkbot
>>>>> approve
>>>>>>>>>> contribution"), then the bot will update the checklist
>>> comment.
>>>>>>>>>> As an upcoming feature, I also plan to add a label to the pull
>>>>> request
>>>>>>>>> when
>>>>>>>>>> all the checklist conditions have been met.
>>>>>>>>>>
>>>>>>>>>> I hope to finish the bot today. After some initial testing,
>>> we can
>>>>>>>>> deploy
>>>>>>>>>> it with Flink (if there are no objections by the community).
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> [1] https://github.com/rmetzger/flink-community-tools
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <
>>> [hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>> Hi everyone,
>>>>>>>>>>>
>>>>>>>>>>> A few months ago the community discussed and agreed to
>>> improve
>>>>> the
>>>>>>> PR
>>>>>>>>>>> review process [1-4].
>>>>>>>>>>> The idea is to follow a checklist to avoid in-depth reviews
>>> on
>>>>>>>>>>> contributions that might not be accepted for other reasons.
>>>>> Thereby,
>>>>>>>>>>> reviewers and contributors do not spend their time on PRs
>>> that
>>>>> will
>>>>>>>>> not
>>>>>>>>>> be
>>>>>>>>>>> merged.
>>>>>>>>>>> The checklist consists of five points:
>>>>>>>>>>>
>>>>>>>>>>> 1. The contribution is well-described.
>>>>>>>>>>> 2. There is consensus that the contribution should go into
>>> to
>>>>> Flink.
>>>>>>>>>>> 3. [Does not need specific attention | Needs specific
>>> attention
>>>>> for
>>>>>>> X
>>>>>>>>> |
>>>>>>>>>> Has
>>>>>>>>>>> attention for X by Y]
>>>>>>>>>>> 4. The architectural approach is sound.
>>>>>>>>>>> 5. Overall code quality is good.
>>>>>>>>>>>
>>>>>>>>>>> Back then we added a review guide to the website [5] but did
>>>>> not put
>>>>>>>>> the
>>>>>>>>>>> new process in place yet. I would like to start this now.
>>>>>>>>>>> There is a PR [6] that adds the review checklist to the PR
>>>>> template.
>>>>>>>>>>> Committers who review add PR should follow the checklist and
>>>>> tick
>>>>>>> and
>>>>>>>>>> sign
>>>>>>>>>>> off the boxes by updating the PR description. For that
>>>>> committers
>>>>>>>>> need to
>>>>>>>>>>> be members of the ASF Github organization.
>>>>>>>>>>>
>>>>>>>>>>> If nobody has concerns, I'll merge the PR in a few days.
>>>>>>>>>>> Once the PR is merged, the reviews of all new PRs should
>>> follow
>>>>> the
>>>>>>>>>>> checklist.
>>>>>>>>>>>
>>>>>>>>>>> Best,
>>>>>>>>>>> Fabian
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>>
>>>>>>>>>>>
>>> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
>>>>>>>>>>> [2]
>>>>>>>>>>>
>>>>>>>>>>>
>>> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
>>>>>>>>>>> [3]
>>>>>>>>>>>
>>>>>>>>>>>
>>> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
>>>>>>>>>>> [4]
>>>>>>>>>>>
>>>>>>>>>>>
>>> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
>>>>>>>>>>> [5] https://flink.apache.org/reviewing-prs.html
>>>>>>>>>>> [6] https://github.com/apache/flink/pull/6873
>>>>>>>>>>>
>

[Robert Metzger]

Sorry, I haven't been active in the business of merging pull requests
recently. But you are both totally right: There is now a shiny big button
for merging pull requests.

Regarding permissions for the bot: I'm going to ask Infra if the bot can
get permissions to label and merge pull requests.

I have addressed most of Fabian's feature requests. I will now update the
website PR with the review guide and then activate the bot!


On Tue, Jan 29, 2019 at 1:13 PM Chesnay Schepler <[hidden email]> wrote:

> I assume he means that bots cannot do it, as they'd require committer
> permissions. Same with assigning reviewers and such.
>
> On 29.01.2019 13:09, Aljoscha Krettek wrote:
> > What do you mean by “merging cannot happen through the GitHub user
> interface”? You can in fact merge PRs by clicking on the merge button, or
> “rebase and merge”.
> >
> > Aljoscha
> >
> >> On 29. Jan 2019, at 11:58, Robert Metzger <[hidden email]> wrote:
> >>
> >> @Fabian: Thank you for your suggestions. Multiple approvals in one
> comment
> >> are already possible.
> >> I will see if I can easily add multiple approvals in one line as well.
> >> I will also address 2) and 3).
> >>
> >> Regarding usage of the bot: Anyone can use it! It is up to the committer
> >> who's merging in the end whether they are happy with the approver.
> >> One of the feature ideas I have is to indicate whether somebody is PMC
> or
> >> committer.
> >>
> >> I'm against enforcing the order of approvals for now. I fear that this
> will
> >> make the tool too restrictive. I like Ufuk's idea of putting a note into
> >> the tracking comment for now.
> >> Once it's active and we are using it day to day, we'll probably learn
> what
> >> features we need the most.
> >>
> >>
> >> @Ufuk: I think we should put it into a Apache repo at some point. But
> I'm
> >> not sure if it's worth going through the effort of setting up a new repo
> >> now, given that the bot is not even active yet, and we are not sure if
> it's
> >> going to be useful.
> >> Once it is active for a month or two, I will move it.
> >>
> >> Regarding the bots in general: I don't see a problem with having
> multiple
> >> bots in place, as long as they get along with each other well.
> >> We should try not to reinvent the wheel, if there's already a good bot
> >> implementation, I don't see a reason to not use it.
> >> The problem in our case is that we have limited access to our GitHub
> page,
> >> and merging can not happen through the GitHub user interface -- so I
> guess
> >> many "off the shelf" bots will not work in our environment.
> >> I'm thinking already about approaches how to automatically merge pull
> >> requests with the bot. But this will be a separate mailing list thread
> :)
> >>
> >> Thanks for the feedback!
> >>
> >>
> >>
> >>
> >> On Mon, Jan 28, 2019 at 5:20 PM Ufuk Celebi <[hidden email]> wrote:
> >>
> >>> Thanks for the clarification. I agree that it only makes sense to
> >>> check the points in order. +1 to add this if we can think of a nice
> >>> way to do it. I'm not sure how we would enforce the order with the bot
> >>> since there is only indirect feedback to a bot command. The only thing
> >>> I can think of at the moment is to add a note to a check in case
> >>> earlier steps where not executed. Just ignoring a bot command because
> >>> other commands have not been executed before feels not helpful to me
> >>> (since we can't prevent reviewers to jump to later steps if they wish
> >>> to do so).
> >>>
> >>> I'd rather add a bold note to the bot template that makes clear that
> >>> all points should be followed in order to avoid potentially redundant
> >>> work.
> >>>
> >>> – Ufuk
> >>>
> >>> On Mon, Jan 28, 2019 at 5:01 PM Fabian Hueske <[hidden email]>
> wrote:
> >>>> The points in the review template are in the order in which they
> should
> >>> be
> >>>> checked, i.e., first checking the description, then consensus and
> finally
> >>>> checking the code.
> >>>> Currently, it is possible to tick off the code box before checking the
> >>>> description.
> >>>> One motivation for the process was to do the steps in exactly the
> >>> proposed
> >>>> order for example to to avoid detailed code reviews before there was
> >>>> consensus whether the contribution was welcome or not.
> >>>>
> >>>> Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <
> [hidden email]>:
> >>>>
> >>>>> I played around with the bot and it works pretty well. :-) @Robert:
> >>>>> Are there any plans to contribute the code for the bot to Apache
> >>>>> (potentially in another repository)?
> >>>>>
> >>>>> I like Fabians suggestions. Regarding the questions:
> >>>>> 1) I would make that dependent on whether you expected the review
> >>>>> guideline to only apply to committers or also regular contributors.
> >>>>> Since the bot is not merging PRs, I don't see a down side in having
> it
> >>>>> open for all contributors (except potential noise).
> >>>>> 2) What do you mean with "order of approvals"?
> >>>>>
> >>>>> Since there is another lively discussion going on about a bot for
> >>>>> stale PRs, I'm wondering what the future plans for @flinkbot are. Do
> >>>>> we want to have multiple bots for the project? Or do you plan to
> >>>>> support staleness checks in the future? What about merging of PRs?
> >>>>>
> >>>>> – Ufuk
> >>>>>
> >>>>> On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]>
> >>> wrote:
> >>>>>> Hi Robert,
> >>>>>>
> >>>>>> Thanks for working on the bot!
> >>>>>> I have a few suggestions / questions:
> >>>>>>
> >>>>>> Suggestions:
> >>>>>> 1) It would be great to approve multiple boxes in one comment.
> >>> Either as
> >>>>>>> @flinkbot approve contribution consensus
> >>>>>> or by
> >>>>>>> @flinkbot approve contribution
> >>>>>>> @flinkbot approve consensus
> >>>>>> 2) Extend the last line of the description by adding something like
> >>> "See
> >>>>>> Pull Request Review Guide for how to use the Flink bot."?
> >>>>>> 3) Rephrase the first line to include "[description]" instead of
> >>>>>> "[contribution]", as it is about approving the description
> >>>>>>
> >>>>>> Questions:
> >>>>>> 1) Can anybody use the bot or just committers?
> >>>>>> 2) Does it make sense to enforce the order in which approvals are
> >>> given?
> >>>>>> Best,
> >>>>>> Fabian
> >>>>>>
> >>>>>> Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
> >>>>>> [hidden email]>:
> >>>>>>
> >>>>>>> I have the bot now running in
> >>>>> https://github.com/flinkbot/test-repo/pulls
> >>>>>>> Feel free to play with it.
> >>>>>>>
> >>>>>>> On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <
> >>> [hidden email]>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Okay, cool! I'll let you know when the bot is ready in a test
> >>> repo.
> >>>>>>>> While you (and others) are testing it, I'll open a PR for the
> >>> docs.
> >>>>>>>> On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <
> >>> [hidden email]>
> >>>>>>> wrote:
> >>>>>>>>> Oh, that's great news!
> >>>>>>>>> In that case we can just close the PR and start with the bot
> >>> right
> >>>>> away.
> >>>>>>>>> I think it would be good to extend the PR Review guide [1] with
> >>> a
> >>>>>>> section
> >>>>>>>>> about the bot and how to use it.
> >>>>>>>>>
> >>>>>>>>> Fabian
> >>>>>>>>>
> >>>>>>>>> [1] https://flink.apache.org/reviewing-prs.html
> >>>>>>>>>
> >>>>>>>>> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> >>>>>>>>> [hidden email]>:
> >>>>>>>>>
> >>>>>>>>>> Hey,
> >>>>>>>>>>
> >>>>>>>>>> as I've mentioned already in the pull request, I have started
> >>>>>>>>> implementing
> >>>>>>>>>> a little bot for GitHub that tracks the checklist [1]
> >>>>>>>>>> The bot is monitoring incoming pull requests. It creates a
> >>> comment
> >>>>>>> with
> >>>>>>>>> the
> >>>>>>>>>> checklist.
> >>>>>>>>>> Reviewers can write a message to the bot (such as "@flinkbot
> >>>>> approve
> >>>>>>>>>> contribution"), then the bot will update the checklist
> >>> comment.
> >>>>>>>>>> As an upcoming feature, I also plan to add a label to the pull
> >>>>> request
> >>>>>>>>> when
> >>>>>>>>>> all the checklist conditions have been met.
> >>>>>>>>>>
> >>>>>>>>>> I hope to finish the bot today. After some initial testing,
> >>> we can
> >>>>>>>>> deploy
> >>>>>>>>>> it with Flink (if there are no objections by the community).
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> [1] https://github.com/rmetzger/flink-community-tools
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <
> >>> [hidden email]>
> >>>>>>>>> wrote:
> >>>>>>>>>>> Hi everyone,
> >>>>>>>>>>>
> >>>>>>>>>>> A few months ago the community discussed and agreed to
> >>> improve
> >>>>> the
> >>>>>>> PR
> >>>>>>>>>>> review process [1-4].
> >>>>>>>>>>> The idea is to follow a checklist to avoid in-depth reviews
> >>> on
> >>>>>>>>>>> contributions that might not be accepted for other reasons.
> >>>>> Thereby,
> >>>>>>>>>>> reviewers and contributors do not spend their time on PRs
> >>> that
> >>>>> will
> >>>>>>>>> not
> >>>>>>>>>> be
> >>>>>>>>>>> merged.
> >>>>>>>>>>> The checklist consists of five points:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. The contribution is well-described.
> >>>>>>>>>>> 2. There is consensus that the contribution should go into
> >>> to
> >>>>> Flink.
> >>>>>>>>>>> 3. [Does not need specific attention | Needs specific
> >>> attention
> >>>>> for
> >>>>>>> X
> >>>>>>>>> |
> >>>>>>>>>> Has
> >>>>>>>>>>> attention for X by Y]
> >>>>>>>>>>> 4. The architectural approach is sound.
> >>>>>>>>>>> 5. Overall code quality is good.
> >>>>>>>>>>>
> >>>>>>>>>>> Back then we added a review guide to the website [5] but did
> >>>>> not put
> >>>>>>>>> the
> >>>>>>>>>>> new process in place yet. I would like to start this now.
> >>>>>>>>>>> There is a PR [6] that adds the review checklist to the PR
> >>>>> template.
> >>>>>>>>>>> Committers who review add PR should follow the checklist and
> >>>>> tick
> >>>>>>> and
> >>>>>>>>>> sign
> >>>>>>>>>>> off the boxes by updating the PR description. For that
> >>>>> committers
> >>>>>>>>> need to
> >>>>>>>>>>> be members of the ASF Github organization.
> >>>>>>>>>>>
> >>>>>>>>>>> If nobody has concerns, I'll merge the PR in a few days.
> >>>>>>>>>>> Once the PR is merged, the reviews of all new PRs should
> >>> follow
> >>>>> the
> >>>>>>>>>>> checklist.
> >>>>>>>>>>>
> >>>>>>>>>>> Best,
> >>>>>>>>>>> Fabian
> >>>>>>>>>>>
> >>>>>>>>>>> [1]
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> >>>>>>>>>>> [2]
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> >>>>>>>>>>> [3]
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> >>>>>>>>>>> [4]
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> >>>>>>>>>>> [5] https://flink.apache.org/reviewing-prs.html
> >>>>>>>>>>> [6] https://github.com/apache/flink/pull/6873
> >>>>>>>>>>>
> >
>
>

[Fabian Hueske-2]

Great, thank you Robert!

Am Do., 31. Jan. 2019 um 16:44 Uhr schrieb Robert Metzger <
[hidden email]>:

> Sorry, I haven't been active in the business of merging pull requests
> recently. But you are both totally right: There is now a shiny big button
> for merging pull requests.
>
> Regarding permissions for the bot: I'm going to ask Infra if the bot can
> get permissions to label and merge pull requests.
>
> I have addressed most of Fabian's feature requests. I will now update the
> website PR with the review guide and then activate the bot!
>
>
> On Tue, Jan 29, 2019 at 1:13 PM Chesnay Schepler <[hidden email]>
> wrote:
>
> > I assume he means that bots cannot do it, as they'd require committer
> > permissions. Same with assigning reviewers and such.
> >
> > On 29.01.2019 13:09, Aljoscha Krettek wrote:
> > > What do you mean by “merging cannot happen through the GitHub user
> > interface”? You can in fact merge PRs by clicking on the merge button, or
> > “rebase and merge”.
> > >
> > > Aljoscha
> > >
> > >> On 29. Jan 2019, at 11:58, Robert Metzger <[hidden email]>
> wrote:
> > >>
> > >> @Fabian: Thank you for your suggestions. Multiple approvals in one
> > comment
> > >> are already possible.
> > >> I will see if I can easily add multiple approvals in one line as well.
> > >> I will also address 2) and 3).
> > >>
> > >> Regarding usage of the bot: Anyone can use it! It is up to the
> committer
> > >> who's merging in the end whether they are happy with the approver.
> > >> One of the feature ideas I have is to indicate whether somebody is PMC
> > or
> > >> committer.
> > >>
> > >> I'm against enforcing the order of approvals for now. I fear that this
> > will
> > >> make the tool too restrictive. I like Ufuk's idea of putting a note
> into
> > >> the tracking comment for now.
> > >> Once it's active and we are using it day to day, we'll probably learn
> > what
> > >> features we need the most.
> > >>
> > >>
> > >> @Ufuk: I think we should put it into a Apache repo at some point. But
> > I'm
> > >> not sure if it's worth going through the effort of setting up a new
> repo
> > >> now, given that the bot is not even active yet, and we are not sure if
> > it's
> > >> going to be useful.
> > >> Once it is active for a month or two, I will move it.
> > >>
> > >> Regarding the bots in general: I don't see a problem with having
> > multiple
> > >> bots in place, as long as they get along with each other well.
> > >> We should try not to reinvent the wheel, if there's already a good bot
> > >> implementation, I don't see a reason to not use it.
> > >> The problem in our case is that we have limited access to our GitHub
> > page,
> > >> and merging can not happen through the GitHub user interface -- so I
> > guess
> > >> many "off the shelf" bots will not work in our environment.
> > >> I'm thinking already about approaches how to automatically merge pull
> > >> requests with the bot. But this will be a separate mailing list thread
> > :)
> > >>
> > >> Thanks for the feedback!
> > >>
> > >>
> > >>
> > >>
> > >> On Mon, Jan 28, 2019 at 5:20 PM Ufuk Celebi <[hidden email]> wrote:
> > >>
> > >>> Thanks for the clarification. I agree that it only makes sense to
> > >>> check the points in order. +1 to add this if we can think of a nice
> > >>> way to do it. I'm not sure how we would enforce the order with the
> bot
> > >>> since there is only indirect feedback to a bot command. The only
> thing
> > >>> I can think of at the moment is to add a note to a check in case
> > >>> earlier steps where not executed. Just ignoring a bot command because
> > >>> other commands have not been executed before feels not helpful to me
> > >>> (since we can't prevent reviewers to jump to later steps if they wish
> > >>> to do so).
> > >>>
> > >>> I'd rather add a bold note to the bot template that makes clear that
> > >>> all points should be followed in order to avoid potentially redundant
> > >>> work.
> > >>>
> > >>> – Ufuk
> > >>>
> > >>> On Mon, Jan 28, 2019 at 5:01 PM Fabian Hueske <[hidden email]>
> > wrote:
> > >>>> The points in the review template are in the order in which they
> > should
> > >>> be
> > >>>> checked, i.e., first checking the description, then consensus and
> > finally
> > >>>> checking the code.
> > >>>> Currently, it is possible to tick off the code box before checking
> the
> > >>>> description.
> > >>>> One motivation for the process was to do the steps in exactly the
> > >>> proposed
> > >>>> order for example to to avoid detailed code reviews before there was
> > >>>> consensus whether the contribution was welcome or not.
> > >>>>
> > >>>> Am Mo., 28. Jan. 2019 um 16:54 Uhr schrieb Ufuk Celebi <
> > [hidden email]>:
> > >>>>
> > >>>>> I played around with the bot and it works pretty well. :-) @Robert:
> > >>>>> Are there any plans to contribute the code for the bot to Apache
> > >>>>> (potentially in another repository)?
> > >>>>>
> > >>>>> I like Fabians suggestions. Regarding the questions:
> > >>>>> 1) I would make that dependent on whether you expected the review
> > >>>>> guideline to only apply to committers or also regular contributors.
> > >>>>> Since the bot is not merging PRs, I don't see a down side in having
> > it
> > >>>>> open for all contributors (except potential noise).
> > >>>>> 2) What do you mean with "order of approvals"?
> > >>>>>
> > >>>>> Since there is another lively discussion going on about a bot for
> > >>>>> stale PRs, I'm wondering what the future plans for @flinkbot are.
> Do
> > >>>>> we want to have multiple bots for the project? Or do you plan to
> > >>>>> support staleness checks in the future? What about merging of PRs?
> > >>>>>
> > >>>>> – Ufuk
> > >>>>>
> > >>>>> On Mon, Jan 28, 2019 at 4:23 PM Fabian Hueske <[hidden email]>
> > >>> wrote:
> > >>>>>> Hi Robert,
> > >>>>>>
> > >>>>>> Thanks for working on the bot!
> > >>>>>> I have a few suggestions / questions:
> > >>>>>>
> > >>>>>> Suggestions:
> > >>>>>> 1) It would be great to approve multiple boxes in one comment.
> > >>> Either as
> > >>>>>>> @flinkbot approve contribution consensus
> > >>>>>> or by
> > >>>>>>> @flinkbot approve contribution
> > >>>>>>> @flinkbot approve consensus
> > >>>>>> 2) Extend the last line of the description by adding something
> like
> > >>> "See
> > >>>>>> Pull Request Review Guide for how to use the Flink bot."?
> > >>>>>> 3) Rephrase the first line to include "[description]" instead of
> > >>>>>> "[contribution]", as it is about approving the description
> > >>>>>>
> > >>>>>> Questions:
> > >>>>>> 1) Can anybody use the bot or just committers?
> > >>>>>> 2) Does it make sense to enforce the order in which approvals are
> > >>> given?
> > >>>>>> Best,
> > >>>>>> Fabian
> > >>>>>>
> > >>>>>> Am Mi., 23. Jan. 2019 um 13:51 Uhr schrieb Robert Metzger <
> > >>>>>> [hidden email]>:
> > >>>>>>
> > >>>>>>> I have the bot now running in
> > >>>>> https://github.com/flinkbot/test-repo/pulls
> > >>>>>>> Feel free to play with it.
> > >>>>>>>
> > >>>>>>> On Wed, Jan 23, 2019 at 10:25 AM Robert Metzger <
> > >>> [hidden email]>
> > >>>>>>> wrote:
> > >>>>>>>
> > >>>>>>>> Okay, cool! I'll let you know when the bot is ready in a test
> > >>> repo.
> > >>>>>>>> While you (and others) are testing it, I'll open a PR for the
> > >>> docs.
> > >>>>>>>> On Wed, Jan 23, 2019 at 10:15 AM Fabian Hueske <
> > >>> [hidden email]>
> > >>>>>>> wrote:
> > >>>>>>>>> Oh, that's great news!
> > >>>>>>>>> In that case we can just close the PR and start with the bot
> > >>> right
> > >>>>> away.
> > >>>>>>>>> I think it would be good to extend the PR Review guide [1] with
> > >>> a
> > >>>>>>> section
> > >>>>>>>>> about the bot and how to use it.
> > >>>>>>>>>
> > >>>>>>>>> Fabian
> > >>>>>>>>>
> > >>>>>>>>> [1] https://flink.apache.org/reviewing-prs.html
> > >>>>>>>>>
> > >>>>>>>>> Am Mi., 23. Jan. 2019 um 10:03 Uhr schrieb Robert Metzger <
> > >>>>>>>>> [hidden email]>:
> > >>>>>>>>>
> > >>>>>>>>>> Hey,
> > >>>>>>>>>>
> > >>>>>>>>>> as I've mentioned already in the pull request, I have started
> > >>>>>>>>> implementing
> > >>>>>>>>>> a little bot for GitHub that tracks the checklist [1]
> > >>>>>>>>>> The bot is monitoring incoming pull requests. It creates a
> > >>> comment
> > >>>>>>> with
> > >>>>>>>>> the
> > >>>>>>>>>> checklist.
> > >>>>>>>>>> Reviewers can write a message to the bot (such as "@flinkbot
> > >>>>> approve
> > >>>>>>>>>> contribution"), then the bot will update the checklist
> > >>> comment.
> > >>>>>>>>>> As an upcoming feature, I also plan to add a label to the pull
> > >>>>> request
> > >>>>>>>>> when
> > >>>>>>>>>> all the checklist conditions have been met.
> > >>>>>>>>>>
> > >>>>>>>>>> I hope to finish the bot today. After some initial testing,
> > >>> we can
> > >>>>>>>>> deploy
> > >>>>>>>>>> it with Flink (if there are no objections by the community).
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> [1] https://github.com/rmetzger/flink-community-tools
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> On Tue, Jan 22, 2019 at 3:48 PM Fabian Hueske <
> > >>> [hidden email]>
> > >>>>>>>>> wrote:
> > >>>>>>>>>>> Hi everyone,
> > >>>>>>>>>>>
> > >>>>>>>>>>> A few months ago the community discussed and agreed to
> > >>> improve
> > >>>>> the
> > >>>>>>> PR
> > >>>>>>>>>>> review process [1-4].
> > >>>>>>>>>>> The idea is to follow a checklist to avoid in-depth reviews
> > >>> on
> > >>>>>>>>>>> contributions that might not be accepted for other reasons.
> > >>>>> Thereby,
> > >>>>>>>>>>> reviewers and contributors do not spend their time on PRs
> > >>> that
> > >>>>> will
> > >>>>>>>>> not
> > >>>>>>>>>> be
> > >>>>>>>>>>> merged.
> > >>>>>>>>>>> The checklist consists of five points:
> > >>>>>>>>>>>
> > >>>>>>>>>>> 1. The contribution is well-described.
> > >>>>>>>>>>> 2. There is consensus that the contribution should go into
> > >>> to
> > >>>>> Flink.
> > >>>>>>>>>>> 3. [Does not need specific attention | Needs specific
> > >>> attention
> > >>>>> for
> > >>>>>>> X
> > >>>>>>>>> |
> > >>>>>>>>>> Has
> > >>>>>>>>>>> attention for X by Y]
> > >>>>>>>>>>> 4. The architectural approach is sound.
> > >>>>>>>>>>> 5. Overall code quality is good.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Back then we added a review guide to the website [5] but did
> > >>>>> not put
> > >>>>>>>>> the
> > >>>>>>>>>>> new process in place yet. I would like to start this now.
> > >>>>>>>>>>> There is a PR [6] that adds the review checklist to the PR
> > >>>>> template.
> > >>>>>>>>>>> Committers who review add PR should follow the checklist and
> > >>>>> tick
> > >>>>>>> and
> > >>>>>>>>>> sign
> > >>>>>>>>>>> off the boxes by updating the PR description. For that
> > >>>>> committers
> > >>>>>>>>> need to
> > >>>>>>>>>>> be members of the ASF Github organization.
> > >>>>>>>>>>>
> > >>>>>>>>>>> If nobody has concerns, I'll merge the PR in a few days.
> > >>>>>>>>>>> Once the PR is merged, the reviews of all new PRs should
> > >>> follow
> > >>>>> the
> > >>>>>>>>>>> checklist.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Best,
> > >>>>>>>>>>> Fabian
> > >>>>>>>>>>>
> > >>>>>>>>>>> [1]
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>
> >
> https://lists.apache.org/thread.html/dcbe377eb477b531f49c462e90d8b1e50e0ff33c6efd296081c6934d@%3Cdev.flink.apache.org%3E
> > >>>>>>>>>>> [2]
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>
> >
> https://lists.apache.org/thread.html/172aa6d12ed442ea4da9ed2a72fe0894c9be7408fb2e1b7b50dfcb8c@%3Cdev.flink.apache.org%3E
> > >>>>>>>>>>> [3]
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>
> >
> https://lists.apache.org/thread.html/5e07c1be8078dd7b89d93c67b71defacff137f3df56ccf4adb04b4d7@%3Cdev.flink.apache.org%3E
> > >>>>>>>>>>> [4]
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>
> >
> https://lists.apache.org/thread.html/d7fd1fe45949f7c706142c62de85d246c7f6a1485a186fd3e9dced01@%3Cdev.flink.apache.org%3E
> > >>>>>>>>>>> [5] https://flink.apache.org/reviewing-prs.html
> > >>>>>>>>>>> [6] https://github.com/apache/flink/pull/6873
> > >>>>>>>>>>>
> > >
> >
> >
>