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