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
