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.

Sign up for GitHub

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

Jump to bottom

[IMP] ocabot merge : check line in migration issue #192

Merged
merged 2 commits into from
Jun 17, 2023
Merged

[IMP] ocabot merge : check line in migration issue #192

merged 2 commits into from
Jun 17, 2023

Conversation

legalsylvain
Copy link
Collaborator

@legalsylvain legalsylvain commented Jul 1, 2022 •
edited
Loading

Rational

When maintainers are merging migration PR, they have to manually go to the according migration issue, and check the according line.
This operation is sometimes forgotten, and could be automated.

This PR introduces that feature.

Fix : #189
Discussion reference : OCA/OpenUpgrade#3288 (comment)

CC : @sbidoul, @pedrobaeza

Tested with ngrox : See edited migration issue by the bot here : grap-org-test-bot/github-ocabot-test#27 (1 July 2022 17h46)

@legalsylvain legalsylvain force-pushed the IMP-ocabot-merge-check-migration-issue branch from b23fa62 to 616a9a1 Compare July 1, 2022 15:48
@legalsylvain legalsylvain marked this pull request as ready for review July 1, 2022 15:52
@sbidoul sbidoul added enhancement New feature or request Bot Task A task the bot does or should do labels Jul 2, 2022
@legalsylvain legalsylvain force-pushed the IMP-ocabot-merge-check-migration-issue branch 3 times, most recently from 67ac186 to dd1b7ed Compare July 3, 2022 15:44
@pedrobaeza pedrobaeza mentioned this pull request Oct 21, 2022
Open
33 tasks
@legalsylvain legalsylvain force-pushed the IMP-ocabot-merge-check-migration-issue branch 2 times, most recently from 4f0668c to 898466f Compare January 27, 2023 14:13
@legalsylvain
Copy link
Collaborator Author

Hi.
@SirTakobi : I merged your Proposal. Thanks, and sorry for the delay.
@ALL : I deployed this PR on my bot and the feature works correctly. (Ref : grap/grap-odoo-custom#279)

I think it could be merged safely.

@pedrobaeza
Copy link
Member

@sbidoul this one is very interesting to be merged and deployed for synchronizing the issue information with the reality.

sbidoul
sbidoul reviewed Feb 7, 2023
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Ah, I had review comments that I forgot to submit, sorry about that.

src/oca_github_bot/tasks/merge_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/merge_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/merge_bot.py Outdated
migration_issue = _find_issue(gh_repo, milestone, target_branch)
if migration_issue:
new_body = _check_line_issue(gh_pr.number, migration_issue.body)
migration_issue.edit(body=new_body)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this whole block could become a function in migration_issue_bot.py, such as mark_migration_done_in_migration_issue(gh_repo, gh_pr) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done here : fd21074

let me know if all is ok or if it requires some changes.

thanks !

SirTakobi
SirTakobi approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@SirTakobi SirTakobi left a comment

Choose a reason for hiding this comment

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

Thanks!
I don't know about the policy for the commits in the repository, but only one commit should be enough: you can squash what I suggested in your changes.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

legalsylvain
legalsylvain commented Feb 7, 2023
View reviewed changes
Copy link
Collaborator Author

@legalsylvain legalsylvain left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @sbidoul. I applied the changes you proposed. Thanks for your review !

Regarding the refactor you propose, I don't have a clear point of view, and no time in the next week to work on the bot.
We can merge it as it, and make some refactor work in a next sprint, or let the PR unmerged if you think it's a blocking point.

I don't know about the policy for the commits in the repository, but only one commit should be enough: you can squash what I suggested in your changes.

@SirTakobi : I don't know about policy, but in that case, the first commit is mine, and the second one is your. If I squash, we will lose that information.

@legalsylvain

This comment was marked as duplicate.

Sign in to view
@OCA-git-bot

This comment was marked as duplicate.

Sign in to view
@OCA-git-bot OCA-git-bot force-pushed the IMP-ocabot-merge-check-migration-issue branch from a7dce55 to f5f22fa Compare February 7, 2023 20:10
@legalsylvain
Copy link
Collaborator Author

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to master.

@OCA-git-bot OCA-git-bot force-pushed the IMP-ocabot-merge-check-migration-issue branch from f5f22fa to d2171d4 Compare February 7, 2023 20:22
@legalsylvain legalsylvain force-pushed the IMP-ocabot-merge-check-migration-issue branch 2 times, most recently from 3018c45 to bfeb11d Compare February 7, 2023 20:42
@legalsylvain
[IMP] ocabot merge : check line in migration issue
654a78d 
Co-authored-by: Simone Rubino <[email protected]>
[IMP] Do not create milestone branch when merging PR
Co-authored-by: Stéphane Bidoul <[email protected]>
[pre-commit.ci] auto fixes from pre-commit.com hooks
@legalsylvain legalsylvain force-pushed the IMP-ocabot-merge-check-migration-issue branch from 431fa36 to 654a78d Compare April 8, 2023 10:00
@sbidoul sbidoul merged commit 16a0546 into OCA:master Jun 17, 2023
@pedrobaeza
Copy link
Member

Bravo! Are you going to deploy this for doing some checks on real scenarios?

@sbidoul
Copy link
Member

sbidoul commented Jun 17, 2023

@pedrobaeza it is now deployed.

@pedrobaeza
Copy link
Member

Thanks. I'll check and tell you back if anything wrong.

@pedrobaeza
Copy link
Member

It seems not working:

The merge process could not be finalized because an exception was raised: _find_issue() missing 1 required positional argument: 'target_branch'.

OCA/server-backend#202

cc @legalsylvain @sbidoul

@legalsylvain legalsylvain mentioned this pull request Jun 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbidoul sbidoul sbidoul left review comments

@SirTakobi SirTakobi SirTakobi approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved Bot Task A task the bot does or should do enhancement New feature or request ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merging PR should mark a line in the "Migration to version xx" issue as done.
5 participants
@legalsylvain @pedrobaeza @OCA-git-bot @sbidoul @SirTakobi