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