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 <rmetz...@apache.org> 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 <u...@apache.org> 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 <fhue...@gmail.com> 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 <u...@apache.org>:
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 <fhue...@gmail.com>
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 <
rmetz...@apache.org>:
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 <
rmetz...@apache.org>
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 <
fhue...@gmail.com>
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 <
rmetz...@apache.org>:
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 <
fhue...@gmail.com>
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
