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