Hey Elad, Daniek,
I understand your concerns, and yes I agree visibility of "resolved"
threads is lower. However I also believe that they are visible "enough"
(i.e. if you review a code you see there IS a conversation and you can
always unfold it) and having them solved might actually lead to more
engagement from people who want to actively contribute..
> 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.
Not all - only conversations that relate to specific lines of code, - and
since they are considered as solved I personally think this is better,
because then you can focus on the "code" first - not on other's comments.
In a way it also nicely solves another problem - which is "suggesting your
thinking by looking at other's comments that were resolved already". I
personally - when I review the code - prefer to see that there is a part
that led to comment and guess what it could be - and only then open it and
see what others commented on. I believe it might lead to more "independent"
comments.
Also I think that if the conversation was "important enough" - it should
find its way to the code or commit message - this also might be the side
effect - "resolving" the conversation should mean that either it's not as
important, or that we think it's been addressed - which - means that the
code in PR should already answer the question raised in the conversation,
so the conversation might be hidden (and revealed when more context is
needed). In a way I think "resolving" conversation is a way to transition
from a more detailed context that needs more discussion, to actual outcome
- i.e. change in code or commit message that answers the problem raised in
the conversation.
So - my assumption here is that when conversation is resolved - the
conversation itself is only needed as "background context" because the PR
already has the answers. So hiding it by default is "appropriate" (while
being able to reveal it easily is also important).
I think that approach mostly makes it easier for the author, reviewer and
that one who merges the code to make sure the conversations have been
resolved. Yes - it makes it more difficult by a bystander to see all the
details - and they need to be more active to see them - by opening the
conversation. And I think it's good. We want to make it generally more
"robust" for author, reviewer, and merging person - not to someone who just
looks and is not active in the conversation. This promotes IMHO being more
active and engaged in general.
Re: all the other things I believe it solves, let me repeat what I wrote
already. I think those are "real" things that this approach improves:
------
The three things I see will be improved.
1) the person merging the PR will not merge PRs when conversations are
still unresolved manually. Some of the PRS have long conversations and
I had many times - for example - cases where I missed that someone
made a comment or question which was important to raise but not
important enough to mark PR as "request changes".
2) the PR author will see clearly that the PR is not ready to merge if
there are unresolved conversations and will have to make an action to
make the PR mergeable - without anyone who comes and takes a look at
the PR and says ("Hey what do you think ? Is your problem solved?") It
will be clearly on the author to make it "ready" for the person who is
merging the PR.
3) it turns "starting conversation" into "soft request for changes".
There are many PRs where committers mark things as "request for
changes" and then they do not realise their comments have been
addressed. There are many, many cases where such PRs are "hanging"
without taking the "request for changes" off and it would be as easy
as "resolve comment".
On Fri, Dec 22, 2023 at 7:59 PM Daniel Standish
<daniel.stand...@astronomer.io.invalid> wrote:
> 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
> > > > > > > >> > > > >>> > > >
> > > > > > > >> > > > >>> > > >
> > > > > > > >> > > > >>> > >
> > > > > > > >> > > > >>> >
> > > > > > > >> > > > >>>
> > > > > > > >> > > > >>
> > > > > > > >> > > >
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
