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

legalsylvain · 2022-04-13T23:49:45Z

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 · 2022-04-14T10:57:29Z

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 · 2022-04-14T12:56:25Z

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"

I'm not sure that this case exists.
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:

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.
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 · 2022-05-09T11:02:49Z

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 · 2022-05-09T13:09:49Z

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.

sbidoul

LGTM, a couple of suggestions.

src/oca_github_bot/manifest.py

sbidoul · 2022-05-18T10:49:30Z

src/oca_github_bot/manifest.py

+                url, allow_redirects=True, headers={"Cache-Control": "no-cache"}
+            )
+            if r.ok:
+                manifest = ast.literal_eval(r.content.decode("utf-8"))


nit: it might be better to add a parse_manifest(manifest: bytes) function in manifest.py.

something like that ?
8d6f595

Co-authored-by: Stéphane Bidoul <[email protected]>

legalsylvain

hi @sbidoul. Thanks for your review. I accepted your 2 proposal.
another comment inline.

otherwise, LGTM.

legalsylvain · 2022-05-20T14:59:42Z

src/oca_github_bot/manifest.py

+                url, allow_redirects=True, headers={"Cache-Control": "no-cache"}
+            )
+            if r.ok:
+                manifest = ast.literal_eval(r.content.decode("utf-8"))


something like that ?
8d6f595

for more information, see https://pre-commit.ci

legalsylvain · 2022-05-20T15:30:09Z

Next try : 744b57d ;-)

sbidoul · 2022-05-20T15:32:25Z

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

legalsylvain · 2022-05-20T15:39:52Z

+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 · 2022-05-25T14:25:28Z

+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 OCA/product-attribute#1083 (comment) is normal.

sbidoul · 2022-05-25T14:27:42Z

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

legalsylvain · 2022-05-25T14:48:47Z

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 · 2022-05-29T07:44:06Z

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

legalsylvain · 2022-05-29T09:03:41Z

Hi @sbidoul. Seems to work :

OCA/product-attribute#1083 (comment)

Thanks !

sbidoul · 2022-05-29T09:07:51Z

Excellent. Thanks for the contrib!

sbidoul · 2022-05-29T09:10:00Z

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

eLBati · 2022-09-24T14:20:10Z

@legalsylvain Hi, following 108c9ae#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 · 2022-09-24T15:26:55Z

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 · 2022-09-27T18:46:08Z

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 #183 (comment) 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).

legalsylvain force-pushed the imp-search-maintainers-in-other-branches branch 8 times, most recently from 0512ef3 to 9751f9c Compare April 14, 2022 00:41

legalsylvain marked this pull request as ready for review April 14, 2022 00:47

sbidoul approved these changes May 18, 2022

View reviewed changes

legalsylvain and others added 2 commits May 20, 2022 16:46

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

108c9ae

[IMP] Apply suggestions from code review

99b7c2a

Co-authored-by: Stéphane Bidoul <[email protected]>

legalsylvain force-pushed the imp-search-maintainers-in-other-branches branch from e763a31 to 99b7c2a Compare May 20, 2022 14:46

legalsylvain commented May 20, 2022

View reviewed changes

[REF] add parse_manifest function

744b57d

legalsylvain force-pushed the imp-search-maintainers-in-other-branches branch from 1aa47e3 to 744b57d Compare May 20, 2022 15:29

[pre-commit.ci] auto fixes from pre-commit.com hooks

f73b541

for more information, see https://pre-commit.ci

sbidoul merged commit ea9e526 into OCA:master May 29, 2022

legalsylvain mentioned this pull request Jul 3, 2022

[IMP] make _set_lines_issue working on empty issue #190

Merged

legalsylvain mentioned this pull request Nov 23, 2022

Maintainer are not able to merge / rebase their modules when migrating #226

Open

SirAionTech mentioned this pull request Jan 15, 2024

[IMP][14.0] l10n_it_delivery_note_customer_code: add fields to view OCA/l10n-italy#3849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

legalsylvain commented Apr 13, 2022 •

edited

Loading

sbidoul commented Apr 14, 2022

legalsylvain commented Apr 14, 2022

legalsylvain commented May 9, 2022

sbidoul commented May 9, 2022

sbidoul left a comment

sbidoul May 18, 2022

legalsylvain May 20, 2022

legalsylvain left a comment

legalsylvain May 20, 2022

legalsylvain commented May 20, 2022

sbidoul commented May 20, 2022

legalsylvain commented May 20, 2022

legalsylvain commented May 25, 2022

sbidoul commented May 25, 2022

legalsylvain commented May 25, 2022

sbidoul commented May 29, 2022

legalsylvain commented May 29, 2022

sbidoul commented May 29, 2022

sbidoul commented May 29, 2022

eLBati commented Sep 24, 2022

legalsylvain commented Sep 24, 2022

TheMule71 commented Sep 27, 2022

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

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

Conversation

legalsylvain commented Apr 13, 2022 • edited Loading

sbidoul commented Apr 14, 2022

legalsylvain commented Apr 14, 2022

legalsylvain commented May 9, 2022

sbidoul commented May 9, 2022

sbidoul left a comment

Choose a reason for hiding this comment

sbidoul May 18, 2022

Choose a reason for hiding this comment

legalsylvain May 20, 2022

Choose a reason for hiding this comment

legalsylvain left a comment

Choose a reason for hiding this comment

legalsylvain May 20, 2022

Choose a reason for hiding this comment

legalsylvain commented May 20, 2022

sbidoul commented May 20, 2022

legalsylvain commented May 20, 2022

legalsylvain commented May 25, 2022

sbidoul commented May 25, 2022

legalsylvain commented May 25, 2022

sbidoul commented May 29, 2022

legalsylvain commented May 29, 2022

sbidoul commented May 29, 2022

sbidoul commented May 29, 2022

eLBati commented Sep 24, 2022

legalsylvain commented Sep 24, 2022

TheMule71 commented Sep 27, 2022

legalsylvain commented Apr 13, 2022 •

edited

Loading