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