I think it's a fine idea to experient with to see if the advantages outweigh the extra friction. I do have a question and forgive my ignorance, but this I assume isn't restricting a commit author from resolving the comments in cases where they either think it doesn't need fixing or if it's perhaps something for a follow-up pr? What I mean is it isn't gonna enforce who can resolve a comment thread?
-- Regards, Aritra Basu On Wed, Dec 20, 2023, 4:09 PM Jarek Potiuk <ja...@potiuk.com> wrote: > If there are no more big objections, I will enable it after the 26th of > December to see how it can work. > > This will also be a naturally slower period due to Holidays and New Year's > eve, so even if it disrupts someone initially it will have a smaller radius > blast potentially. I will also make sure to communicate it in a few of our > channels that we are experimenting with and be on the lookout to help > people if they will have problems with it. > > J. > > > On Wed, Dec 20, 2023 at 8:31 AM Jarek Potiuk <ja...@potiuk.com> wrote: > > > Amogh: > > > > > 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. > > > > I think we already do that in a number of cases. And I would not enforce > > it, especially that (unlike "do not merge until resolved") it's not > > automatable. Here I would actually say what Ash said about being "adult". > > The whole idea is not because we do not trust people to do the right > thing > > - it's there to make sure that we do not forget about some open > > conversation (and that goes to both - author and reviewer). > > > > For the author, this will be a sign to see that there is "something" to > do > > when there are conversations that are still open. For the reviewer, it > > should also be a sign "I can merge it now, really" or "let's see if there > > is anything left to address here, I have not realised there is this one > > more comment not addressed". > > > > On Wed, Dec 20, 2023 at 5:16 AM Amogh Desai <amoghdesai....@gmail.com> > > wrote: > > > >> 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 > >> > > > >>> > > > > >> > > > >>> > > > > >> > > > >>> > > > >> > > > >>> > > >> > > > >>> > >> > > > >> > >> > > > > >> > > > >> > > >> > > >
