So far, my only real down side is that sometimes discussions happen in comments (not in threaded code comments, but in one-off "add a comment" comments) which this doesn't capture and they are annoying to track. While that isn't an actual issue with this CI change, this change may possibly lead to complacency on making sure THOSE are all resolved as well. Not worth a -1, just an honest thought and a reminder to all of us (myself included) that we still need to check if those are resolved.
- ferruzzi ________________________________ From: Jarek Potiuk <ja...@potiuk.com> Sent: Tuesday, January 30, 2024 10:55 AM To: dev@airflow.apache.org Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [ANNOUNCE] Starting experimenting with "Require conversation resolution" setting 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. Glad to see such positive feedback :) . To be honest - I've been interacting with ~ 120 PR over the last few months and for me it was only positive. I had a few cases where I wanted to merge a PR and got "1 conversation unresolved" and it made me come back: Particularly this one https://github.com/apache/airflow/pull/36513 - with 68 (!) conversations where I got a lot of comments from Bas and I wanted to make sure I address all of them (and Github's interface hiding conversations if there are many) does not make it easy. There were quite a few I'd have missed if not the "2 unresolved conversations". I also had an exchange with Bolke on https://github.com/apache/airflow/pull/22253 - which was a very old PR, with past conversations, where I personally think having to come back and re-review the conversations that were not resolved from long past are - I believe at least - great refresher when you come back to a PR from long past. Definitely +1 from me. J. On Tue, Jan 30, 2024 at 7:43 PM Oliveira, Niko <oniko...@amazon.com.invalid> wrote: > Yeah, I've found this to be pretty smooth as well. In most cases comments > were already resolved and in lesser cases it was useful to see which > conversations still needed addressing before merging. +1 from me! > > ________________________________ > From: Ryan Hatter <ryan.hat...@astronomer.io.INVALID> > Sent: Tuesday, January 30, 2024 8:58:30 AM > To: dev@airflow.apache.org > Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [ANNOUNCE] Starting > experimenting with "Require conversation resolution" setting > > 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. > > > > In my experience outside of Airflow, the benefit of not missing a review > comment outweighs the friction of being required to resolve each > conversation. > > On Mon, Jan 29, 2024 at 8:47 PM Wei Lee <weilee...@gmail.com> wrote: > > > I didn't notice much of a difference as a contributor. +1 vote > > > > Best, > > Wei > > > > > On Jan 30, 2024, at 11:41 AM, Amogh Desai <amoghdesai....@gmail.com> > > wrote: > > > > > > Contrary to my initial expectation of the trouble this would bring in > for > > > reviewers, it has been > > > pretty nice. I have not faced any issues in marking the conversations > as > > > resolved for the pull > > > requests I have reviewed and it has even given me a chance to re review > > > prior to approval. > > > > > > I am happy with this overall and my vote will be a +1 > > > > > > Thanks & Regards, > > > Amogh Desai > > > > > > On Mon, Jan 29, 2024 at 7:56 PM Aritra Basu <aritrabasu1...@gmail.com> > > > wrote: > > > > > >> I personally haven't had too much friction due to the change and it > has > > >> helped me keep track of any comments people have made. I remain +1 to > > the > > >> change so far. > > >> > > >> -- > > >> Regards, > > >> Aritra Basu > > >> > > >> On Mon, Jan 29, 2024, 6:11 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > >> > > >>> Just wanted to remind everyone, we are nearing the end of the trial > > >> period > > >>> for "require conversation" feature to be enabled. I have my own > > >>> observations and examples, but since I was the one to propose it, I > am > > >>> likely biased, so I'd love to hear from others what their feedback > and > > >>> assessment is. Or maybe we need more time to assess it ? > > >>> > > >>> I would love to hear your thoughts. > > >>> > > >>> J, > > >>> > > >>> > > >>> On Sat, Dec 30, 2023 at 2:20 PM Jarek Potiuk <ja...@potiuk.com> > wrote: > > >>> > > >>>> After an initial indentation problem in .asf.yaml it's not working > as > > >>>> expected. So .... let's see how resolving conversations will work > for > > >> us. > > >>>> > > >>>> On Sat, Dec 30, 2023 at 12:17 PM Amogh Desai < > > amoghdesai....@gmail.com > > >>> > > >>>> wrote: > > >>>> > > >>>>> Wooho! Looking to see how this turns out for airflow 😃 > > >>>>> > > >>>>> On Sat, 30 Dec 2023 at 1:35 PM, Jarek Potiuk <ja...@potiuk.com> > > >> wrote: > > >>>>> > > >>>>>> Hello everyone, > > >>>>>> > > >>>>>> As discussed in > > >>>>>> https://lists.apache.org/thread/cs6mcvpn2lk9w2p4oz43t20z3fg5nl7l > I > > >>> just > > >>>>>> enabled "require conversation resolution" for our main/stable > > >>> branches. > > >>>>> We > > >>>>>> have not used it in the past so it might not work as we think or > we > > >>>>> might > > >>>>>> need to tweak something. > > >>>>>> > > >>>>>> Generally speaking (if all works) all conversations on PRs should > be > > >>>>>> resolved before we can merge the PR. This "resolving" is > encouraged > > >> to > > >>>>> be > > >>>>>> done by the author when they think the conversation is resolved, > but > > >>> it > > >>>>> can > > >>>>>> also be done by reviewers or the maintainer who wants to merge the > > >> PR. > > >>>>>> > > >>>>>> We attempted to describe some basic rules and expectations here: > > >>>>>> > > >>>>>> > > >>>>> > > >>> > > >> > > > https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#step-5-pass-pr-review > > >>>>>> but undoubtedly there will be questions and issues that we might > > >> want > > >>> to > > >>>>>> solve - so feel free to discuss it here or raise question/issues > in > > >>>>>> #development channel in slack (I am also happy to be pinged > directly > > >>>>> about > > >>>>>> it and help to resolve any issues/gather feedback). > > >>>>>> > > >>>>>> J. > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > >
