OpenUpgrade icon indicating copy to clipboard operation
OpenUpgrade copied to clipboard

[RFC] new tools to mark PR as blocked or OK automatically

Open legalsylvain opened this issue 2 years ago • 9 comments

Hi @OCA/openupgrade-maintainers

Since I've been working on Openupgrade, I'm regularly annoyed to see PRs reviewed (or very simple), but not merged. the problem is that to merge PRs in OpenUpgrade, you have to follow the dependency ordering. (to merge "product", you need to have merged "uom").

so I spent a few hours making a script that automatically scans all OpenUpgrade open PRs, marking them as "Blocked by dependency" or "Dependency OK". (new green label).

As a result,

  • I merge #3964 (done by @huguesdk some days ago)
  • I run the script
  • Automatically the PR #3782 (done by @marielejeune, 3 monthes ago) is marked as "Dependency OK"

image

So once portal is merged, the following modules can be marked as Dependency OK (if all the other dependencies are OK): mail_group, website, test_mail_full, digest, mass_mailing_sms, auth_password_policy_portal, account, event, project, website_crm_partner_assign, payment, website_payment, portal_rating, auth_totp_portal etc ...

  • you can click here https://github.com/OCA/OpenUpgrade/pulls?q=is%3Apr+is%3Aopen+label%3A%22Dependency+OK%22 to review / merge PRs that are ready in a "dependency meaning"

note:

For the moment, the tools is available here : https://github.com/legalsylvain/openupgrade-extra-tools as a simple script. I'll execute it regularly. You can do it also, (as soon as you have maintainer accreditation).

CC : @vdewulf, @remytms

legalsylvain avatar Jul 02 '23 14:07 legalsylvain

great! simple and effective. thanks @legalsylvain!

huguesdk avatar Jul 03 '23 08:07 huguesdk

great tool ! :rocket:

remytms avatar Jul 03 '23 20:07 remytms

Hi @remytms and @huguesdk. Thanks for your feedbacks !

FYI, I just updated the script to add another feature :

  • For each opened PRs that are "blocked", enumerate the list of the PRs that we have to review first. For exemple : in the sale_stock PR (https://github.com/OCA/OpenUpgrade/pull/4015#issuecomment-1618878824), add a link to sale and stock_account PR.

image

Note : the text will be updated, each time I run the script and if the related PRs has been merged.

legalsylvain avatar Jul 03 '23 20:07 legalsylvain

this is great! Can we shoehorn this into a github action to run on every merge/PR opening?

hbrunn avatar Jul 11 '23 04:07 hbrunn

  • can be on every merge, indeed.
  • regarding every opening, it doesn't work, because my script rely on the main "Migration to X" issue. So we need to wait a maintainer has written "ocabot migration xxx". run on each comment looks a little bit heavy.

Alternatively, we can just run it each day. What do you think ?

Side question, for the time being, it is a custom repo, and that is my login that is changing / adding comment. do you want this repo to move under oca umbrella ?

legalsylvain avatar Jul 11 '23 09:07 legalsylvain

triggering the action on merge + once per day sounds good to me. and if it's an action in this repo, no problems with authentication, right?

hbrunn avatar Jul 11 '23 10:07 hbrunn

no problems with authentication, right?

I have to pass some authentication token to the script. don't know if Openupgrade contains oca-github-bot token...

https://github.com/legalsylvain/openupgrade-extra-tools/blob/master/check_dependency.py#L9

legalsylvain avatar Jul 11 '23 11:07 legalsylvain

that's just a configuration in the repo settings, where we can choose if the workflow's token has write permissions. alas, I don't have access to that for openupgrade

hbrunn avatar Jul 11 '23 12:07 hbrunn

@legalsylvain

Thank you very much for your utility.

Do you think it would be possible to transfer this work to the "Migration to 17.0" page, let me explain:

For example, I would like to migrate l10n-spain/l10n_en_account_asset. When you run this utility, the migration issue may be updated. So that it would look something like this:

l10n-spain

  • [ ] l10n_en_account_asset: PR Depends on:
    • [ ] account_asset_management: PR

account-financial-tools

  • [ ] account_asset_management: PR Depends on:
    • [ ] report_xlsx_helper: PR
    • [ ] account (Don't include is Odoo Community)

reporting-engine

  • [ ] report_xlsx_helper: PR Depends on:
    • [X] report_xlsx: PR

Then i will started by report_xlsx_helper, the last in the chain. Or if I look at any Migration to 17 page, I will know if all the dependencies are migrated or not.

FernandoRomera avatar Dec 14 '23 09:12 FernandoRomera