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