search

OCA / oca-github-bot

The GitHub Bot of the Odoo Community Association (OCA)
MIT License
42 stars 60 forks source link

[IMP] Search addons maintainers in other branches. Fix : #122 #183

Closed legalsylvain closed 2 years ago

legalsylvain commented 2 years ago

Search for maintainers in all the branches, to make possible maintainer to merge migration PRs. (#122) Note, it works also for the past. (If i'm declared as maintainers of a 12.0 module, I will also have the possibility to merge PR against 8.0 (for backport / fixes / etc...)

Deployment Note Once deployed, you should update your environment file. if not, the feature will be simply anavailable.

sbidoul commented 2 years ago

I think we should do this only for migration PRs, i.e. when the addon is not yet on the target branch. Otherwise, this may have security implications, because a maintainers on a branch may not be the same as on other branches.

legalsylvain commented 2 years ago

I think we should do this only for migration PRs, i.e. when the addon is not yet on the target branch. Otherwise, this may have security implications, because a maintainers on a branch may not be the same as on other branches.

Good question I asked myself last night !

Well, in fact, when I adopt an addon, I adopt it most of the time "globally". = "I'd like to have the possibility to merge bugfixes, and backport of bugfixes in older version".

Making a PR to add the key in the manifest file is a workflow that can be slow in some repo without a lot of reviewers, but force me to make a PR against all the branches the module is available will make the workflow slower.

typical use case : I adopt a module in V12, (making a PR). The PR is merged, but in the meantime, the port of this module has been done for V14. I'm so not the maintainer of the V14 module and I have to make another PR to be set as maintainer. but maybe a V15 PR is pending ! and I'll have to make a third PR. Finally, when i'll make the port of the module in V16 (that is the next release I target) I'll not have the possibility to merge the PR.

because a maintainers on a branch may not be the same as on other branches.

Is it a real use case ? I mean : "module in V12 maintained by @sbidoul + module in V15 maintained by @legalsylvain" 1) I'm not sure that this case exists. 2) But even if it were the case, do we really want to make sure that you are not allowed to merge a patch in V16 ?

So, After these nocturnal reflections, my point of view was: 1) Be optimistic ! if we can make bugfix / migration workflow quicker, it's great ! And if abuse is observed, we can always change the bot's behavior very quickly. at one time when I was on the board, I had the rights to all the repos in the OCA, but still, I didn't throw any ``ocabot merge'' into repos/modules that I didn't understand. 2) I think that fundamentally, the problem lies elsewhere and should be managed more globally. Over time, people declared as maintainers of OCA modules will leave. And so, their account will be able to modify OCA code forever, which can be a more global problem.

legalsylvain commented 2 years ago

Hello @sbidoul ! Do you keep your point of view ? I'd like to move forward with that PR. I can change the algorithm if it's a blocking point. Let me know !

sbidoul commented 2 years ago

Sorry I had lost track of this. I think you are right. If and when we have this kind of conflicts between maintainers in different branches, we can solve that by other means. So +1 with your approach.

legalsylvain commented 2 years ago

Next try : https://github.com/OCA/oca-github-bot/pull/183/commits/744b57d35a47f44772ff0f083dbad24f37fe9569 ;-)

sbidoul commented 2 years ago

:+1: If it's ready for you I can deploy to the OCA server tomorrow.

legalsylvain commented 2 years ago

+1 If it's ready for you I can deploy to the OCA server tomorrow.

Yes ! ready from my part.

when deploying don't forget to add MAINTAINER_CHECK_ODOO_RELEASES=8.0,9.0,10.0,11.0,12.0,13.0,14.0,15.0 in the environment file otherwise, the feature will be disabled.

Please ping me if troubles.

legalsylvain commented 2 years ago

+1 If it's ready for you I can deploy to the OCA server tomorrow.

Hi. did you deployed the feature ?

Just to know if https://github.com/OCA/product-attribute/pull/1083#issuecomment-1137313667 is normal.

sbidoul commented 2 years ago

No, it was a sunny week-end :) I'll do it in the next days.

legalsylvain commented 2 years ago

No, it was a sunny week-end :) I'll do it in the next days.

hum... Next week-end will be sunny also ! ;-)

Let me know when it's done.

regards.

sbidoul commented 2 years ago

Hi @legalsylvain. It is deployed. I'll let you test and announce :)

legalsylvain commented 2 years ago

Hi @sbidoul. Seems to work :

https://github.com/OCA/product-attribute/pull/1083#issuecomment-1140407144

Thanks !

sbidoul commented 2 years ago

Excellent. Thanks for the contrib!

sbidoul commented 2 years ago

Can you also try merging something you are not allowed to ?

eLBati commented 2 years ago

@legalsylvain Hi, following https://github.com/OCA/oca-github-bot/commit/108c9aea1a1a3a1055f1513ddfdb37e2fe1d7d0f#r83501261 , we would need to grant some users the ability to merge PRs only for a specific (old) version, as they have competence in that version and PSC members do not work anymore on that (old) version

legalsylvain commented 2 years ago

Hi @elbati. Thanks for your message.

However : I understand what you ask, but I dont understand the problem you try to fix. Do you face some abuses, people that merge PR that should not be merged ?

Thanks for your precisions.

TheMule71 commented 2 years ago

I dont understand the problem you try to fix.

Hi there! Thank you for your answer. Please allow me to clarify a bit more.

There's no problem to fix. It's just a feature request.

Someone volunteered to maintain a very old branch (8.0), but they have clearly stated they are not interested in moving on from that, and they're not going to contribute anything to more recent branches. We would like to grant them maintainer status for all modules in the 8.0 branch only.

Currently, that would imply granting maintainer status for all modules in other branches too, something completely unnecessary (per their request). We feel that limiting permissions to the minimun required is best practice anyway.

So we're wondering if there's a way to override the environtment variable MAINTAINER_CHECK_ODOO_RELEASES=8.0,9.0,10.0,11.0,12.0,13.0,14.0,15.0 and set it locally (for our repo) to something along the line of MAINTAINER_CHECK_ODOO_RELEASES=10.0,11.0,12.0,13.0,14.0,15.0.

Regarding https://github.com/OCA/oca-github-bot/pull/183#issuecomment-1099157614 we don't need module granularity at all. Excluding maintainers specified in the 8.0 branch as a whole would be fine for us. We don't need extra complexity, just a way to override the MAINTAINER_CHECK_ODOO_RELEASES environment variable. (Apologies if there's a documented way of doing that, if so it went completely over our heads).