Re: [DISCUSS] Definition of Done for Apache Flink

[Johannes Moser <jo...@ververica.com>]

Hi everyone,

thanks for discussing this and providing your input. 

Here's a summary of what I got out of your responses:
* There are already process and template in place, which isn't really executed.
* Just adjusting the process or the template won't help.
* Opening PRs shouldn't become more difficult (rather easier)
* Committers should be aware of the 'Definition of Done' and check it.
* The Github template should inform the contributor on what is expected when issuing a PR.
* Martijn is suggesting a three step template (code contribution process, tests, documentation).
* They could be automated.
* There are silent fans of the current template.
* Manually listing what tests changed have no value.
* We should use checkboxes.
* The bot is not leveraged.

Clarification/thoughts from my end:
* @Francesco, I'm aware of that list and extended it here [1]
* @Fabian, we were hoping by putting more emphasis on quality, the stability of master would also increase.
* Most of the comments circled around the PR template, that's why I assume the community is fine with the change in the guide.

Generally speaking I'm aware that there are a lot of potential improvements around the whole contribution process. Bot, awareness, PR template in general, ... a few of them have been mentioned. With my PRs I mainly wanted to address the clarification of the 'definition of done'. While I'm happy to contribute to other suggestions in further step, I'd like to keep to focus on the DoD for now.

What I would suggest:
* I will merge the flink-web PR as this seems to be fine. [1]
* I will update the template PR to make it simpler (and use CHECKBOXES) in general, while keeping the aspects.

Allow me, just one more comment. I'm well aware, as some of you pointed out, this is a cultural and habit topic. The best template doesn't make sense when it is not executed. My goal on the one hand was to complete the level of done, raise awareness for tests and documentation, as well as for a sense of what 'done' means to the Apache Flink community in general. At the end it is up to everyone individually to execute what is already there and whatever we might agree on adding. By having so many people discussing this we already came a step closer. 

Best, Joe

[1] https://github.com/apache/flink-web/pull/481

