Agree with Elad On Fri, Dec 22, 2023 at 8:40 AM Elad Kalif <elad...@apache.org> wrote:
> I am not sure I understand what is the problem that we are trying to solve > here? > > I think it's the maintainer's responsibility to check if the PR is ready to > be merged (closed threads or open). > I actually sometimes consider leaving some of the threads open - especially > when referencing a followup in a new issue. > If all threads are folded It's a nightmare to find the comment that > referenced it in case you want to read the chain of events that lead to the > new issue. > > I am not against trying something new but I don't think we should introduce > a solution for an unknown problem(?) > > > On Fri, Dec 22, 2023 at 6:19 AM Amogh Desai <amoghdesai....@gmail.com> > wrote: > > > Thanks for your message, Jarek. > > > > We can try it out since year ends are low traffic periods, we get some > > experimental phase with this. > > > > Thanks & Regards, > > Amogh Desai > > > > On Wed, Dec 20, 2023 at 7:45 PM Aritra Basu <aritrabasu1...@gmail.com> > > wrote: > > > > > Thanks for the clarification Jarek, I see the value in it. It's a +1 > from > > > me to try it out. > > > > > > -- > > > Regards, > > > Aritra Basu > > > > > > On Wed, Dec 20, 2023, 7:04 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > > > Aritra: > > > > > > > > > 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? > > > > > > > > Not at all. Just to repeat from the earlier answer: > > > > > > > > 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. > > > > > > > > I do not think we need (and that was not a suggestion) to add > > > > more bureaucracy or process around it. It's purely a gatekeeper to > make > > > > sure that all the comments are "marked as resolved" before merging > the > > > PR. > > > > > > > > Maybe that's not clear - but in my proposal, I do not want to decide, > > nor > > > > mandate whether it's the author, reviewer or even another maintainer > > who > > > > decides to merge the PR who closes the conversation. This is really > the > > > > part of "adult trust" that I was talking about. > > > > > > > > As I see it, we are really putting the trust in the hands of those > who > > > mark > > > > the comment as resolved (whether it's author, reviewer, or person who > > > > merges it). And we trust them that their intention is "yes, I believe > > > that > > > > conversation is resolved". > > > > We still have other ways when we really want to block things before > we > > > > re-verify them as maintainers (i.e. "request changes") and this > should > > be > > > > in our toolbox as well. And we should use it when needed of course. > > > > > > > > But what I really like in this case is that: > > > > > > > > a) it allows bystanders, non-committers to have an opinion there > which > > is > > > > non-blocking but eventually requires some reaction from someone - > > author > > > or > > > > maintainer (even if it is - "nope look up, it's explained above" or > > even > > > > just "resolving" without comment in some obvious cases. Yes, there > is a > > > > small risk of slowing things down/spamming but we can always contain > > that > > > > when it happens. We have tools for that. > > > > > > > > b) I think it will result (and this is also what the experience of > > > Sourcery > > > > creators in the podcast I mentioned above is) results with less > number > > of > > > > stale "request changes" and people feeling more empowered and trusted > > to > > > do > > > > the right things. It's also more difficult for people - because they > > have > > > > to do something with the trust that is given to them. But I think > it's > > > the > > > > right thing to do and eventually might result in faster turnaround on > > PRs > > > > (or at least that's one the mechanisms described in the Podcast as > > > "working > > > > this way" eventually. > > > > > > > > I really recommend listening to the podcast - > > > > https://realpython.com/podcasts/rpp/183/ - while I do not > necessarily > > > > agree with everything there, this one got stuck in my head when I > > thought > > > > that it might, indeed, work as described. > > > > > > > > J. > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 2:05 PM Aritra Basu < > aritrabasu1...@gmail.com> > > > > wrote: > > > > > > > > > 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 > > > > > > >> > > > >>> > > > > > > > > > >> > > > >>> > > > > > > > > > >> > > > >>> > > > > > > > > >> > > > >>> > > > > > > > >> > > > >>> > > > > > > >> > > > >> > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
