[DISCUSS] Improve the flinkbot

[Robert Metzger]

Hey all,

the flinkbot has been active for a week now, and I hope the initial hiccups
have been resolved :)

I wanted to start this as a permanent thread to discuss problems and
improvements with the bot.

*So please post here if you have questions, problems or ideas how to
improve it!*

[Robert Metzger]

The first improvement to Flink Bot I would like to introduce is the use of labels.

I’m proposing to apply one of the following labels depending on the review progress:

review=needsDescriptionApproval ❌

review=needsConsensusApproval ❌

review=needsArchitectureApproval ❌

review=needsQualityApproval ❌

review=approved ✅

This is how it looks in my test repository: 

(screenshot url: https://user-images.githubusercontent.com/89049/52701055-9e022600-2f79-11e9-919e-df4338bc0fa3.png ) 

What are the benefits of this?

Labels allow to filter pull requests, so we can get a view of all approved pull requests, to merge them (after a final review :) )

More senior members of the community can focus on approving consensus and architecture of pull requests, while newer members of the community can focus on “just” reviewing the code quality.

If nobody objects here, I will activate this new feature in the coming days.

On Wed, Feb 13, 2019 at 10:29 AM Robert Metzger <[hidden email]> wrote:

Hey all,

the flinkbot has been active for a week now, and I hope the initial hiccups have been resolved :)

I wanted to start this as a permanent thread to discuss problems and improvements with the bot.

So please post here if you have questions, problems or ideas how to improve it!

[Chesnay Schepler-3]

As of right now I'd like 2 things:

1) By default the bot shows a big red X next to every item; I'd prefer a
question mark here as this allows us to differentiate between rejected
and unaddressed points. It's also a bit nicer for contributors imo as it
does not have such a negative connotation.

2) Be able to approve multiple items without requiring multiple
mentions, i.e. @flinkbot approve X Y Z should approve X,Y,Z at once.

On 13.02.2019 10:29, Robert Metzger wrote:

> Hey all,
>
> the flinkbot has been active for a week now, and I hope the initial hiccups
> have been resolved :)
>
> I wanted to start this as a permanent thread to discuss problems and
> improvements with the bot.
>
> *So please post here if you have questions, problems or ideas how to
> improve it!*
>

[Chesnay Schepler-3]

"More senior members of the community can focus on approving consensus
and architecture of pull requests, while newer members of the community
can focus on “just” reviewing the code quality."

TBH I reallydon't see this happening, so I'm not too hot for this change.

How have you solved the permission issue for the bot?

If the permissions are set what I'd like to see is /attention/
automatically adding the respective person to the list of reviewers.

On 13.02.2019 10:30, Robert Metzger wrote:

>
> The first improvement to Flink Bot I would like to introduce is the
> use of labels.
>
> I’m proposing to apply one of the following labels depending on the
> review progress:
>
>
> review=needsDescriptionApproval ❌
>
> review=needsConsensusApproval ❌
>
> review=needsArchitectureApproval ❌
>
> review=needsQualityApproval ❌
>
> review=approved ✅
>
>
> This is how it looks in my test repository:
>
> Screenshot 2019-02-13 10.24.16.png
> (screenshot url:
> https://user-images.githubusercontent.com/89049/52701055-9e022600-2f79-11e9-919e-df4338bc0fa3.png )
>
>
>
> What are the benefits of this?
>
> Labels allow to filter pull requests, so we can get a view of all
> approved pull requests, to merge them (after a final review :) )
>
> More senior members of the community can focus on approving consensus
> and architecture of pull requests, while newer members of the
> community can focus on “just” reviewing the code quality.
>
>
> If nobody objects here, I will activate this new feature in the coming
> days.
>
>
>
> On Wed, Feb 13, 2019 at 10:29 AM Robert Metzger <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hey all,
>
>     the flinkbot has been active for a week now, and I hope the
>     initial hiccups have been resolved :)
>
>     I wanted to start this as a permanent thread to discuss problems
>     and improvements with the bot.
>
>     *So please post here if you have questions, problems or ideas how
>     to improve it!*
>

[Stephan Ewen]

+1 for question mark instead of X - definitely comes across nicer

Maybe we can we make these labels shorter / more compact?
Do ne need to go through all steps individually, or can one immediately
jump to "approved" with one command? Or jump to "code quality review"?

On Wed, Feb 13, 2019 at 10:41 AM Chesnay Schepler <[hidden email]>
wrote:

