flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Hogan <c...@greghogan.com>
Subject Re: [DISCUSS] A more thorough Pull Request check list and template
Date Tue, 18 Jul 2017 15:52:00 GMT
Thanks for leading this discussion, Stephan. I don’t disagree with anything that has been
said but am slightly concerned that the improvements in documenting pull requests won’t
translate into the git commit messages. Acceptance of a higher standard will be swift as long
as reviewers set the example and expectation. Perhaps a new template could be trialled as
opt-in for a few weeks by several or more committers.


> On Jul 18, 2017, at 10:58 AM, Stephan Ewen <sewen@apache.org> wrote:
> My thinking was exactly as echoed by Gordon and Ufuk:
>  - The yes/no sections are also for reviewers a reminder of what to watch
> out for.
>    Let's face it, probably half of the committers are not aware that these
> things need to be checked implicitly against every change.
>    A good part of the recent issues came from exactly that. Changes get
> merged (because the pull request lingered or the number of open PRs is
> high) and these implications are not thought through.
>  - This is to me a tradeoff between requiring explicit +1s from certain
> people (maintainers) for certain components, and getting an awareness into
> everybody's mind.
>  - It also makes all users aware that these things are considered and
> implicitly manages expectations in how fast can things get merged.
> Concerning the long text: I think it is fine to play the ball a bit more to
> the contributors.
> Making it easy, yes. But also making it correct and well. We need to make
> contributors aware of what it means to contribute to a system to runs
> highly available critical infrastructure. There is quite often still the
> mindset of "hey, cool, open source, let me throw something out there".
> My take is that anyone who is serious about contributing and serious about
> quality is not put off by this template.
> Concerning the introductory text: I bet that rarely anyone reads the "how
> to contribute" guide. Before the template, virtually no new pull request
> had even the required naming.
> That text needs to be in the template, or we might as well not have it
> anywhere at all.
> Just for reference: Below is the introductory text of the JDK ;-)
> 5. Know what to expect
> Only the best patches submitted will actually make it all the way into a
> JDK code base. The goal is not to take in the maximum number of
> contributions possible, but rather to accept only the highest-quality
> contributions. The JDK is used daily by millions of people and thousands of
> businesses, often in mission-critical applications, and so we can't afford
> to accept anything less than the very best.
> If you're relatively new to the Java platform then we recommend that you
> gain more experience writing Java applications before you attempt to work
> on the JDK itself. The purpose of the sponsored-contribution process is to
> bring developers who already have the skills required to work on the JDK
> into the existing development community. The members of that community have
> neither the time nor the patience required to teach basic Java programming
> skills or platform implementation techniques.
> On Tue, Jul 18, 2017 at 12:15 PM, Ufuk Celebi <uce@apache.org> wrote:
>> On Tue, Jul 18, 2017 at 10:47 AM, Fabian Hueske <fhueske@gmail.com> wrote:
>>> For example even if the question about changed dependencies is answered
>>> with "no", the reviewer still has to check that.
>> But having it as a required option/text in the PR descriptions helps
>> reviewers to actually remember to check that. I think we should be
>> more realistic here and assume that reviewers will also overlook
>> things etc.
>> To me, keeping the questions is more important than the intro text.
>> Therefore, I would be OK with moving the text to the contrib guide,
>> but I would definitely keep the detailed yes/nos and not go with high
>> level questions that everyone will answer differently.
>> – Ufuk

View raw message