OCA / oca-github-bot

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

[ADD] add warning in ocabot migration if previous PR was present for the same module #191

Closed legalsylvain closed 1 year ago

legalsylvain commented 2 years ago

Use case

Current behaviour The line of the migration issue is replaced silently by the new PR. (and new author). However, in many cases, it is a duplication of work.

Current workaround

Maintainers should take a look on the migration issue, to check if an existing PR still exist and alert on the PR. See for exemple : https://github.com/OCA/OpenUpgrade/pull/3334#issuecomment-1164117298 For repos with a large amount of modules, it is very complicated to remember that a porting is already in progress.

With that PR

If an existing PR is referenced in the migration issue, a message is added to the new PR by the bot, referencing the first PR and pinging the original author.

image

CC :

Note : tested with ngrox on a grap repo. See : https://github.com/grap-org-test-bot/github-ocabot-test/pull/29#issuecomment-1172407880

legalsylvain commented 2 years ago

Rebase done. This one is ready to review.

CC : @OCA/openupgrade-maintainers : this one could interest you in case two contributors work on the same module migration.

pedrobaeza commented 2 years ago

Isn't better to not overwrite it in such case unless the PR is closed?

legalsylvain commented 2 years ago

Isn't better to not overwrite it in such case unless the PR is closed?

I don't know ! I abstain on that topic. Feel free to open an issue / PR if you think it's relevant. In my opinion, maintainers will have to do manual work. (if we keep old PR, maybe we should set the new one manually, and if we get the new PR, maybe we should set the old one manually...) We can not predict what is the best PR.

My concern is to make sure that everyone knows that there is a duplicate work in progress. (The author of the new PR, the author of the previous PR, and the rest maintainers). For the time being, with 130 openened PR on Openupgrade project, I really can't remember if anyone has already worked on the migration from account to V15.

Note : that PR is not changing the algorithm you introduced in #97. That is the current behaviour.

legalsylvain commented 2 years ago

Note : CI is failing for weird reason. Don't understand.

pedrobaeza commented 2 years ago

I know that my initial intention was to always overwrite, thinking that maintainers will know what to do, but now seeing in perspective, it seems that people don't look too much, so that's why I say to change the initial intention to prevent such change unless the PR is closed.

legalsylvain commented 2 years ago

You probably right. I created the issue. https://github.com/OCA/oca-github-bot/issues/194

In the meantime, could you review this one ?

Thanks !

legalsylvain commented 2 years ago

/ocabot rebase

OCA-git-bot commented 2 years ago

Congratulations, PR rebased to master.

legalsylvain commented 2 years ago

Hi @hbrunn.

Thanks for your review. i accepted your two changes and squashed commits.

Should be OK.

Regards.

legalsylvain commented 2 years ago

@sbidoul : coule you deploy this one ?

sbidoul commented 1 year ago

@legalsylvain I just deployed the master branch on the OCA server

legalsylvain commented 1 year ago

Thanks !