> "More senior members of the community can focus on approving consensus
> and architecture of pull requests, while newer members of the community
> can focus on “just” reviewing the code quality."
>
> TBH I reallydon't see this happening, so I'm not too hot for this change.
>
> How have you solved the permission issue for the bot?
>
> If the permissions are set what I'd like to see is /attention/
> automatically adding the respective person to the list of reviewers.
>
> On 13.02.2019 10:30, Robert Metzger wrote:
> >
> > The first improvement to Flink Bot I would like to introduce is the
> > use of labels.
> >
> > I’m proposing to apply one of the following labels depending on the
> > review progress:
> >
> >
> > review=needsDescriptionApproval ❌
> >
> > review=needsConsensusApproval ❌
> >
> > review=needsArchitectureApproval ❌
> >
> > review=needsQualityApproval ❌
> >
> > review=approved ✅
> >
> >
> > This is how it looks in my test repository:
> >
> > Screenshot 2019-02-13 10.24.16.png
> > (screenshot url:
> >
> https://user-images.githubusercontent.com/89049/52701055-9e022600-2f79-11e9-919e-df4338bc0fa3.png
> )
> >
> >
> >
> > What are the benefits of this?
> >
> > Labels allow to filter pull requests, so we can get a view of all
> > approved pull requests, to merge them (after a final review :) )
> >
> > More senior members of the community can focus on approving consensus
> > and architecture of pull requests, while newer members of the
> > community can focus on “just” reviewing the code quality.
> >
> >
> > If nobody objects here, I will activate this new feature in the coming
> > days.
> >
> >
> >
> > On Wed, Feb 13, 2019 at 10:29 AM Robert Metzger <[hidden email]
> > <mailto:[hidden email]>> wrote:
> >
> >     Hey all,
> >
> >     the flinkbot has been active for a week now, and I hope the
> >     initial hiccups have been resolved :)
> >
> >     I wanted to start this as a permanent thread to discuss problems
> >     and improvements with the bot.
> >
> >     *So please post here if you have questions, problems or ideas how
> >     to improve it!*
> >
>
>

[jincheng sun]

Hi Robert, Thanks for bring up the discussion! I think add the labels is
good idea!

About the state of labels, I suggest that the state initializes the red X
turns yellow question mark , and turns blue checkmark when approved.

This way the contributors can know if these tags have been processed. What
to you think?

Best,
Jincheng

Robert Metzger <[hidden email]> 于2019年2月13日周三 下午5:30写道：

> The first improvement to Flink Bot I would like to introduce is the use of
> labels.
>
> I’m proposing to apply one of the following labels depending on the review
> progress:
>
>
> review=needsDescriptionApproval ❌
>
> review=needsConsensusApproval ❌
>
> review=needsArchitectureApproval ❌
>
> review=needsQualityApproval ❌
>
> review=approved ✅
>
>
> This is how it looks in my test repository:
>
> [image: Screenshot 2019-02-13 10.24.16.png]
> (screenshot url:
> https://user-images.githubusercontent.com/89049/52701055-9e022600-2f79-11e9-919e-df4338bc0fa3.png
>  )
>
>
> What are the benefits of this?
>
> Labels allow to filter pull requests, so we can get a view of all approved
> pull requests, to merge them (after a final review :) )
>
> More senior members of the community can focus on approving consensus and
> architecture of pull requests, while newer members of the community can
> focus on “just” reviewing the code quality.
>
>
> If nobody objects here, I will activate this new feature in the coming
> days.
>
>
>
> On Wed, Feb 13, 2019 at 10:29 AM Robert Metzger <[hidden email]>
> wrote:
>
>> Hey all,
>>
>> the flinkbot has been active for a week now, and I hope the initial
>> hiccups have been resolved :)
>>
>> I wanted to start this as a permanent thread to discuss problems and
>> improvements with the bot.
>>
>> *So please post here if you have questions, problems or ideas how to
>> improve it!*
>>
>

[Till Rohrmann]

I'd like to specify that a PR does not need special attention. Atm you need
to specify a person for point 3.

Big +1 for having a command to approve everything until (and also
including) a specified state.

Cheers,
Till

On Wed, Feb 13, 2019 at 11:17 AM jincheng sun <[hidden email]>
wrote:

> Hi Robert, Thanks for bring up the discussion! I think add the labels is
> good idea!
>
> About the state of labels, I suggest that the state initializes the red X
> turns yellow question mark , and turns blue checkmark when approved.
>
> This way the contributors can know if these tags have been processed. What
> to you think?
>
> Best,
> Jincheng
>
> Robert Metzger <[hidden email]> 于2019年2月13日周三 下午5:30写道：
>
> > The first improvement to Flink Bot I would like to introduce is the use
> of
> > labels.
> >
> > I’m proposing to apply one of the following labels depending on the
> review
> > progress:
> >
> >
> > review=needsDescriptionApproval ❌
> >
> > review=needsConsensusApproval ❌
> >
> > review=needsArchitectureApproval ❌
> >
> > review=needsQualityApproval ❌
> >
> > review=approved ✅
> >
> >
> > This is how it looks in my test repository:
> >
> > [image: Screenshot 2019-02-13 10.24.16.png]
> > (screenshot url:
> >
> https://user-images.githubusercontent.com/89049/52701055-9e022600-2f79-11e9-919e-df4338bc0fa3.png
> >  )
> >
> >
> > What are the benefits of this?
> >
> > Labels allow to filter pull requests, so we can get a view of all
> approved
> > pull requests, to merge them (after a final review :) )
> >
> > More senior members of the community can focus on approving consensus and
> > architecture of pull requests, while newer members of the community can
> > focus on “just” reviewing the code quality.
> >
> >
> > If nobody objects here, I will activate this new feature in the coming
> > days.
> >
> >
> >
> > On Wed, Feb 13, 2019 at 10:29 AM Robert Metzger <[hidden email]>
> > wrote:
> >
> >> Hey all,
> >>
> >> the flinkbot has been active for a week now, and I hope the initial
> >> hiccups have been resolved :)
> >>
> >> I wanted to start this as a permanent thread to discuss problems and
> >> improvements with the bot.
> >>
> >> *So please post here if you have questions, problems or ideas how to
> >> improve it!*
> >>
> >
>

[Chesnay Schepler-3]

The bot could check that the PR title to starts with [FLINK-X] or [hotfix].

On 13.02.2019 10:29, Robert Metzger wrote:

> Hey all,
>
> the flinkbot has been active for a week now, and I hope the initial hiccups
> have been resolved :)
>
> I wanted to start this as a permanent thread to discuss problems and
> improvements with the bot.
>
> *So please post here if you have questions, problems or ideas how to
> improve it!*
>

[Robert Metzger]

Thank you all for the proposals!

I've implemented most of the suggestions already, I hope to deploy it soon
to the repo

For the long label names:
I agree with Stephan that they are pretty long at the moment.

*Alternative 1:*
review=☐☐☐☐
review=☐☐☐☑
review=☐☐☑☑
review=☐☑☑☑
review=✅

*Alternative 2:*

review=description

review=consensus

review=architecture

review=quality

review=approved ✅

We could also add a ( ❓) emoji to alternative 2, but I found it looks ugly.
I lean towards alternative 2.

@jincheng sun <[hidden email]> I could actually not find
appropriate emojis with the colors you've proposed. The only thing that has
a nice range of colors are hearts, but I think that's not a good fit in our
case :)


On Fri, Feb 15, 2019 at 12:59 PM Chesnay Schepler <[hidden email]>
wrote:

> The bot could check that the PR title to starts with [FLINK-X] or [hotfix].
>
> On 13.02.2019 10:29, Robert Metzger wrote:
> > Hey all,
> >
> > the flinkbot has been active for a week now, and I hope the initial
> hiccups
> > have been resolved :)
> >
> > I wanted to start this as a permanent thread to discuss problems and
> > improvements with the bot.
> >
> > *So please post here if you have questions, problems or ideas how to
> > improve it!*
> >
>
>

[Chesnay Schepler-3]

I prefer alternative 2 as the first is rather ambiguous. The emojis seem
unnecessary, the approved label could be shorted to "Approved"; the
review prefix isn't necessary here imo.

I would stick the green checkmarks as this is consistent with Travis.

