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 <ches...@apache.org> 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 <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 > >>>>>>>>>>> > > > >
