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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> 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 <[email protected]> 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 [email protected]. >>>>> >>>>> 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 < >>>>> [email protected]> 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 >>>>>> >>>>>