As another request, we may want to ignore flinkbot comments if they come
from the person opening the PR.
(Yes, there's already a precedence)

On 19.02.2019 15:49, Robert Metzger wrote:

> Thank you all for the proposals!
>
> I've implemented most of the suggestions already, I hope to deploy it soon
> to the repo
>
> For the long label names:
> I agree with Stephan that they are pretty long at the moment.
>
> *Alternative 1:*
> review=☐☐☐☐
> review=☐☐☐☑
> review=☐☐☑☑
> review=☐☑☑☑
> review=✅
>
> *Alternative 2:*
>
> review=description
>
> review=consensus
>
> review=architecture
>
> review=quality
>
> review=approved ✅
>
> We could also add a ( ❓) emoji to alternative 2, but I found it looks ugly.
> I lean towards alternative 2.
>
> @jincheng sun <[hidden email]> I could actually not find
> appropriate emojis with the colors you've proposed. The only thing that has
> a nice range of colors are hearts, but I think that's not a good fit in our
> case :)
>
>
> On Fri, Feb 15, 2019 at 12:59 PM Chesnay Schepler <[hidden email]>
> wrote:
>
>> The bot could check that the PR title to starts with [FLINK-X] or [hotfix].
>>
>> On 13.02.2019 10:29, Robert Metzger wrote:
>>> Hey all,
>>>
>>> the flinkbot has been active for a week now, and I hope the initial
>> hiccups
>>> have been resolved :)
>>>
>>> I wanted to start this as a permanent thread to discuss problems and
>>> improvements with the bot.
>>>
>>> *So please post here if you have questions, problems or ideas how to
>>> improve it!*
>>>
>>

[Jark Wu-2]

I also prefer alternative 2. Instead of using an emoji ( ❓) , can we just
use the "?" character ?  For example:

review=description?

IMO, this will be more clear that the description has not been approved
than "review=description".

Thanks,
Jark


On Wed, 20 Feb 2019 at 22:15, Chesnay Schepler <[hidden email]> wrote:

> I prefer alternative 2 as the first is rather ambiguous. The emojis seem
> unnecessary, the approved label could be shorted to "Approved"; the
> review prefix isn't necessary here imo.
>
> I would stick the green checkmarks as this is consistent with Travis.
>
> As another request, we may want to ignore flinkbot comments if they come
> from the person opening the PR.
> (Yes, there's already a precedence)
>
> On 19.02.2019 15:49, Robert Metzger wrote:
> > Thank you all for the proposals!
> >
> > I've implemented most of the suggestions already, I hope to deploy it
> soon
> > to the repo
> >
> > For the long label names:
> > I agree with Stephan that they are pretty long at the moment.
> >
> > *Alternative 1:*
> > review=☐☐☐☐
> > review=☐☐☐☑
> > review=☐☐☑☑
> > review=☐☑☑☑
> > review=✅
> >
> > *Alternative 2:*
> >
> > review=description
> >
> > review=consensus
> >
> > review=architecture
> >
> > review=quality
> >
> > review=approved ✅
> >
> > We could also add a ( ❓) emoji to alternative 2, but I found it looks
> ugly.
> > I lean towards alternative 2.
> >
> > @jincheng sun <[hidden email]> I could actually not find
> > appropriate emojis with the colors you've proposed. The only thing that
> has
> > a nice range of colors are hearts, but I think that's not a good fit in
> our
> > case :)
> >
> >
> > On Fri, Feb 15, 2019 at 12:59 PM Chesnay Schepler <[hidden email]>
> > wrote:
> >
> >> The bot could check that the PR title to starts with [FLINK-X] or
> [hotfix].
> >>
> >> On 13.02.2019 10:29, Robert Metzger wrote:
> >>> Hey all,
> >>>
> >>> the flinkbot has been active for a week now, and I hope the initial
> >> hiccups
> >>> have been resolved :)
> >>>
> >>> I wanted to start this as a permanent thread to discuss problems and
> >>> improvements with the bot.
> >>>
> >>> *So please post here if you have questions, problems or ideas how to
> >>> improve it!*
> >>>
> >>
>
>

[jincheng sun]

Hi @Robert Metzger <[hidden email]>  the colors of the emojis is not
very important. I think is enough if we can express the intentions clear.
+1 for alternative2, but for the corresponding pictures, if need I can look
for visual design classmates to help. what do you think?

Best,
Jincheng

Robert Metzger <[hidden email]> 于2019年2月19日周二 下午10:49写道：

