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