Alright, I have a typo I was going to fix anyway so I tried submitting it using `--author "Boring Cyborg <kaxiln...@apache.org>"`. If it doesn't do what we planned we may need another email address, but it's a simple typo fix I was going to submit anyway so no harm done if it doesn't work.
https://github.com/apache/airflow/pull/47038 - ferruzzi ________________________________ From: Ferruzzi, Dennis <ferru...@amazon.com.INVALID> Sent: Monday, February 24, 2025 12:23 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. I'm just looking for an email address to attribute it to and I'll throw a typo fix up using that method and w can see. I just learned Boring Cyborg is /our/ Kaxil. :facepalm: I probably should have known that by now. - ferruzzi ________________________________ From: Jarek Potiuk <ja...@potiuk.com> Sent: Monday, February 24, 2025 12:18 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. 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 > >> > > >> > > >> > > >