> Thank you all for the proposals!
>
> I've implemented most of the suggestions already, I hope to deploy it soon
> to the repo
>
> For the long label names:
> I agree with Stephan that they are pretty long at the moment.
>
> *Alternative 1:*
> review=☐☐☐☐
> review=☐☐☐☑
> review=☐☐☑☑
> review=☐☑☑☑
> review=✅
>
> *Alternative 2:*
>
> review=description
>
> review=consensus
>
> review=architecture
>
> review=quality
>
> review=approved ✅
>
> We could also add a ( ❓) emoji to alternative 2, but I found it looks
> ugly.
> I lean towards alternative 2.
>
> @jincheng sun <[hidden email]> I could actually not find
> appropriate emojis with the colors you've proposed. The only thing that has
> a nice range of colors are hearts, but I think that's not a good fit in our
> case :)
>
>
> On Fri, Feb 15, 2019 at 12:59 PM Chesnay Schepler <[hidden email]>
> wrote:
>
>> The bot could check that the PR title to starts with [FLINK-X] or
>> [hotfix].
>>
>> On 13.02.2019 10:29, Robert Metzger wrote:
>> > Hey all,
>> >
>> > the flinkbot has been active for a week now, and I hope the initial
>> hiccups
>> > have been resolved :)
>> >
>> > I wanted to start this as a permanent thread to discuss problems and
>> > improvements with the bot.
>> >
>> > *So please post here if you have questions, problems or ideas how to
>> > improve it!*
>> >
>>
>>

[dwysakowicz]

Probably I am bit late to the party, but I just started using the Flinkbot.

A big +1 for having 3 states for each step. (Pending, Approved,
Rejected). Right now it is impossible to say

that I checked e.g. consensus and decided that the feature requires
further discussion.

Best,

Dawid


On 21/02/2019 08:25, jincheng sun wrote:

> Hi @Robert Metzger <[hidden email]>  the colors of the emojis is not
> very important. I think is enough if we can express the intentions clear.
> +1 for alternative2, but for the corresponding pictures, if need I can look
> for visual design classmates to help. what do you think?
>
> Best,
> Jincheng
>
> Robert Metzger <[hidden email]> 于2019年2月19日周二 下午10:49写道：
>
>> Thank you all for the proposals!
>>
>> I've implemented most of the suggestions already, I hope to deploy it soon
>> to the repo
>>
>> For the long label names:
>> I agree with Stephan that they are pretty long at the moment.
>>
>> *Alternative 1:*
>> review=☐☐☐☐
>> review=☐☐☐☑
>> review=☐☐☑☑
>> review=☐☑☑☑
>> review=✅
>>
>> *Alternative 2:*
>>
>> review=description
>>
>> review=consensus
>>
>> review=architecture
>>
>> review=quality
>>
>> review=approved ✅
>>
>> We could also add a ( ❓) emoji to alternative 2, but I found it looks
>> ugly.
>> I lean towards alternative 2.
>>
>> @jincheng sun <[hidden email]> I could actually not find
>> appropriate emojis with the colors you've proposed. The only thing that has
>> a nice range of colors are hearts, but I think that's not a good fit in our
>> case :)
>>
>>
>> On Fri, Feb 15, 2019 at 12:59 PM Chesnay Schepler <[hidden email]>
>> wrote:
>>
>>> The bot could check that the PR title to starts with [FLINK-X] or
>>> [hotfix].
>>>
>>> On 13.02.2019 10:29, Robert Metzger wrote:
>>>> Hey all,
>>>>
>>>> the flinkbot has been active for a week now, and I hope the initial
>>> hiccups
>>>> have been resolved :)
>>>>
>>>> I wanted to start this as a permanent thread to discuss problems and
>>>> improvements with the bot.
>>>>
>>>> *So please post here if you have questions, problems or ideas how to
>>>> improve it!*
>>>>
>>>

[Chesnay Schepler-3]

The bot could check the diff and tag pull requests that only touch the
docs as "Documentation". Many of these are easy to review and usually
don't require a deeper understanding of Flink.

On 13.02.2019 10:29, Robert Metzger wrote:

> Hey all,
>
> the flinkbot has been active for a week now, and I hope the initial hiccups
> have been resolved :)
>
> I wanted to start this as a permanent thread to discuss problems and
> improvements with the bot.
>
> *So please post here if you have questions, problems or ideas how to
> improve it!*
>

[Robert Metzger]

I will try to deploy the first version using labels today.

Here are my responses to your comments:

The emojis seem unnecessary, the approved label could be shorted to
> "Approved"; the
> review prefix isn't necessary here imo.


My idea was that each system (review bot, component labeler, size
estimator) has its own prefix, that is managed only by the system.
The Guava project is doing this as well:
https://github.com/google/guava/pulls
also Kubernetes to some extend:
https://github.com/kubernetes/kubernetes/pulls

I'm still in favor of keeping "review=approved".

As another request, we may want to ignore flinkbot comments if they come
> from the person opening the PR.


