Great work! What puzzles me a little bit, is the shepherding of PR from committers. Why should it be a different committer?
1) As a committer, you don't even need to open a PR for small code changes at all (especially, if the committer is the most experience one regard a certain component/library). Just open a JIRA, fix it, and commit. 2) It is clear, that if a PR is complex and maybe touches different components, feedback from the most experiences committer on those components should be given on the PR to ensure code quality. But the role of a shepherd is a "meta" role, if a understand the guideline correctly, and not a technical one (-> everybody should discuss, comment, accept, mark to get merged, and merge PRs). So why do we need a different shepherd there? I think, committers can drive their own PRs by themselves. -Matthias On 10/23/2015 06:00 PM, Fabian Hueske wrote: > Hi everybody, > > I created a wiki page for our Pull Request Process. [1] > Please review, refine, and comment. > > I would suggest to start following the process once 0.10 is released. > What do you think? > > Cheers, > Fabian > > [1] > https://cwiki.apache.org/confluence/display/FLINK/Pull+Request+Management > > 2015-10-19 17:53 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: > >> Thanks for your feedback, Alex. >> >> Chesnay is right, we cannot modify the GH assignee field at the moment. If >> this changes at some point, I would support your proposal. >> >> Regarding the PR - JIRA rule, this has been discussed as part of the new >> contribution guidelines discussion [1]. >> I agree, it is not always easy to draw a line here. However, if in doubt I >> would go for the JIRA issue because it allows everybody to track the issue >> later, esp. if it solves a bug that other people might run into as well. >> In your concrete example, you could have referenced the original JIRA >> issue to remove Spargel from your PR. >> >> I also agree that the shepherd should not be the author of the PR. >> However, every committer can always commit to the master branch (unless >> somebody raised concerns of course). So committers should be free to commit >> their own PRs, IMO. >> >> Cheers, Fabian >> >> [1] >> http://mail-archives.apache.org/mod_mbox/flink-dev/201509.mbox/%3CCAAdrtT0RLgqJgJVrK1zH_e_7VW1k8VTv4ChnumTRh7fLGA_zmw%40mail.gmail.com%3E >> >> >> >> 2015-10-17 13:00 GMT+02:00 Alexander Alexandrov < >> alexander.s.alexand...@gmail.com>: >> >>> Also, regarding the "Each PR should be backed by a JIRA" rule - please >>> don't make this strict and consider the relative effort to opening a JIRA >>> as opposed to just opening a PR for small PRs that fix something obvious. >>> >>> For example, two days ago I opened two PRs while investigating a reported >>> JIRA bug (FLINK-2858 <https://issues.apache.org/jira/browse/FLINK-2858>): >>> >>> https://github.com/apache/flink/pull/1259 >>> https://github.com/apache/flink/pull/1260 >>> >>> 1259 removes obsolete references to the (now removed 'flink-spargel' code >>> from the POM), while 1260 updates a paragraph of the "How to Build" >>> documentation and fixes some broken href anchors. >>> >>> Just my two cents here, but fixes (not only hotfixes) that >>> >>> * resolve minor and obvious issues with the existing code or the >>> documentation, >>> * can be followed by everybody just by looking at the diff >>> >>> should be just cherry-picked (and if needed amended) by a committer >>> without >>> too much unnecessary discussion and excluded from the "shepherding >>> process". >>> >>> >>> 2015-10-17 12:32 GMT+02:00 Alexander Alexandrov < >>> alexander.s.alexand...@gmail.com>: >>> >>>> One suggestion from me: in GitHub you can make clear who the current >>>> sheppard is through the "Assignee" field in the PR (which can and IMHO >>>> should be different from the user who actually opened the request). >>>> >>>> Regards, >>>> A. >>>> >>>> 2015-10-16 15:58 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: >>>> >>>>> Hi folks, >>>>> >>>>> I think we can split of the discussion of a PR meeting. >>>>> >>>>> Are there any more comments on the proposal itself? >>>>> Otherwise, I would go ahead and put the described process (modulo the >>>>> comments) into a document for our website. >>>>> >>>>> Cheers, Fabian >>>>> >>>>> 2015-10-07 16:57 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: >>>>> >>>>>> I like the idea of meeting once a week to discuss about PRs as well. >>>>>> >>>>>> Regarding lingering PRs, you can simply sort the Flink PRs in Github >>> by >>>>>> "least recently updated" >>>>>> >>>>>> Cheers, >>>>>> Fabian >>>>>> >>>>>> 2015-10-07 16:48 GMT+02:00 Theodore Vasiloudis < >>>>>> theodoros.vasilou...@gmail.com>: >>>>>> >>>>>>>> >>>>>>>> Could we maybe do a "PR overall status assessment" once per week >>> or >>>>> so, >>>>>>>> where we find those problematic PRs and try to assign them / close >>>>> them? >>>>>>> >>>>>>> >>>>>>> I like this idea, as it would raise awareness about lingering PRs. >>>>> Does >>>>>>> anybody know if there is >>>>>>> some way to integrate this into JIRA, so we can easily see (and >>>>>>> filter/sort) lingering PRs? >>>>>>> >>>>>>> Cheers, >>>>>>> Theo >>>>>>> >>>>>>> On Wed, Oct 7, 2015 at 12:04 PM, Vasiliki Kalavri < >>>>>>> vasilikikala...@gmail.com >>>>>>>> wrote: >>>>>>> >>>>>>>> Hey, >>>>>>>> >>>>>>>> I agree that we need to organize the PR process. A PR management >>> tool >>>>>>> would >>>>>>>> be great. >>>>>>>> >>>>>>>> However, it seems to me that the shepherding process described is >>>>> -more >>>>>>> or >>>>>>>> less- what we've already been doing. There is usually a person >>> that >>>>>>> reviews >>>>>>>> the PR and kind-of drives the process. Maybe making this explicit >>>>> will >>>>>>> make >>>>>>>> things go faster. >>>>>>>> >>>>>>>> There is a problem I see here, quite related to what Theo also >>>>>>> mentioned. >>>>>>>> For how long do we let a PR lingering around without a shepherd? >>> What >>>>>>> do we >>>>>>>> do if nobody volunteers? >>>>>>>> Could we maybe do a "PR overall status assessment" once per week >>> or >>>>> so, >>>>>>>> where we find those problematic PRs and try to assign them / close >>>>> them? >>>>>>>> >>>>>>>> Finally, I think shepherding one's own PR is a bad idea (the >>> review >>>>>>> part) >>>>>>>> unless it's something trivial. >>>>>>>> >>>>>>>> Cheers and see you very soon, >>>>>>>> -Vasia. >>>>>>>> On Oct 7, 2015 11:24 AM, "Matthias J. Sax" <mj...@apache.org> >>> wrote: >>>>>>>> >>>>>>>>> Ok. That makes sense. So most people will need to change >>> behavior >>>>> and >>>>>>>>> start discussions in JIRA and not over dev list. Furthermore, >>>>> issues >>>>>>>>> list must be monitored more carefully... (I personally, watch >>> dev >>>>>>>>> carefully and only skip over issues list right now) >>>>>>>>> >>>>>>>>> -Matthias >>>>>>>>> >>>>>>>>> On 10/07/2015 10:22 AM, Fabian Hueske wrote: >>>>>>>>>> @Matthias: That's a good point. Each PR should be backed by a >>>>> JIRA >>>>>>>> issue. >>>>>>>>>> If that's not the case, we have to make the contributor aware >>> of >>>>>>> that >>>>>>>>> rule. >>>>>>>>>> I'll update the process to keep all discussions in JIRA (will >>> be >>>>>>>> mirrored >>>>>>>>>> to issues ML), OK? >>>>>>>>>> >>>>>>>>>> @Theo: You are right. Adding this process won't be the silver >>>>>>> bullet to >>>>>>>>> fix >>>>>>>>>> all PR related issues. >>>>>>>>>> But I hope it will help to improve the overall situation. >>>>>>>>>> >>>>>>>>>> Are there any other comment? Otherwise I would start to >>> prepare >>>>> add >>>>>>>> this >>>>>>>>> to >>>>>>>>>> our website. >>>>>>>>>> >>>>>>>>>> Thanks, Fabian >>>>>>>>>> >>>>>>>>>> 2015-10-06 9:46 GMT+02:00 Theodore Vasiloudis < >>>>>>>>>> theodoros.vasilou...@gmail.com>: >>>>>>>>>> >>>>>>>>>>> One problem that we are seeing with FlinkML PRs is that there >>>>> are >>>>>>>> simply >>>>>>>>>>> not enough commiters to "shepherd" all of them. >>>>>>>>>>> >>>>>>>>>>> While I think this process would help generally, I don't >>> think >>>>> it >>>>>>>> would >>>>>>>>>>> solve this kind of problem. >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Theodore >>>>>>>>>>> >>>>>>>>>>> On Mon, Oct 5, 2015 at 3:28 PM, Matthias J. Sax < >>>>> mj...@apache.org> >>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> One comment: >>>>>>>>>>>> We should ensure that contributors follow discussions on the >>>>> dev >>>>>>>>> mailing >>>>>>>>>>>> list. Otherwise, they might miss important discussions >>>>> regarding >>>>>>>> their >>>>>>>>>>>> PR (what happened already). Thus, the contributor was >>> waiting >>>>> for >>>>>>>>>>>> feedback on the PR, while the reviewer(s) waited for the PR >>> to >>>>> be >>>>>>>>>>>> updated according to the discussion consensus, resulting in >>>>>>>> unnecessary >>>>>>>>>>>> delays. >>>>>>>>>>>> >>>>>>>>>>>> -Matthias >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 10/05/2015 02:18 PM, Fabian Hueske wrote: >>>>>>>>>>>>> Hi everybody, >>>>>>>>>>>>> >>>>>>>>>>>>> Along with our efforts to improve the “How to contribute” >>>>> guide, >>>>>>> I >>>>>>>>>>> would >>>>>>>>>>>>> like to start a discussion about a setting up a review >>> process >>>>>>> for >>>>>>>>> pull >>>>>>>>>>>>> requests. >>>>>>>>>>>>> >>>>>>>>>>>>> Right now, I feel that our PR review efforts are often a >>> bit >>>>>>>>>>> unorganized. >>>>>>>>>>>>> This leads to situation such as: >>>>>>>>>>>>> >>>>>>>>>>>>> - PRs which are lingering around without review or feedback >>>>>>>>>>>>> - PRs which got a +1 for merging but which did not get >>> merged >>>>>>>>>>>>> - PRs which have been rejected after a long time >>>>>>>>>>>>> - PRs which became irrelevant because some component was >>>>>>> rewritten >>>>>>>>>>>>> - PRs which are lingering around and have been abandoned by >>>>> their >>>>>>>>>>>>> contributors >>>>>>>>>>>>> >>>>>>>>>>>>> To address these issues I propose to define a pull request >>>>> review >>>>>>>>>>> process >>>>>>>>>>>>> as follows: >>>>>>>>>>>>> >>>>>>>>>>>>> 1. [Get a Shepherd] Each pull request is taken care of by a >>>>>>>> shepherd. >>>>>>>>>>>>> Shepherds are committers that voluntarily sign up and *feel >>>>>>>>>>> responsible* >>>>>>>>>>>>> for helping the PR through the process until it is merged >>> (or >>>>>>>>>>> discarded). >>>>>>>>>>>>> The shepherd is also the person to contact for the author >>> of >>>>> the >>>>>>>> PR. A >>>>>>>>>>>>> committer becomes the shepherd of a PR by dropping a >>> comment >>>>> on >>>>>>>> Github >>>>>>>>>>>> like >>>>>>>>>>>>> “I would like to shepherd this PR”. A PR can be reassigned >>>>> with >>>>>>> lazy >>>>>>>>>>>>> consensus. >>>>>>>>>>>>> >>>>>>>>>>>>> 2. [Accept Decision] The first decision for a PR should be >>>>>>> whether >>>>>>>> it >>>>>>>>>>> is >>>>>>>>>>>>> accepted or not. This depends on a) whether it is a desired >>>>>>> feature >>>>>>>> or >>>>>>>>>>>>> improvement for Flink and b) whether the higher-level >>> solution >>>>>>>> design >>>>>>>>>>> is >>>>>>>>>>>>> appropriate. In many cases such as bug fixes or discussed >>>>>>> features >>>>>>>> or >>>>>>>>>>>>> improvements, this should be an easy decision. In case of >>>>> more a >>>>>>>>>>> complex >>>>>>>>>>>>> feature, the discussion should have been started when the >>>>>>> mandatory >>>>>>>>>>> JIRA >>>>>>>>>>>>> was created. If it is still not clear whether the PR >>> should be >>>>>>>>> accepted >>>>>>>>>>>> or >>>>>>>>>>>>> not, a discussion should be started in JIRA (a JIRA issue >>>>> needs >>>>>>> to >>>>>>>> be >>>>>>>>>>>>> created if none exists). The acceptance decision should be >>>>>>> recorded >>>>>>>> by >>>>>>>>>>> a >>>>>>>>>>>>> “+1 to accept” message in Github. If the PR is not >>> accepted, >>>>> it >>>>>>>> should >>>>>>>>>>> be >>>>>>>>>>>>> closed in a timely manner. >>>>>>>>>>>>> >>>>>>>>>>>>> 3. [Merge Decision] Once a PR has been “accepted”, it >>> should >>>>> be >>>>>>>>> brought >>>>>>>>>>>>> into a mergeable state. This means the community should >>>>> quickly >>>>>>>> react >>>>>>>>>>> on >>>>>>>>>>>>> contributor questions or PR updates. Everybody is >>> encouraged >>>>> to >>>>>>>> review >>>>>>>>>>>> pull >>>>>>>>>>>>> requests and give feedback. Ideally, the PR author does not >>>>> have >>>>>>> to >>>>>>>>>>> wait >>>>>>>>>>>>> for a long time to get feedback. The shepherd of a PR >>> should >>>>> feel >>>>>>>>>>>>> responsible to drive this process forward. Once the PR is >>>>>>> considered >>>>>>>>> to >>>>>>>>>>>> be >>>>>>>>>>>>> mergeable, this should be recorded by a “+1 to merge” >>> message >>>>> in >>>>>>>>>>> Github. >>>>>>>>>>>> If >>>>>>>>>>>>> the pull request becomes abandoned at some point in time, >>> it >>>>>>> should >>>>>>>> be >>>>>>>>>>>>> either taken over by somebody else or be closed after a >>>>>>> reasonable >>>>>>>>>>> time. >>>>>>>>>>>>> Again, this can be done by anybody, but the shepherd should >>>>> feel >>>>>>>>>>>>> responsible to resolve the issue. >>>>>>>>>>>>> >>>>>>>>>>>>> 4. Once, the PR is in a mergeable state, it should be >>> merged. >>>>>>> This >>>>>>>> can >>>>>>>>>>> be >>>>>>>>>>>>> done by anybody, however the shepherd should make sure >>> that it >>>>>>>> happens >>>>>>>>>>>> in a >>>>>>>>>>>>> timely manner. >>>>>>>>>>>>> >>>>>>>>>>>>> IMPORTANT: Everybody is encouraged to discuss pull >>> requests, >>>>> give >>>>>>>>>>>> feedback, >>>>>>>>>>>>> and merge pull requests which are in a good shape. However, >>>>> the >>>>>>>>>>> shepherd >>>>>>>>>>>>> should feel responsible to drive a PR forward if nothing >>>>> happens. >>>>>>>>>>>>> >>>>>>>>>>>>> By defining a review process for pull requests, I hope we >>> can >>>>>>>>>>>>> >>>>>>>>>>>>> - Improve the satisfaction of and interaction with >>>>> contributors >>>>>>>>>>>>> - Improve and speed-up the review process of pull requests. >>>>>>>>>>>>> - Close irrelevant and stale PRs more timely >>>>>>>>>>>>> - Reduce the effort for code reviewing >>>>>>>>>>>>> >>>>>>>>>>>>> The review process can be supported by some tooling: >>>>>>>>>>>>> >>>>>>>>>>>>> - A QA bot that checks the quality of pull requests such as >>>>>>> increase >>>>>>>>> of >>>>>>>>>>>>> compiler warnings, code style, API changes, etc. >>>>>>>>>>>>> - A PR management tool such as Sparks PR dashboard (see: >>>>>>>>>>>>> https://spark-prs.appspot.com/) >>>>>>>>>>>>> >>>>>>>>>>>>> I would like to start discussions about using such tools >>>>> later as >>>>>>>>>>>> separate >>>>>>>>>>>>> discussions. >>>>>>>>>>>>> >>>>>>>>>>>>> Looking forward to your feedback, >>>>>>>>>>>>> Fabian >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
