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