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 > > > > > > > > > > > >