> On 25.11.2021, at 10:56, Martijn Visser <ma...@ververica.com> wrote:
> 
>> * 187 contain "yes / no / don't know" and thus have not answered at least
> one of the questions
> 
> I think there are quite some PRs that highlight in bold the answer to this
> question while keeping all options there. I'm all in favour of replacing
> this with a checkbox.
> 
> 
> 
> On Thu, 25 Nov 2021 at 10:47, Ingo Bürk <in...@ververica.com> wrote:
> 
>> Hi Till,
>> 
>>> * I agree with Ingo that the "Verifying this change" section can be
>>> cumbersome to fill in. On the other hand it reminds contributors to
>> verify
>>> that his/her changes are covered by tests. Therefore, I would keep it.
>> 
>> IMO it could be replaced with a checkbox "I have made sure all changes are
>> covered by tests", though. I still see no benefit in painstakingly picking
>> out all individual test cases that have been touched or affected.
>> 
>>> In order to verify this I went through
>>> the first two pages of open PRs and I was very positively surprised that
>>> almost all PRs filled out the current template.
>> 
>> Just to try and complete the picture here, counting open PRs by including
>> search terms like "yes / no / don't know" the rough statistics are
>> 
>> * 731 open PRs
>> * 187 contain "yes / no / don't know" and thus have not answered at least
>> one of the questions
>> * 134 PRs seem to have deleted the PR template altogether as the question
>> text does not appear in it
>> 
>> Given that the latter two have to be largely mutually exclusive, that would
>> mean that 40+% of all PRs do not fill out the PR template.
>> 
>>> because what is not used can be removed.
>> 
>> Just because someone answers a question doesn't make it a useful question,
>> though. Some questions undoubtedly have a purpose for PR authors, like
>> having to decide whether the public API is affected. But what purpose do
>> "Anything that affects deployment or recovery" or "The S3 file system
>> connector" serve? I don't think this is useful to authors in any way, so
>> the other question is whether any committer actually looks at these answers
>> and does something with it. Even if we don't remove everything (which I
>> wouldn't want to, either), we can still remove those things that aren't
>> needed (if that is the case).
>> 
>> (Also, in any case I would really love if all questions could be converted
>> to checkboxes instead)
>> 
>> 
>> Ingo
>> 
>> On Thu, Nov 25, 2021 at 10:31 AM Till Rohrmann <tr...@apache.org>
>> wrote:
>> 
>>> When I started writing this response I thought that the current PR
>> template
>>> is mostly ignored by the community. In order to verify this I went
>> through
>>> the first two pages of open PRs and I was very positively surprised that
>>> almost all PRs filled out the current template. Hence, I had to redact my
>>> original response to get rid of the template because what is not used can
>>> be removed.
>>> 
>>> What I like about the current template is that it gives structure and
>>> reminds contributors about certain things. At some places, the current
>>> template is a bit verbose imo and could be shortened. Especially, with
>>> Joe's proposal to add some more text to the template we might want to use
>>> the chance to consolidate things. Here are some ideas:
>>> 
>>> * Having a description of what the PR changes is very important. Whether
>>> every PR needs additionally the "Brief changelog" is not clear to me.
>> Maybe
>>> we can fuse those two sections.
>>> * I agree with Ingo that the "Verifying this change" section can be
>>> cumbersome to fill in. On the other hand it reminds contributors to
>> verify
>>> that his/her changes are covered by tests. Therefore, I would keep it.
>>> * The section "Does this pull request potentially affect one of the
>>> following parts" could be shortened. Some of the points are obvious when
>>> looking at the code, others are really hard to know for a contributor and
>>> others are niche. Maybe keeping whether we changed dependencies and
>> changed
>>> the public API (even though this should be automatically verifiable)
>> could
>>> be a start.
>>> * The section "Documentation" could be replaced by Joe's checklist.
>>> 
>>> Cheers,
>>> Till
>>> 
>>> On Mon, Nov 22, 2021 at 11:13 AM Matthias Pohl <ma...@ververica.com>
>>> wrote:
>>> 
>>>> I also like the checklist provided by our current PR template. One
>>> annoying
>>>> thing in my opinion, though, is that we do not rely on checkboxes. Ingo
>>>> already proposed such a change in [1]. Chesnay had some good points on
>>>> certain items that were meant to be added to the template but are
>>> actually
>>>> already checked automatically [2]. In the end it comes down to noticing
>>>> these checks and acting on it if one of them fails. I see the benefits
>> of
>>>> having an explicit check for something like that in the PR. But again,
>>>> adding more items increases the risk of users just ignoring it.
>>>> 
>>>> One other thing to raise awareness for users might be to move the
>>>> CONTRIBUTING.md into the root folder. Github still recognizes the file
>> if
>>>> it is located in the project's root [3]. Hence, I don't see a need to
>>>> "hide" it in the .github subfolder. Or was there another reason to put
>>> the
>>>> file into that folder?
>>>> 
>>>> Matthias
>>>> 
>>>> [1] https://github.com/apache/flink/pull/17801#issuecomment-969363303
>>>> [2] https://github.com/apache/flink/pull/17801#issuecomment-970048058
>>>> [3]
>>>> 
>>>> 
>>> 
>> https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors#about-contributing-guidelines
>>>> 
>>>> On Thu, Nov 18, 2021 at 12:03 PM Yun Tang <ta...@apache.org> wrote:
>>>> 
>>>>> Hi Joe,
>>>>> 
>>>>> Thanks for bringing this to our attention.
>>>>> 
>>>>> In general, I agreed with Chesnay's reply on PR [1]. For the rule-3,
>> we
>>>>> might indeed create another PR to add documentation previously. And I
>>>> think
>>>>> if forcing to obey it to include the documentation in the same PR,
>> that
>>>>> could benefit the review progress. Thus, I am not against for this
>>> rule.
>>>>> 
>>>>> For the rule related to the PR description, I think current flinkbot
>>> has
>>>>> tools to let committer to run command like "@flinkbot approve
>>>> description".
>>>>> However, I think many committers did not leverage this, which makes
>> the
>>>> bot
>>>>> useless at most of the time. I think this discussion draws the
>>> attention
>>>>> that whether we should strictly obey the review process via using
>>>> flinkbot
>>>>> or still not force committer to leverage it.
>>>>> 
>>>>> [1]
>> https://github.com/apache/flink/pull/17801#issuecomment-970048058
>>>>> 
>>>>> Best
>>>>> Yun Tang
>>>>> 
>>>>> On 2021/11/16 10:38:39 Ingo Bürk wrote:
>>>>>>> On the other hand I am a silent fan of the current PR template
>>>> because
>>>>>>> it also provides a summary of the PR to make it easier for
>>> committers
>>>>>>> to determine the impacts.
>>>>>> 
>>>>>> I 100% agree that part of a PR (and thus the template) should be
>> the
>>>>>> summary of the what, why, and how of the changes. I also see value
>> in
>>>>>> marking a PR as a breaking change if the author is aware of it
>> being
>>>> one
>>>>>> (of course a committer needs to verify this nonetheless).
>>>>>> 
>>>>>> But apart from that, there's a lot of questions in there that no
>> one
>>>>> seems
>>>>>> to care about, and e.g. the question of how a change can be
>> verified
>>>>> seems
>>>>>> fairly useless to me: if tests have been changed, that can
>> trivially
>>> be
>>>>>> seen in the PR. The CI runs on top of that anyway as well. So I
>> never
>>>>>> really understood why I need to manually list all the tests I have
>>>>> touched
>>>>>> here (or maybe I misunderstood this question the entire time?).
>>>>>> 
>>>>>> If the template is supposed to be useful for the committer rather
>>> than
>>>>> the
>>>>>> author, it would have to be mandatory to fill it out, which
>> de-facto
>>> it
>>>>>> isn't.
>>>>>> 
>>>>>> Also, even if we keep all the same information, I would still love
>> to
>>>> see
>>>>>> it converted into checkboxes. I know it's a small detail, but it's
>>> much
>>>>>> less annoying than the current template. Something like
>>>>>> 
>>>>>> ```
>>>>>> - [ ] This pull requests changes the public API (i.e., any class
>>>>> annotated
>>>>>> with `@Public(Evolving)`)
>>>>>> - [ ] This pull request adds, removes, or updates dependencies
>>>>>> - [ ] I have updated the documentation to reflect the changes made
>> in
>>>>> this
>>>>>> pull request
>>>>>> ```
>>>>>> 
>>>>>> On Tue, Nov 16, 2021 at 10:28 AM Fabian Paul <fp...@apache.org>
>>> wrote:
>>>>>> 
>>>>>>> Hi all,
>>>>>>> 
>>>>>>> Maybe I am the devil's advocate but I see the stability of master
>>> and
>>>>>>> the definition of done as disjunct properties. I think it is
>> more a
>>>>>>> question of prioritization that test instabilities are treated as
>>>>>>> critical tickets and have to be addressed before continuing any
>>> other
>>>>>>> work. It will always happen that we merge code that is not 100%
>>>>>>> stable; that is probably the nature of software development. I
>>> agree
>>>>>>> when it comes to documentation that PRs are only mergeable if the
>>>>>>> documentation has also been updated.
>>>>>>> 
>>>>>>> On the other hand I am a silent fan of the current PR template
>>>> because
>>>>>>> it also provides a summary of the PR to make it easier for
>>> committers
>>>>>>> to determine the impacts. It also reminds the contributors of our
>>>>>>> principles i.e. how do you verify the change should probably not
>> be
>>>>>>> answered with "test were not possible".
>>>>>>> 
>>>>>>> I agree with @Martijn Visser that we can improve the CI i.e.
>>>>>>> performance regression test, execute s3 test but these things
>>> should
>>>>>>> be addressed in another discussion.
>>>>>>> 
>>>>>>> So I would prefer to keep the current PR template.
>>>>>>> 
>>>>>>> Best,
>>>>>>> Fabian
>>>>>>> 
>>>>>>> On Tue, Nov 16, 2021 at 10:17 AM Martijn Visser <
>>>> martijn@ververica.com
>>>>>> 
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> Hi all,
>>>>>>>> 
>>>>>>>> Thanks for bringing this up for this discussion, because I
>> think
>>>>> it's an
>>>>>>>> important aspect.
>>>>>>>> 
>>>>>>>> From my perspective, a 'definition of done' serves two
>> purposes:
>>>>>>>> 1. It informs the contributor on what's expected when making a
>>>>>>> contribution
>>>>>>>> in the form of a PR
>>>>>>>> 2. It instructs the committer on what to check before
>>>>> accepting/merging
>>>>>>> a PR
>>>>>>>> 
>>>>>>>> I would use a Github template primarily to deal with the first
>>>>> purpose. I
>>>>>>>> think that should be short and easily understandable,
>> preferably
>>>>> with as
>>>>>>>> many automated checks as possible.
>>>>>>>> 
>>>>>>>> I would propose something like this to condense information.
>>>>>>>> 
>>>>>>>> 1. It is following the code contribution process, including
>> code
>>>>> style
>>>>>>> and
>>>>>>>> quality guide
>>>>> https://flink.apache.org/contributing/contribute-code.html
>>>>>>>> 2. It is covered by tests and all tests have passed
>>>>>>>> 3. If it has user facing changes the documentation has been
>>> updated
>>>>>>>> according to the documentation style guide
>>>>>>>> 
>>>>>>>> These 3 DoD can probably be broken down into multiple
>> automation
>>>>> tests:
>>>>>>>> 
>>>>>>>> * Run a spotless check
>>>>>>>> * Run a license check
>>>>>>>> * Compile application
>>>>>>>> * Run tests
>>>>>>>> * Run E2E tests
>>>>>>>> * Build documentation
>>>>>>>> * Check if JIRA has been mentioned and exists in the PR title
>> and
>>>>> commit
>>>>>>>> message
>>>>>>>> etc.
>>>>>>>> 
>>>>>>>> Best regards,
>>>>>>>> 
>>>>>>>> Martijn
>>>>>>>> 
>>>>>>>> On Tue, 16 Nov 2021 at 09:08, Francesco Guardiani <
>>>>>>> francesco@ververica.com>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> +1 with Ingo proposal, the goal of the template should be to
>>> help
>>>>>>> developer
>>>>>>>>> to do a self check of his/her PR quality, not to define
>> whether
>>>>>>> something
>>>>>>>>> is done or not. It's up to the committer to check that the
>>>>> "definition
>>>>>>> of
>>>>>>>>> done" is fulfilled.
>>>>>>>>> 
>>>>>>>>>> The Definition of Done as suggested:
>>>>>>>>> 
>>>>>>>>> This checklist makes sense to me, although it seems to me we
>>>>> already
>>>>>>> have
>>>>>>>>> these bullet points defined here:
>>>>>>>>> https://flink.apache.org/contributing/contribute-code.html
>>>>>>>>> 
>>>>>>>>> On Tue, Nov 16, 2021 at 8:16 AM Ingo Bürk <
>> ingo@ververica.com>
>>>>> wrote:
>>>>>>>>> 
>>>>>>>>>> Hi Joe,
>>>>>>>>>> 
>>>>>>>>>> thank you for starting this discussion. Having a common
>>>>> agreement on
>>>>>>> what
>>>>>>>>>> to expect from a PR for it to be merged is very much a
>>>> worthwhile
>>>>>>> goal.
>>>>>>>>>> 
>>>>>>>>>> I'm slightly worried about the addition to the PR template.
>>> We
>>>>>>> shouldn't
>>>>>>>>>> make opening PRs even more difficult (unless it adds
>>> sufficient
>>>>>>> benefit).
>>>>>>>>>> 
>>>>>>>>>> There are two main benefits to have from using templates:
>>>>> requiring
>>>>>>>>>> information from authors to automate certain feedback, and
>>>>> serving
>>>>>>> as a
>>>>>>>>>> self-control checklist for contributors.
>>>>>>>>>> 
>>>>>>>>>> As it stands, a large number of PRs don't fill out the
>>>> template,
>>>>> and
>>>>>>> I
>>>>>>>>>> haven't yet seen anyone not merge a PR over that, so
>> de-facto
>>>> we
>>>>> are
>>>>>>> not
>>>>>>>>>> using it for the former.
>>>>>>>>>> 
>>>>>>>>>> For the latter purpose of contributors having a checklist
>> for
>>>>>>>>> themselves, I
>>>>>>>>>> think the current template is too long already and contains
>>> the
>>>>> wrong
>>>>>>>>>> content. Being short here is key if we want anyone to read
>>> it,
>>>>> and
>>>>>>>>>> personally I would cut it down significantly to a
>> description
>>>>> and a
>>>>>>>>> couple
>>>>>>>>>> of checkboxes.
>>>>>>>>>> 
>>>>>>>>>> This isn't exactly the scope of your proposal, but
>>> personally I
>>>>>>> wouldn't
>>>>>>>>>> like to add even more questions that need to be filled out,
>>>>>>> especially
>>>>>>>>>> since they don't actually need to be filled out. It just
>>>> creates
>>>>> an
>>>>>>>>>> annoying burden for contributors and is ignored by those
>> who
>>>>> might
>>>>>>>>> benefit
>>>>>>>>>> most from reading it anyway.
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> Ingo
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On Mon, Nov 15, 2021, 22:36 Johannes Moser <
>>> joe@ververica.com>
>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> Dear Flink Community,
>>>>>>>>>>> 
>>>>>>>>>>> We as the release managers of the 1.15 release are
>>> suggesting
>>>>> to
>>>>>>>>>> introduce
>>>>>>>>>>> a “Definition of Done".
>>>>>>>>>>> 
>>>>>>>>>>> Let me elaborate a bit on the reasons:
>>>>>>>>>>> * During the release process for 1.14 the stability of
>>> master
>>>>> was
>>>>>>>>>>> sometimes in a state that made contributing to Apache
>>> Flink a
>>>>> bad
>>>>>>>>>>> experience.
>>>>>>>>>>> * Some of the changes that have been contributed seem to
>> be
>>>>>>> unusable by
>>>>>>>>>>> users because of defects.
>>>>>>>>>>> * Documentation is neglected which also leads to users
>>> unable
>>>>> to
>>>>>>> make
>>>>>>>>> use
>>>>>>>>>>> of changes. One of the reasons is, because documentation
>> is
>>>>> often
>>>>>>>>> pushed
>>>>>>>>>> to
>>>>>>>>>>> a later state.
>>>>>>>>>>> 
>>>>>>>>>>> With this definition of done awareness and sensibility
>> for
>>>>> these
>>>>>>> aspect
>>>>>>>>>>> should be increased. Both, for the ones who are
>> committing
>>>> and
>>>>> for
>>>>>>> the
>>>>>>>>>> ones
>>>>>>>>>>> that are reviewing.
>>>>>>>>>>> We focus on code quality, testing and documentation. A
>>> shared
>>>>>>>>>>> understanding is created.
>>>>>>>>>>> 
>>>>>>>>>>> The Definition of Done as suggested:
>>>>>>>>>>> 
>>>>>>>>>>> -
>>>>>>>>>>> A PR is done and can be merged, when:
>>>>>>>>>>> 
>>>>>>>>>>> 1. It is following the code contribution process
>>>>>>>>>>> 2. It is implemented according to the code style and
>>> quality
>>>>> guide.
>>>>>>>>>>> 3. If it has user facing changes the documentation has
>> been
>>>>> updated
>>>>>>>>>>> according to the documentation style guide.
>>>>>>>>>>> 4. It is covered by tests.
>>>>>>>>>>> 5. All tests passed.
>>>>>>>>>>> -
>>>>>>>>>>> 
>>>>>>>>>>> There are two PRs to illustrate the changes.
>>>>>>>>>>> https://github.com/apache/flink-web/pull/481 <
>>>>>>>>>>> https://github.com/apache/flink-web/pull/481>
>>>>>>>>>>> https://github.com/apache/flink/pull/17801 <
>>>>>>>>>>> https://github.com/apache/flink/pull/17801>
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> It isn’t the goal to make it harder to get changes into
>>>> Apache
>>>>>>> Flink.
>>>>>>>>> It
>>>>>>>>>>> is rather the opposite of making contributing and using
>>>> Apache
>>>>>>> Flink a
>>>>>>>>>>> better experience.
>>>>>>>>>>> By creating awareness a push towards quality and
>> usability
>>>>> should
>>>>>>>>> happen.
>>>>>>>>>>> 
>>>>>>>>>>> I’m happy to hear your feedback.
>>>>>>>>>>> 
>>>>>>>>>>> Best,
>>>>>>>>>>> Joe
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>> 
>>> 
>>