Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

legalsylvain
Copy link
Collaborator

@legalsylvain legalsylvain commented Jul 1, 2022

Use case

  • When maintainers are calling ocabot migration
  • if previous PR was present,

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 : OCA/OpenUpgrade#3334 (comment)
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 : grap-org-test-bot/github-ocabot-test#29 (comment)

@legalsylvain legalsylvain force-pushed the IMP-ocabot-migration-alert-potentiel-duplicated-work branch 3 times, most recently from 17dbea3 to cd30252 Compare July 3, 2022 15:15
@legalsylvain
Copy link
Collaborator Author

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
Copy link
Member

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

@legalsylvain
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@pedrobaeza
Copy link
Member

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
Copy link
Collaborator Author

legalsylvain commented Jul 3, 2022

You probably right. I created the issue. #194

In the meantime, could you review this one ?

Thanks !

@legalsylvain
Copy link
Collaborator Author

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to master.

@legalsylvain legalsylvain force-pushed the IMP-ocabot-migration-alert-potentiel-duplicated-work branch from a49f4be to d99463a Compare July 3, 2022 21:51
Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, and I agree with the above re overwriting

@legalsylvain legalsylvain force-pushed the IMP-ocabot-migration-alert-potentiel-duplicated-work branch from 8b12670 to c8555ba Compare July 4, 2022 17:28
@legalsylvain
Copy link
Collaborator Author

Hi @hbrunn.

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

Should be OK.

Regards.

@legalsylvain
Copy link
Collaborator Author

@sbidoul : coule you deploy this one ?

@sbidoul sbidoul merged commit 5fee2a5 into OCA:master Jul 16, 2022
@sbidoul
Copy link
Member

sbidoul commented Jul 16, 2022

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

@legalsylvain
Copy link
Collaborator Author

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants