I like your definition of "stewardship" in this case and I think it's in line with what I had in mind.
________________________________ From: Jarek Potiuk <ja...@potiuk.com> Sent: Monday, April 11, 2022 11:34 AM To: dev@airflow.apache.org Subject: RE: [EXTERNAL] Code ownership over the provider's source code 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. Let's see how it plays out with GitHub. If this does not work, we could potentially extend boring cyborg to add some notifications or do something similar. I think until we get the answer (hopefully that it works) we should really discuss and decide as a community how we approach providers we have in the community and especially the "governance vs. stewardship". I think (see the other discussions) we are rather close to think about some kind of separation of providers from core - on a repository level. I think it's inevitable (and the time is ripe for it). But I do not yet want to start discussion on the "mechanics" of it before we solve some "governance vs. stewardship" approach and what we want to achieve and why. The technical decisions should follow. >From the discussion I see - so far from people at Amazon, Google who are >involved we need some way to help the stakeholders to manage code for "their" >services (on top of ASF expectations for code contributions and decision >making) * those "stakeholder" relevant people would like to get notified when there are relevant changes for their provider * they want to have a possibility to comment and state their preference/opinion/ideas on the changes (to make sure they are in line with the "stakeholder" strategy for services) * they cannot get more "rights" in the providers than others (unless they are committers which gives them same rights as other committers) * also they want to be able to help others who contribute to the provider (hence becoming reviewer automatically makes sense) - this might be actually a good road to becoming a committer (help others to contribute by doing reviews). * possibly they could commit some rules (in the form of pre-commit checks or some guidelines if they want to get some more consistency - specific for "their" provider) I think this is really what I'd call "stewardship" over parts of the code. And CODEOWNERS for me is really a bad name - it should really be CODESTEWARDS. Personally (but this is my opinion only) is that while Airflow Community (and formally the PMC/committers) must keep the "governance" of providers, it should be possible to more or less formally delegate "stewardship" in this case. Not to give them more formal "rights" but make them more "involved" with the part of the code. Happy to hear other comments here - maybe there are different perspectives on the 'governance" of those. J. On Mon, Apr 11, 2022 at 7:05 PM Oliveira, Niko <oniko...@amazon.com.invalid> wrote: > BTW. Seems that there is a problem that you STILL need write access to be > CODEOWNER (despite the updated documentation :) ) Sad, I was also very excited about this. It would make the workflow of keeping an eye on changes to code we "own" much easier. Fingers crossed it's a feature coming soon! ________________________________ From: Jarek Potiuk <ja...@potiuk.com<mailto:ja...@potiuk.com>> Sent: Monday, April 11, 2022 3:39 AM To: dev@airflow.apache.org<mailto:dev@airflow.apache.org> Subject: RE: [EXTERNAL] Code ownership over the provider's source code 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. BTW. Seems that there is a problem that you STILL need write access to be CODEOWNER (despite the updated documentation :) ) https://github.com/apache/airflow/pull/22903 I hope it's just "future documentation" :) and this feature is being released now (or maybe just the error message is outdated) .... I raised a support ticket for that to GitHub (you can't see it - but I put Elad and Bartłomiej on CC:) https://support.github.com/ticket/personal/0/1581140 Let's see! J. On Mon, Apr 11, 2022 at 12:29 PM Bartłomiej Hirsz <bartek.hi...@gmail.com<mailto:bartek.hi...@gmail.com>> wrote: Yes, you worded it perfectly - I don't intend to be the owner of the code (and in any way gatekeeping it) but merely improve how we're notifying on code changes (or assigning to code review). I listed my PR as an example but it was a question in general - where I see that there could be people interested to see that particular code is about to be changed by PR and would be easy to miss in big projects without automation such as codeowner file. In that particular case I would just point to the new AIP and help to migrate the PR (and I wouldn't be able to vetoed if of course, just provide review + help with comments). I love that part of OSS where others contribute to the code and we're all owners of it in a broad sense so I don't want to lose it :) pon., 11 kwi 2022 o 12:20 Jarek Potiuk <ja...@potiuk.com<mailto:ja...@potiuk.com>> napisał(a): I think you are both right and both wrong - Elad and Bartłomiej :) Elad is quite right when it comes to "gating" changes. You might get notified about a change and raise your concerns but the nature of ASF project is simple - committers decide whether a change is ready. Any commiter (and commiter only) can veto a code change if it is justified (https://www.apache.org/foundation/voting.html#votes-on-code-modification). The non-committer cannot veto a change. So you cannot (and will not have) decide on what can be merged or not if you are not a commiter. This is what commiter status gives - and it gives it for all code in the whole Apache project (and it cannot be - by definition only for parts of the code). There are no (and will never be) partial committers or "less access committers" for an Apache project. I am quite certain of it (as I discussed this very thing at the OSS Backstage in Berlin with a few ASF old-timers). So comitter is "all-or-nothing". However there is absolutely nothing wrong (and this is even encouraged) that non-committers review and provide opinions and helpful comments and raise concerns. But - and I believe this is what Bartek asked for - they have to be notified somehow about the change for relevant parts of the code they are interested in. I can imagine tracking "all changes" is impossible so having a nice tool to do it automatically is great. But also Bartłomiej - you are wrong that you have to wait for the code owner. Even being in CODEOWNER does not imply it. We don't do it even now when we have committers being CODEOWNERS - we do not wait for those CODEOWNERS who are defined for parts of the code when we merge stuff - not even for the core. Sometimes we reach out when we need someones opinion (if we know that this person is more knowledgeable in this area, but it's rarely is a "blocking" call. The ONLY real gate for merging the code is any committer's approval. We have a rule that two committers need to agree on "core" change additionally (but this is more of a gentleman's agreement, rather than enforced rule so far). And I think we cannot and should not change it. It would be nice to wait for an opinion from someone who is CODEOWNER, but this is just "nice to have". By the rules of ASF if you are not a committer, you cannot decide for the code to be merged nor veto it. You can give advice and have opinions and concerns but you cannot decide. And CODEOWNERS as of recently seems to allow that and make non-committers CODEOWNERS (which I really like actually). I think the name is a little misleading. CODEOWNER does not give you more rights when it comes to make code ready to be merged, or having a veto. It's merely "you are by default assigned to be a reviewer of that code". It does not mean (contrary to the name) that you are OWNER of that code. It means that you are a stakeholder who is automatically added as reviewer. That's it. No more, no less. And I think this is what you - Bartłomiej really want. BTW. If we want to 'enforce" some rules, then we can add an automated pre-commit for that. This is what we do all the time. And once committers approve this pre-commit it becomes a "rule" really. So if we want to have some "let's never do this and that in a certain part of the code" - adding pre-commit is the way to go. J. On Mon, Apr 11, 2022 at 11:34 AM Bartłomiej Hirsz <bartek.hi...@gmail.com<mailto:bartek.hi...@gmail.com>> wrote: > Committers can merge PRs as they see fit. We don't have to wait for a > specific code owner. I must disagree with you. I know how open source projects works (I'm the maintainer of some) but in Airflow the problem is different. We have multiple providers that are responsible for their code but we're allowing anyone with contribution access to accept changes without notifying the code owners. It often happened that some provider introduced a major change in operators (and updated existing code) but then someone raised new PR with some changes, without this major change and it was merged without notifying given provider. It will be resolved if we move to separate repositories but for now we need to solve this issue in another way. See answer from Jarek to see if it's actually possible. In my exact case we introduced a new way of writing system tests, I've raised the PR migrating X test to new design and after my PR (which is still open) there was another PR that also modified the same file but using old design (which was merged - I see it's because the person raising and approving are from the same company, which helped to fasten the process). It's not the matter of simple rebase - the other PR shouldn't be approved since it's not up to our (current) community standards. And it wouldn't be approved if we were notified about this - we're responsible for our tests but our tests were modified without informing us. So I agree that "Commiters can merge PRs when they see fit" but I think in a repository that big as Airflow we need to at least make sure that people who know the given code the best (for example provider) are notified of proposed changes. pon., 11 kwi 2022 o 10:46 Elad Kalif <elad...@apache.org<mailto:elad...@apache.org>> napisał(a): Hi Bartłomiej, Due to the nature of the project multiple authors can work on the same files and not even know one to another. GitHub doesn't offer a feature to let the PR author know that there are other open PRs requesting to change the same file. In general this is not "Priority to who opens first". It's "Priority for what is ready". I feel your frustration. We all have been there. We all had to rebase and resolve conflicts caused by other PRs. I don't think being a code owner is the solution here. Committers can merge PRs as they see fit. We don't have to wait for a specific code owner. On Mon, Apr 11, 2022 at 11:41 AM Jarek Potiuk <ja...@potiuk.com<mailto:ja...@potiuk.com>> wrote: This is a very good point :) and has surprising (at least to me) answer: We have two mechanisms now: * Labels by Boring Cyborg (not very helpful as you cannot selectively subscribe to a label): https://github.com/apache/airflow/blob/main/.github/boring-cyborg.yml * CODEOWNERS feature (this one is precisely what you want, because you are marked as reviewer and notified as CODEOWNER): https://github.com/apache/airflow/blob/main/.github/CODEOWNERS The problem with CODEOWNERS is (or rather was(!) ) that you could only add maintainers as CODEOWNERS. Up until recently the CODEOWNER documentation stated this: > The people you choose as code owners must have write permissions for the > repository. When the code owner is a team, that team must have write > permissions, even if all the individual members of the team already have > write permissions directly, through organization membership, or through > another team membership. However it seems that GitHub silently changed it :) and now it's a bit different: > Users must have read access to the repository and teams must have explicit > write access, even if the team's members already have access. You can also > refer to a user by an email address that has been added to their account on > GitHub.com, for example u...@example.com<mailto:u...@example.com>. So - seems you can become a CODEOWNER even if you are not a maintainer!!!! YAY!. Something we really wanted to add for providers for a long time! I think you can achieve PRECISELY what you want - just make a PR to CODEOWNERS and add yourself to all Google Provider paths and we can test it. J. On Mon, Apr 11, 2022 at 10:07 AM Bartłomiej Hirsz <bartek.hi...@gmail.com<mailto:bartek.hi...@gmail.com>> wrote: Hi, Do we have any mechanism in Airflow to be notified on PRs touching provider code hosted in the Airflow repository? I think that's not the case - and it's related to recent discussions about providers in the core repository (on dev list). I've had some problems in the past where I discovered that there were PRs that changed behaviour of our provider and it would be useful to be noticed on such PRs. For example I've raised PR migrating Google GCS system tests to new design: https://github.com/apache/airflow/pull/22778 (5 days ago) Then someone raised PR extending the same files, but using old design: https://github.com/apache/airflow/pull/22808 (4 days ago) The latter is already merged - now causing headache if it comes to properly merging the changes. It could be avoided if there would be a code owner set per files. Is it possible in Airflow? I know there is an ownership file but previously only commiters (people with write access) could be added. Best regards, Bartłomiej Hirsz