This is on the TODO list. I will address it with the next big development
iteration. For now, it is up to the merger to make sure that the approvals
were given by the right persons.

but for the corresponding pictures, if need I can look for visual design
> classmates to help. what do you think?


Afaik, we can not use custom emoji's on GitHub. Also, it seems that the use
of emojis is not very popular with the others here :)


Probably I am bit late to the party, but I just started using the Flinkbot.
> A big +1 for having 3 states for each step. (Pending, Approved, Rejected).
> Right now it is impossible to say that I checked e.g. consensus and decided
> that the feature requires further discussion.


Welcome to the party :)
Did somebody else suggest already 3 states for each step?
Since this is a bigger implementation effort, and requires additional
thinking about the semantics (what happens if we have conflicting approvals
etc.) I would suggest to add this to the TODO list, and have a separate
discussion with a full proposal?
As part of this, I also want to revisit the "attention" action.

The bot could check the diff and tag pull requests that only touch the
> docs as "Documentation". Many of these are easy to review and usually
> don't require a deeper understanding of Flink.


Yes, I think we should automatically label pull requests such as: hotfixes,
documentation only, new contributors.
It's on my list.


For the labels, I will now go with the following:

review=description?

review=consensus?

review=architecture?

review=quality?

review=approved ✅



On Fri, Feb 22, 2019 at 2:00 PM Chesnay Schepler <[hidden email]> wrote:

> The bot could check the diff and tag pull requests that only touch the
> docs as "Documentation". Many of these are easy to review and usually
> don't require a deeper understanding of Flink.
>
> On 13.02.2019 10:29, Robert Metzger wrote:
> > Hey all,
> >
> > the flinkbot has been active for a week now, and I hope the initial
> hiccups
> > have been resolved :)
> >
> > I wanted to start this as a permanent thread to discuss problems and
> > improvements with the bot.
> >
> > *So please post here if you have questions, problems or ideas how to
> > improve it!*
> >
>
>

[dwysakowicz]

Wasn't Chesnay's comment suggesting 3 states as well?:

> 1) By default the bot shows a big red X next to every item; I'd prefer a
> question mark here as this allows us to differentiate between rejected
> and unaddressed points. It's also a bit nicer for contributors imo as it
> does not have such a negative connotation.

On 22/02/2019 14:19, Robert Metzger wrote:

> I will try to deploy the first version using labels today.
>
> Here are my responses to your comments:
>
> The emojis seem unnecessary, the approved label could be shorted to
>> "Approved"; the
>> review prefix isn't necessary here imo.
>
> My idea was that each system (review bot, component labeler, size
> estimator) has its own prefix, that is managed only by the system.
> The Guava project is doing this as well:
> https://github.com/google/guava/pulls
> also Kubernetes to some extend:
> https://github.com/kubernetes/kubernetes/pulls
>
> I'm still in favor of keeping "review=approved".
>
> As another request, we may want to ignore flinkbot comments if they come
>> from the person opening the PR.
>
> This is on the TODO list. I will address it with the next big development
> iteration. For now, it is up to the merger to make sure that the approvals
> were given by the right persons.
>
> but for the corresponding pictures, if need I can look for visual design
>> classmates to help. what do you think?
>
> Afaik, we can not use custom emoji's on GitHub. Also, it seems that the use
> of emojis is not very popular with the others here :)
>
>
> Probably I am bit late to the party, but I just started using the Flinkbot.
>> A big +1 for having 3 states for each step. (Pending, Approved, Rejected).
>> Right now it is impossible to say that I checked e.g. consensus and decided
>> that the feature requires further discussion.
>
> Welcome to the party :)
> Did somebody else suggest already 3 states for each step?
> Since this is a bigger implementation effort, and requires additional
> thinking about the semantics (what happens if we have conflicting approvals
> etc.) I would suggest to add this to the TODO list, and have a separate
> discussion with a full proposal?
> As part of this, I also want to revisit the "attention" action.
>
> The bot could check the diff and tag pull requests that only touch the
>> docs as "Documentation". Many of these are easy to review and usually
>> don't require a deeper understanding of Flink.
>
> Yes, I think we should automatically label pull requests such as: hotfixes,
> documentation only, new contributors.
> It's on my list.
>
>
> For the labels, I will now go with the following:
>
> review=description?
>
> review=consensus?
>
> review=architecture?
>
> review=quality?
>
> review=approved ✅
>
>
>
> On Fri, Feb 22, 2019 at 2:00 PM Chesnay Schepler <[hidden email]> wrote:
>
>> The bot could check the diff and tag pull requests that only touch the
>> docs as "Documentation". Many of these are easy to review and usually
>> don't require a deeper understanding of Flink.
>>
>> On 13.02.2019 10:29, Robert Metzger wrote:
>>> Hey all,
>>>
>>> the flinkbot has been active for a week now, and I hope the initial
>> hiccups
>>> have been resolved :)
>>>
>>> I wanted to start this as a permanent thread to discuss problems and
>>> improvements with the bot.
>>>
>>> *So please post here if you have questions, problems or ideas how to
>>> improve it!*
>>>
>>

