> * 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 <i...@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 <trohrm...@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 <matth...@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 <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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
