I generally like the idea of adding this rule but I have a few concerns around the amount of unfinished/incomplete PRs this could lead to.
We generally follow such things in my org where we try to resolve all the comments from the maintainers before performing a merge. But what we also do is that if a comment/ask from maintainer is outside bounds for that a particular PR, we generally create follow up JIRAs and then follow up on it later. Should we enforce creating a follow up Github issue in our case for the same? This would solve the issue of unfinished PRs while also giving confidence that the comments won't be ignored. Thanks & Regards, Amogh Desai On Wed, Dec 20, 2023 at 5:11 AM Jarek Potiuk <ja...@potiuk.com> wrote: > Dennis: > > > I guess as long as we are not dictating that the person who left the > comment has to resolve it, then I'm alright with trying this. I don't want > a PR to get blocked because of a drive-by comment. > > Yep. I think it is perfectly fine for the person who reviews and wants to > merge the code to resolve such obvious cases where a comment was just a > "comment" not an invitation to conversation. Or when you are an author, and > you see "All right - I am really done with it, now it's time to resolve all > the remaining conversations". > > Note that not every "comment" is one that gets into "resolvable" > conversation. There are generic comments for the whole PR that do not land > as "resolvable" conversations. Only the "suggestions" and comments for a > particular line (or lines) of code are "conversations" - when they relate > to a particular line of code. And those tend to be actual issues, questions > or doubts that **should** get some reaction IMHO. > > J. > > > > > On Tue, Dec 19, 2023 at 11:57 PM Ferruzzi, Dennis > <ferru...@amazon.com.invalid> wrote: > > > Interesting. I generally try to follow that policy as a best practice > on > > my own PRs just so I make sure I didn't miss comments, but there are also > > times I intentionally leave certain discussions "out in the open". I > > guess as long as we are not dictating that the person who left the > comment > > has to resolve it, then I'm alright with trying this. I don't want a PR > to > > get blocked because of a drive-by comment. > > > > Seems like this is easily reversible and we can give it a trial run and > > decide later if we want to keep it. > > > > > > - ferruzzi > > > > > > ________________________________ > > From: Jarek Potiuk <ja...@potiuk.com> > > Sent: Tuesday, December 19, 2023 12:45 PM > > To: dev@airflow.apache.org > > Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [DISCUSS] "Require > conversation > > resolution" in our PRs before merge? > > > > CAUTION: This email originated from outside of the organization. Do not > > click links or open attachments unless you can confirm the sender and > know > > the content is safe. > > > > > > > > AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. > > Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne > pouvez > > pas confirmer l’identité de l’expéditeur et si vous n’êtes pas certain > que > > le contenu ne présente aucun risque. > > > > > > > > > Given the slow down over the holidays I don't think two weeks will be > > enough - make it 4? > > > > Ah. True :) > > > > On Tue, Dec 19, 2023 at 9:41 PM Ash Berlin-Taylor <a...@apache.org> > wrote: > > > > > Given the slow down over the holidays I don't think two weeks will be > > > enough - make it 4? > > > > > > On 19 December 2023 20:33:23 GMT, Jarek Potiuk <ja...@potiuk.com> > wrote: > > > >Ash: > > > > > > > >> I.e. Convention over enforcement and treating people as mature > adults > > > not > > > >children who need guard rails. > > > > > > > >I think it's quite the opposite, Both 1) 2) and 3) reasoning is more > of > > > the > > > >aid for whoever looks at the PR that there are still some > conversations > > > not > > > >addressed, I personally feel it's treating people as more adult, when > > you > > > >allow them to unilaterally say "I believe the conversation is > resolved" > > > > > > > >Bolke: > > > > > > > >> This reflect my feelings as well. I'm not convinced we are solving > > > >something that needs to be solved. > > > > > > > >I think 1) 2) 3) are real problems that it addresses. > > > > > > > > > > > >If we have no "strong" -1s we can give it a try. It's not a one-way > > > street. > > > >We can always go back if we see it slows us down or annoys people. > > > > > > > >We can even set a way how we assess it. Maybe everyone should just > > collect > > > >cases where it caused some problems - in two weeks or so (should be > > > enough). > > > >Why not everyone who actively participates in the PR review process > > brings > > > >their experience and explains if it caused them unnecessary burden for > > no > > > >gain (also the opposite - where it helped). > > > > > > > >J. > > > > > > > > > > > >On Tue, Dec 19, 2023 at 9:15 PM Jarek Potiuk <ja...@potiuk.com> > wrote: > > > > > > > >> Answering some of the recent questions. > > > >> > > > >> Daniel: > > > >> > > > >> > E.g. when you "resolve" a conversation, then you make it less > > > visible. > > > >> This isn't always a good thing. Sometimes you just want to +1 it. > > When > > > >> others visit the PR, then they will not see the conversation. Maybe > > they > > > >> would want to engage in discussion. And when you get a notification > in > > > an > > > >> email about a comment and want to engage and respond, I've had > issues > > > with > > > >> following links to conversations after resolution before. > > > >> > > > >> True. However, you still see that there was conversation (and can > > always > > > >> un-collapse it). Resolving conversation does not "remove" it. > Actually > > > when > > > >> the conversation is resolved. > > > >> Also you can see it in the "conversations menu". And well, > assumption > > is > > > >> that resolving conversation makes it well - resolved :). And +1 > until > > > it is > > > >> resolved is still fine and cool (or even after). And as a > maintainer, > > > you > > > >> can always "unresolve" such a conversation. > > > >> > > > >> [image: image.png] > > > >> > > > >> Hussein: > > > >> > > > >> > The proposed rule isn't a bad idea, especially for ensuring that > > > >> maintainers wanting to merge have reviewed all conversions. However, > > > it's > > > >> essential to permit them to close open conversations if they find > the > > > >> comments have been addressed. Only ping the commenter if uncertain, > > > with a > > > >> maximum waiting time (let's say 48 hours on workdays). If the > > commenter > > > >> doesn't reply and there are no other open conversations, we can > merge. > > > >> > > > >> Absolutely. I think it should be fine to have either the author or > the > > > >> maintainer to resolve conversations - it all depends on context and > > > >> problem. I tend to think about "resolving" conversation as a > statement > > > of > > > >> intention / understanding rather than "certainty". It might be > either > > > the > > > >> author or the maintainer who "BELIEVES" that the conversation is > > > resolved. > > > >> It's subjective, not objective IMHO. We - as humans - can make > > mistakes > > > but > > > >> as long as we have good intentions, it's fine to resolve > conversation > > by > > > >> either. What matters here is that no conversation is simply "left > > > >> unaddressed" and that there was a deliberate action on each > > > conversation. > > > >> > > > >> From > > > >> > > > > > > https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations > > > >> > > > >> > You can resolve a conversation in a pull request if you opened the > > > pull > > > >> request or if you have write access to the repository where the pull > > > >> request was opened. > > > >> > > > >> So in our case - either the author, or one of the maintainers can > mark > > > the > > > >> conversation as resolved. > > > >> > > > >> > Could we forbid the authors from closing conversations? > > > >> > > > >> I am afraid not. > > > >> > > > >> > > > >> On Tue, Dec 19, 2023 at 7:31 PM Hussein Awala <huss...@awala.fr> > > wrote: > > > >> > > > >>> The proposed rule isn't a bad idea, especially for ensuring that > > > >>> maintainers wanting to merge have reviewed all conversions. > However, > > > it's > > > >>> essential to permit them to close open conversations if they find > the > > > >>> comments have been addressed. Only ping the commenter if uncertain, > > > with a > > > >>> maximum waiting time (let's say 48 hours on workdays). If the > > commenter > > > >>> doesn't reply and there are no other open conversations, we can > > merge. > > > >>> Additionally, should we wait for all open conversations or only > those > > > >>> opened by maintainers? Could we forbid the authors from closing > > > >>> conversations? > > > >>> > > > >>> On Tue 19 Dec 2023 at 19:19, Daniel Standish > > > >>> <daniel.stand...@astronomer.io.invalid> wrote: > > > >>> > > > >>> > +1 > > > >>> > > > > >>> > On Tue, Dec 19, 2023 at 9:36 AM Pierre Jeambrun < > > > pierrejb...@gmail.com> > > > >>> > wrote: > > > >>> > > > > >>> > > This is something I already try to apply on my own PRs, never > > merge > > > >>> > before > > > >>> > > explicitly solving all conversations. > > > >>> > > > > > >>> > > Also for a reviewer, I feel like this gives more confidence to > > the > > > >>> fact > > > >>> > > that the PR is ready, and indeed we are less subject to > missing a > > > >>> > > discussion or something going on making it 'not ok' to merge. > > Going > > > >>> over > > > >>> > > the entire thread before merging a PR to double check that > > > everything > > > >>> is > > > >>> > > actually addressed can be time consuming. That is especially > true > > > if > > > >>> > things > > > >>> > > are not marked as resolved. > > > >>> > > > > > >>> > > I agree that this is something that adds up some work, but I > > think > > > it > > > >>> is > > > >>> > > worth the experiment and see what happens. We can easily revert > > if > > > we > > > >>> are > > > >>> > > not happy with the way it goes. > > > >>> > > > > > >>> > > The workload will most likely be on the contributors' side, > that > > > will > > > >>> > have > > > >>> > > to actually address and solve all the conversations. > > > >>> > > > > > >>> > > Le mar. 19 déc. 2023 à 16:44, Vincent Beck < > vincb...@apache.org> > > a > > > >>> > écrit : > > > >>> > > > > > >>> > > > I am wondering too if this is not something that gives more > > work > > > to > > > >>> > > > maintainer without real benefits. A maintainer can still mark > > all > > > >>> > > > conversations as resolved and merge the PR if he wants. > > Though, I > > > >>> > > > understand there is the intention here as oppose as today > > where a > > > >>> > > > maintainer can just miss some comments. I am quite doubtful > > but I > > > >>> am in > > > >>> > > to > > > >>> > > > try it out and see how it goes. > > > >>> > > > > > > >>> > > > On 2023/12/19 14:55:13 Bolke de Bruin wrote: > > > >>> > > > > I'm less enthusiastic. What problem are we solving with > this? > > > If > > > >>> > > > something has not been addressed it can be done in a follow > up > > or > > > >>> of if > > > >>> > > it > > > >>> > > > was just part of the conversation it won't have impact on the > > > code. > > > >>> In > > > >>> > > > addition, the ones that need to deal with it are the ones > > merging > > > >>> and > > > >>> > > those > > > >>> > > > are not necessarily the same as the ones contributing. > > > >>> > > > > > > > >>> > > > > So for the friction that it creates with both the committer > > and > > > >>> the > > > >>> > > > contributer what is the benefit? > > > >>> > > > > > > > >>> > > > > B. > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > Sent from my iPhone > > > >>> > > > > > > > >>> > > > > > On 19 Dec 2023, at 15:45, Wei Lee <weilee...@gmail.com> > > > wrote: > > > >>> > > > > > > > > >>> > > > > > +1 for trying and observing how it works. My concern is > > that > > > >>> > adding > > > >>> > > > an additional obstacle might lead to more unfinished PRs. It > > > might > > > >>> be > > > >>> > > > helpful to give the contributor some guidance on when we can > > > resolve > > > >>> > the > > > >>> > > > comments. > > > >>> > > > > > > > > >>> > > > > > Best, > > > >>> > > > > > Wei > > > >>> > > > > > > > > >>> > > > > >> On Dec 19, 2023, at 9:28 PM, Andrey Anshin < > > > >>> > > andrey.ans...@taragol.is> > > > >>> > > > wrote: > > > >>> > > > > >> > > > >>> > > > > >> We could try and if found it slows down for some reason > we > > > >>> always > > > >>> > > > might > > > >>> > > > > >> revert it back. > > > >>> > > > > >> > > > >>> > > > > >> Just one suggestion, sometimes discussion contains some > > > useful > > > >>> > > > information, > > > >>> > > > > >> e.g. "What the reason of finally decision", "Useful > > > information > > > >>> > why > > > >>> > > it > > > >>> > > > > >> should works by suggested way, or should not work", > which > > > >>> might be > > > >>> > > > useful > > > >>> > > > > >> for someone who investigate why this changes was made, > so > > in > > > >>> this > > > >>> > > > case I > > > >>> > > > > >> would suggest to create link in the main thread of PR > with > > > >>> useful > > > >>> > > > > >> discussions. > > > >>> > > > > >> > > > >>> > > > > >> > > > >>> > > > > >> > > > >>> > > > > >> > > > >>> > > > > >>> On Tue, 19 Dec 2023 at 17:16, Jarek Potiuk < > > > ja...@potiuk.com> > > > >>> > > wrote: > > > >>> > > > > >>> > > > >>> > > > > >>> Hey everyone, > > > >>> > > > > >>> > > > >>> > > > > >>> TL;DR; I have a small proposal/discussion proposal to > > > modify a > > > >>> > bit > > > >>> > > > the > > > >>> > > > > >>> branch protection rules for Airflow. Why don't we add a > > > >>> > protection > > > >>> > > > > >>> rule in our PRs that requires all the comments in the > PRs > > > to > > > >>> be > > > >>> > > > > >>> "marked as resolved" before merging the PR ? > > > >>> > > > > >>> > > > >>> > > > > >>> I have been following myself - for quite some time - > an > > > >>> approach > > > >>> > > > that > > > >>> > > > > >>> whenever there are comments/suggestions/doubts in my > PRs > > I > > > do > > > >>> not > > > >>> > > > > >>> merge the PR until I **think** all of those have been > > > >>> addressed > > > >>> > > > > >>> (somehow). > > > >>> > > > > >>> > > > >>> > > > > >>> The resolution might not be what the person commenting > > > wants > > > >>> > > > directly, > > > >>> > > > > >>> it might be "I hear your comment, and there are good > > > reasons > > > >>> to > > > >>> > do > > > >>> > > > > >>> otherwise" or simply saying - "I know it could be done > > this > > > >>> way > > > >>> > > but I > > > >>> > > > > >>> think otherwise" etc. etc. But sometimes I miss that > > there > > > is > > > >>> a > > > >>> > > > > >>> comment that I have not reacted to, I skipped it > > > unconsciously > > > >>> > etc. > > > >>> > > > > >>> > > > >>> > > > > >>> I think having "some" kind of reaction to comments and > > > >>> deliberate > > > >>> > > "I > > > >>> > > > > >>> believe the conversation is resolved" is a very good > > thing > > > and > > > >>> > > having > > > >>> > > > > >>> the author making a deliberate effort to "mark the > > > >>> conversation > > > >>> > as > > > >>> > > > > >>> resolved" is a sign it's been read, though about and > > > >>> consciously > > > >>> > > > > >>> reacted to. > > > >>> > > > > >>> > > > >>> > > > > >>> I've learned recently that you can add protection rule > > that > > > >>> will > > > >>> > > > > >>> require all conversations on PR to be resolved before > > > merging > > > >>> > it, I > > > >>> > > > > >>> even went to a great length to create (and get merged) > a > > > PR to > > > >>> > ASF > > > >>> > > > > >>> infra to enable it via .asf.yml feature > > > >>> > > > > >>> (https://github.com/apache/infrastructure-p6/pull/1740 > ) > > - > > > so > > > >>> we > > > >>> > > can > > > >>> > > > > >>> enable it now by a simple PR to our .asf.yaml enabling > > it. > > > >>> > > > > >>> > > > >>> > > > > >>> I'd love to try it - but of course it will have to > > change > > > a > > > >>> bit > > > >>> > > the > > > >>> > > > > >>> workflow of everyone, where the author (or reviewer, or > > > >>> > maintainer) > > > >>> > > > > >>> will have to mark all conversations as resolved > > > deliberately > > > >>> > before > > > >>> > > > > >>> merging PR. > > > >>> > > > > >>> > > > >>> > > > > >>> I'd love to enable it - at least to try and see how it > > can > > > >>> work - > > > >>> > > but > > > >>> > > > > >>> I understand it might add a bit of burden for everyone, > > > >>> however, > > > >>> > I > > > >>> > > > > >>> think it might be worth it. > > > >>> > > > > >>> > > > >>> > > > > >>> WDYT? > > > >>> > > > > >>> > > > >>> > > > > >>> J. > > > >>> > > > > >>> > > > >>> > > > > >>> > > > >>> > > > > > --------------------------------------------------------------------- > > > >>> > > > > >>> To unsubscribe, e-mail: > > dev-unsubscr...@airflow.apache.org > > > >>> > > > > >>> For additional commands, e-mail: > > > dev-h...@airflow.apache.org > > > >>> > > > > >>> > > > >>> > > > > >>> > > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > --------------------------------------------------------------------- > > > >>> > > > > > To unsubscribe, e-mail: > dev-unsubscr...@airflow.apache.org > > > >>> > > > > > For additional commands, e-mail: > > dev-h...@airflow.apache.org > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > --------------------------------------------------------------------- > > > >>> > > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > > >>> > > > > For additional commands, e-mail: > dev-h...@airflow.apache.org > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > >>> > --------------------------------------------------------------------- > > > >>> > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > > >>> > > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > >>> > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >> > > > > > >