[Chesnay Schepler-3]

3 states for each step is effectively what I've been suggesting at the
very start. (initialize with question mark instead of red cross)

On 22.02.2019 14:19, Robert Metzger wrote:

> I will try to deploy the first version using labels today.
>
> Here are my responses to your comments:
>
>     The emojis seem unnecessary, the approved label could be shorted
>     to "Approved"; the
>     review prefix isn't necessary here imo.
>
>
> My idea was that each system (review bot, component labeler, size
> estimator) has its own prefix, that is managed only by the system.
> The Guava project is doing this as well:
> https://github.com/google/guava/pulls
> also Kubernetes to some extend:
> https://github.com/kubernetes/kubernetes/pulls
>
> I'm still in favor of keeping "review=approved".
>
>     As another request, we may want to ignore flinkbot comments if
>     they come
>     from the person opening the PR.
>
>
> This is on the TODO list. I will address it with the next big
> development iteration. For now, it is up to the merger to make sure
> that the approvals were given by the right persons.
>
>     but for the corresponding pictures, if need I can look for visual
>     design classmates to help. what do you think?
>
>
> Afaik, we can not use custom emoji's on GitHub. Also, it seems that
> the use of emojis is not very popular with the others here :)
>
>
>     Probably I am bit late to the party, but I just started using the
>     Flinkbot. A big +1 for having 3 states for each step. (Pending,
>     Approved, Rejected). Right now it is impossible to say that I
>     checked e.g. consensus and decided that the feature requires
>     further discussion.
>
>
> Welcome to the party :)
> Did somebody else suggest already 3 states for each step?
> Since this is a bigger implementation effort, and requires additional
> thinking about the semantics (what happens if we have conflicting
> approvals etc.) I would suggest to add this to the TODO list, and have
> a separate discussion with a full proposal?
> As part of this, I also want to revisit the "attention" action.
>
>     The bot could check the diff and tag pull requests that only touch
>     the
>     docs as "Documentation". Many of these are easy to review and usually
>     don't require a deeper understanding of Flink.
>
>
> Yes, I think we should automatically label pull requests such as:
> hotfixes, documentation only, new contributors.
> It's on my list.
>
> For the labels, I will now go with the following:
>
> review=description?
>
> review=consensus?
>
> review=architecture?
>
> review=quality?
>
> review=approved ✅
>
>
>
>
> On Fri, Feb 22, 2019 at 2:00 PM Chesnay Schepler <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     The bot could check the diff and tag pull requests that only touch
>     the
>     docs as "Documentation". Many of these are easy to review and usually
>     don't require a deeper understanding of Flink.
>
>     On 13.02.2019 10:29, Robert Metzger wrote:
>     > Hey all,
>     >
>     > the flinkbot has been active for a week now, and I hope the
>     initial hiccups
>     > have been resolved :)
>     >
>     > I wanted to start this as a permanent thread to discuss problems and
>     > improvements with the bot.
>     >
>     > *So please post here if you have questions, problems or ideas how to
>     > improve it!*
>     >
>

[Robert Metzger]

Ah, I didn't get that. I thought you only wanted to show ❓ instead of  ❌.
Maybe the "disapprove" command is also a bit misleading. What it actually
means is just "remove approval".


On Fri, Feb 22, 2019 at 2:29 PM Chesnay Schepler <[hidden email]> wrote:

