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 <tang...@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 <mart...@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 < > > > france...@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 <i...@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 <j...@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 > > > > > > > > > > > > > > > >
