Thanks for your input Jarek, and thanks Dennis for the PR. I'd wait on Kaxil as it's not super urgent.
I would only be able to try this out over the weekend but I'd love to give those ideas a try. Thanks & Regards, Amogh Desai On Tue, Feb 25, 2025 at 2:01 AM Ferruzzi, Dennis <ferru...@amazon.com.invalid> wrote: > Alright, I have a typo I was going to fix anyway so I tried submitting it > using `--author "Boring Cyborg <kaxiln...@apache.org>"`. If it doesn't > do what we planned we may need another email address, but it's a simple > typo fix I was going to submit anyway so no harm done if it doesn't work. > > https://github.com/apache/airflow/pull/47038 > > > - ferruzzi > > > ________________________________ > From: Ferruzzi, Dennis <ferru...@amazon.com.INVALID> > Sent: Monday, February 24, 2025 12:23 PM > To: dev@airflow.apache.org > Subject: RE: [EXT] [DISCUSS] Proposal to enhance the PR template > > 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. > > > > I'm just looking for an email address to attribute it to and I'll throw a > typo fix up using that method and w can see. I just learned Boring Cyborg > is /our/ Kaxil. :facepalm: I probably should have known that by now. > > > - ferruzzi > > > ________________________________ > From: Jarek Potiuk <ja...@potiuk.com> > Sent: Monday, February 24, 2025 12:18 PM > To: dev@airflow.apache.org > Subject: RE: [EXT] [DISCUSS] Proposal to enhance the PR template > > 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. > > > > Good idea ! worth trying :) > > On Mon, Feb 24, 2025 at 9:17 PM Ferruzzi, Dennis > <ferru...@amazon.com.invalid> wrote: > > > Would it work if we just gave Boring Cyborg a co-author credit on a > commit > > once? Or does it strictly need to be committed in "their" name? The > > former is easy, the latter might be tricky. > > > > We could try `git commit --author` to give them (co-?)author credit. I'm > > not sure if that counts for what we need here, but the git docs > > specifically say "override the commit author", so it might work for this: > > > > ``` > > > > --author=<author> > > > > Override the commit author. Specify an explicit author using the standard > > A U Thor <aut...@example.com> format. Otherwise <author> is assumed to > be > > a pattern and is used to search for an existing commit by that author > (i.e. > > rev-list --all -i --author=<author>); the commit author is then copied > from > > the first such commit found. > > > > ``` > > > > > > > > > > - ferruzzi > > > > > > ________________________________ > > From: Jarek Potiuk <ja...@potiuk.com> > > Sent: Sunday, February 23, 2025 11:43 PM > > To: dev@airflow.apache.org > > Subject: RE: [EXT] [DISCUSS] Proposal to enhance the PR template > > > > 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. > > > > > > > > One small thing though.. Currently boring cyborg usefulness is limited > > because we have to manually "approve the workflows" that boring cyborg > > runs. So might be a good opportunity to look how we can fix it, the > problem > > is that the boring cyborg is seen as a user who never committed anything > to > > our repo, and all workflows generated by the cyborg have to be manually > > approved. So either we somehow make a PR and merge it for the boring > cyborg > > (could be possible actually) - or figure a better way of applying the > > labels. > > > > On Mon, Feb 24, 2025 at 8:40 AM Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > > based on what has changed. I need to take a look at how this can be > > done. > > > > > > This should be very easy with boring-cyborg. What we **really** need to > > > make sure though is that our code is structured in the way that when we > > > select "paths" it nicely identifies the "impact". And rather than > trying > > to > > > fit-in the selection, we can also "change the reality" and restructure > > our > > > code a bit to make it easier. > > > > > > That was one of the things that moving providers allowed - previously, > > > when we wanted to apply "provider-amazon" label (for example) - we had > to > > > specify I think 5 different paths and they were all different. What we > > have > > > now is "providers/amazon/**" - because all the amazon provider files > are > > > there. > > > > > > J. > > > > > > > > > On Mon, Feb 24, 2025 at 8:35 AM Amogh Desai <amoghdesai....@gmail.com> > > > wrote: > > > > > >> Thanks all for your responses. > > >> > > >> > Perhaps instead of a checkbox we could auto-add a label that > indicates > > >> which API is being touched, based on which subfolder of api_fastapi. > > This > > >> might be a better way to help meet your aim of helping with tracking > > down > > >> changes, that isn't reliant on humans checking a box (and doesn't add > > the > > >> noise) that people will likely generally ignore. > > >> > > >> Yeah, this seems like a pretty good idea. Let me try and investigate a > > bit > > >> on this front. > > >> > > >> Thanks for the idea Daniel and Jarek. So to summarise so far: > > >> > > >> - Instead of adding a checkbox to the PR template, we could do better > by > > >> adding some "labels" to the PR > > >> based on what has changed. I need to take a look at how this can be > > done. > > >> > > >> Thanks & Regards, > > >> Amogh Desai > > >> > > >> > > >> On Sun, Feb 23, 2025 at 10:36 AM Wei Lee <weilee...@gmail.com> wrote: > > >> > > >> > Yep, I also think better labeling (and other CI checks) would be a > > >> better > > >> > choice! (if we could and have the bandwidth to do so) Checkboxes > often > > >> get > > >> > overlooked, and might need some time to teach all contributors. Take > > the > > >> > significant template as an example, I still need to check new > > >> newsfragments > > >> > from time to time. > > >> > > > >> > Best, > > >> > Wei > > >> > > > >> > > On Feb 23, 2025, at 1:11 AM, Jarek Potiuk <ja...@potiuk.com> > wrote: > > >> > > > > >> > > I like much more "fixing" (or upgrading) the labelling scheme > rather > > >> than > > >> > > checkbox. With changes like that we have to be careful to impact > > >> > experience > > >> > > of everyone - while improving some "failure" scenario, you also > make > > >> live > > >> > > of everyone who raises PR and has to learn about those checkboxes, > > >> know > > >> > > what they mean, do not be confused and more importantly, you have > to > > >> > teach > > >> > > **every, single, person** who might submit a fast api change when > to > > >> tick > > >> > > the box (and continue teaching all new contributors). > > >> > > > > >> > > Automating it - by setting the right label based on which files > are > > >> > changed > > >> > > in a more granular way seems like a much better approach - because > > >> then > > >> > you > > >> > > only need to teach reviewers what to do and how to react (and > there > > >> is a > > >> > > much smaller group of those to teach). > > >> > > > > >> > > BTW. That's why I love that in PEP's of Python there is always a > > >> section > > >> > > "how to teach this?". It's often easy to describe the "wishful" > > >> > behaviour, > > >> > > but if it involves teaching a lot of people what to do, the task > > >> suddenly > > >> > > becomes much more complex. > > >> > > > > >> > > J. > > >> > > > > >> > > On Sat, Feb 22, 2025 at 4:40 PM Daniel Standish > > >> > > <daniel.stand...@astronomer.io.invalid> wrote: > > >> > > > > >> > >> RE > > >> > >>> I think this small change would significantly improve the > > >> confidence in > > >> > >> code reviewers during review and also make it easier to track > down > > >> > issues > > >> > >> if they arise at a later stage. > > >> > >> > > >> > >> So, expanding... any time you see a change to > airflow/api_fastapi, > > >> you > > >> > are > > >> > >> probably making changes to "the interface". Particularly if you > > see > > >> > >> changes to `v1-generated.yaml`. > > >> > >> > > >> > >> We have multiple APIs within that folder -- some of which are > > public > > >> and > > >> > >> others which are internal. And then of course there are various > > >> bits of > > >> > >> airflow code that are public interface and others that are not. > > >> > >> > > >> > >> Perhaps instead of a checkbox we could auto-add a label that > > >> indicates > > >> > >> which API is being touched, based on which subfolder of > > api_fastapi. > > >> > This > > >> > >> might be a better way to help meet your aim of helping with > > tracking > > >> > down > > >> > >> changes, that isn't reliant on humans checking a box (and doesn't > > add > > >> > the > > >> > >> noise) that people will likely generally ignore. > > >> > >> > > >> > >> > > >> > >> > > >> > >> > > >> > >> > > >> > >> > > >> > >> On Fri, Feb 21, 2025 at 11:07 AM Daniel Standish < > > >> > >> daniel.stand...@astronomer.io> wrote: > > >> > >> > > >> > >>> It feels more like a failure of CI / testing than a failure of > PR > > >> > >>> description. > > >> > >>> > > >> > >>> Is an author supposed to exercise all APIs for every "public > > >> interface" > > >> > >>> change? > > >> > >>> > > >> > >>> On Fri, Feb 21, 2025 at 8:52 AM Ankit Chaurasia < > > >> sunank...@gmail.com> > > >> > >>> wrote: > > >> > >>> > > >> > >>>> Hi Amogh, > > >> > >>>> > > >> > >>>> Great idea! I agree with adding a checkbox for public interface > > >> > changes > > >> > >> in > > >> > >>>> the PR template. Additionally, I suggest we add a brief section > > >> where > > >> > >>>> contributors can note any potential side effects or impacts if > > >> known. > > >> > >> This > > >> > >>>> extra step could help reviewers catch issues early. > > >> > >>>> > > >> > >>>> Additionally, the API label was not automatically added to this > > PR. > > >> > >>>> > > >> > >>>> Regards, > > >> > >>>> *Ankit Chaurasia* > > >> > >>>> HomePage <https://ankitchaurasia.info/> | LinkedIn > > >> > >>>> <https://www.linkedin.com/in/sunank200/> > > >> > >>>> > > >> > >>>> > > >> > >>>> > > >> > >>>> > > >> > >>>> > > >> > >>>> > > >> > >>>> On Fri, Feb 21, 2025 at 12:10 PM Amogh Desai < > > >> amoghde...@apache.org> > > >> > >>>> wrote: > > >> > >>>> > > >> > >>>>> Hello Everyone, > > >> > >>>>> > > >> > >>>>> I have noticed an increased number of PRs introducing changes > to > > >> the > > >> > >>>> public > > >> > >>>>> interfaces of Airflow (UI / API / CLI) do not provide > sufficient > > >> > >>>>> information / evidence about their working, either in form of > > >> > >>>> screenshots > > >> > >>>>> (for UI mainly) and/or API responses. > > >> > >>>>> > > >> > >>>>> For example, a recent PR merge: > > >> > >>>>> https://github.com/apache/airflow/pull/46666, > > >> > >>>>> while not showing any apparent side effects during code > review, > > >> ended > > >> > >> up > > >> > >>>>> breaking a ton of UI and API and as a result, follow up issues > > and > > >> > PRs > > >> > >>>> had > > >> > >>>>> to be created to fix it. > > >> > >>>>> > > >> > >>>>> To minimize such occurrences, I propose we slightly modify the > > PR > > >> > >>>> template > > >> > >>>>> to include a checkbox that could have content along the lines > > of: > > >> > >>>>> *- My PR introduces a public interface change (UI/API), and I > > have > > >> > >> added > > >> > >>>>> screenshots and/or API responses before and after my change.* > > >> > >>>>> > > >> > >>>>> I think this small change would significantly improve the > > >> confidence > > >> > >> in > > >> > >>>>> code reviewers during review and also make it easier to track > > down > > >> > >>>> issues > > >> > >>>>> if they arise at a later stage. > > >> > >>>>> > > >> > >>>>> I personally believe that this would enhance our review > process > > >> and > > >> > >>>> reduce > > >> > >>>>> such occurrences. > > >> > >>>>> Interested to hear your opinions and other suggestions that > you > > >> may > > >> > >>>> have. > > >> > >>>>> > > >> > >>>>> Thanks, > > >> > >>>>> Amogh > > >> > >>>>> > > >> > >>>> > > >> > >>> > > >> > >> > > >> > > > >> > > > >> > > --------------------------------------------------------------------- > > >> > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > >> > For additional commands, e-mail: dev-h...@airflow.apache.org > > >> > > > >> > > > >> > > > > > >
