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