> 3 states for each step is effectively what I've been suggesting at the
> very start. (initialize with question mark instead of red cross)
>
> On 22.02.2019 14:19, Robert Metzger wrote:
>
> I will try to deploy the first version using labels today.
>
> Here are my responses to your comments:
>
> The emojis seem unnecessary, the approved label could be shorted to
>> "Approved"; the
>> review prefix isn't necessary here imo.
>
>
> My idea was that each system (review bot, component labeler, size
> estimator) has its own prefix, that is managed only by the system.
> The Guava project is doing this as well:
> https://github.com/google/guava/pulls
> also Kubernetes to some extend:
> https://github.com/kubernetes/kubernetes/pulls
>
> I'm still in favor of keeping "review=approved".
>
> As another request, we may want to ignore flinkbot comments if they come
>> from the person opening the PR.
>
>
> This is on the TODO list. I will address it with the next big development
> iteration. For now, it is up to the merger to make sure that the approvals
> were given by the right persons.
>
> but for the corresponding pictures, if need I can look for visual design
>> classmates to help. what do you think?
>
>
> Afaik, we can not use custom emoji's on GitHub. Also, it seems that the
> use of emojis is not very popular with the others here :)
>
>
> Probably I am bit late to the party, but I just started using the
>> Flinkbot. A big +1 for having 3 states for each step. (Pending, Approved,
>> Rejected). Right now it is impossible to say that I checked e.g. consensus
>> and decided that the feature requires further discussion.
>
>
> Welcome to the party :)
> Did somebody else suggest already 3 states for each step?
> Since this is a bigger implementation effort, and requires additional
> thinking about the semantics (what happens if we have conflicting approvals
> etc.) I would suggest to add this to the TODO list, and have a separate
> discussion with a full proposal?
> As part of this, I also want to revisit the "attention" action.
>
> The bot could check the diff and tag pull requests that only touch the
>> docs as "Documentation". Many of these are easy to review and usually
>> don't require a deeper understanding of Flink.
>
>
> Yes, I think we should automatically label pull requests such as:
> hotfixes, documentation only, new contributors.
> It's on my list.
>
>
> For the labels, I will now go with the following:
>
> review=description?
>
> review=consensus?
>
> review=architecture?
>
> review=quality?
>
> review=approved ✅
>
>
>
> On Fri, Feb 22, 2019 at 2:00 PM Chesnay Schepler <[hidden email]>
> wrote:
>
>> The bot could check the diff and tag pull requests that only touch the
>> docs as "Documentation". Many of these are easy to review and usually
>> don't require a deeper understanding of Flink.
>>
>> On 13.02.2019 10:29, Robert Metzger wrote:
>> > Hey all,
>> >
>> > the flinkbot has been active for a week now, and I hope the initial
>> hiccups
>> > have been resolved :)
>> >
>> > I wanted to start this as a permanent thread to discuss problems and
>> > improvements with the bot.
>> >
>> > *So please post here if you have questions, problems or ideas how to
>> > improve it!*
>> >
>>
>>
>

[Chesnay Schepler-3]

Just noticed that _anyone_ can approve a PR now, see
https://github.com/apache/flink/pull/7801.

Not sure about the solution, but as it stands it is rather trivial to
nuke the review process of the entire project.

On 13.02.2019 10:29, Robert Metzger wrote:

> Hey all,
>
> the flinkbot has been active for a week now, and I hope the initial hiccups
> have been resolved :)
>
> I wanted to start this as a permanent thread to discuss problems and
> improvements with the bot.
>
> *So please post here if you have questions, problems or ideas how to
> improve it!*
>

[Robert Metzger]

I did not put any restrictions on who can communicate with the bot!
But since there is currently no way of overriding somebody's approval for
something, this can easily lead to such a situation.

My thinking was that a committer still needs to manually check who approved
a pull request, and I wanted to be open for non-committers to participate
in the review process.
WIth the labels in place, this can easily send the wrong message.

What should we do?
A) we restrict sending commands to the bot to committers?
B) only approvals from committers matter for applying labels?
C) we allow committers to override approvals

I'm leaning towards B, as it encourages non-committers to participate.


On Wed, Feb 27, 2019 at 2:01 PM Chesnay Schepler <[hidden email]> wrote:

> Just noticed that _anyone_ can approve a PR now, see
> https://github.com/apache/flink/pull/7801.
>
> Not sure about the solution, but as it stands it is rather trivial to
> nuke the review process of the entire project.
>
> On 13.02.2019 10:29, Robert Metzger wrote:
> > Hey all,
> >
> > the flinkbot has been active for a week now, and I hope the initial
> hiccups
> > have been resolved :)
> >
> > I wanted to start this as a permanent thread to discuss problems and
> > improvements with the bot.
> >
> > *So please post here if you have questions, problems or ideas how to
> > improve it!*
> >
>
>