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

Auto merge pull requests when all checks succeeded via API #9307

Merged
merged 307 commits into from
May 7, 2022
Merged

Auto merge pull requests when all checks succeeded via API #9307

merged 307 commits into from
May 7, 2022

Conversation

kolaente
Copy link
Member

@kolaente kolaente commented Dec 9, 2019 •
edited by 6543
Loading

This adds the functionality to auto merge a pull request, once all status checks succeed.

To achieve this, it schedules a pr to merge when a user request a merge via API and set merge_when_checks_succeed to true.
On every status check update, if there are scheduled merge requests for this pull and that pull pass all merge requirements, they get merged.

I created fake statuses for testing by running curl -X POST "http://localhost:3000/api/v1/repos/kolaente/testing/statuses/ccd425c09136a411236830785c41afdc172e8208" -H "accept: application/json" -H "authorization: Basic a29sYWVudGU6anVwMjAwMA==" -H "Content-Type: application/json" -d '{"context": "test/context", "description": "description", "state": "pending", "target_url": "http://localhost"}' | jq

TODO

  • Show on the pr page if a pr was scheduled for auto merge
    • Add a comment {User} scheduled this pr to auto-merge
  • Provide an option to bypass auto merge entirely and merge directly without waiting for status checks
  • Provide an option to cancel auto merge once it's set
  • Add a check to see if all checks already succeeded in routers/repo/pull.go:676 to prevent scheduling a pr for merging after all checks already passed.
  • Only check if all required status checks succeeded or all if none are required in services/automerge/pull_auto_merge.go:89
  • Add an event comment to the pr timeline if it was set/unset to auto merge
  • Make sure to check all prs with the same head branch of a commit for merging PRs when a check succeeds
  • Fix dropdown button width of the merge button
  • Allow both directly merging and scheduling for merge
  • Add "merge now" button next to "merge when checks succeed"
  • Cleanup scheduled pr merge when it failed to get merged + log error
  • Move handling to [a queue](example: Use queue instead of memory queue in webhook send service #19390)
  • Check permissions again right before merging
  • Automerge does not respect other branch protections yet
  • pull_service.Merge() need a lock -> move it into won pull (PullService lock via pullID #19520)
  • Fix UI glitch to Create AutoMerge -> new issue ... new pull Auto merge pull requests when all checks succeeded via WebUI #19621
  • API cancel auto-merge

Optional TODOs

  • Show auto merge status on PR list

close #3905

@kolaente
Copy link
Member Author

kolaente commented Dec 9, 2019

I'm not sure why the vendor check fails, it passes on my local machine.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 9, 2019
guillep2k
guillep2k reviewed Dec 10, 2019
View reviewed changes
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

I've only given it a quick look.
BTW, is it possible that you're still using go 1.11 instead of 1.13?

models/pull.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
modules/git/commit.go Outdated Show resolved Hide resolved
routers/repo/pull.go Outdated Show resolved Hide resolved
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 10, 2019
@lunny lunny added this to the 1.12.0 milestone Dec 10, 2019
@kolaente
Copy link
Member Author

BTW, is it possible that you're still using go 1.11 instead of 1.13?

I'm on go 1.12.9, should be fine. I'll try upgrading to see if the problem still exists.

@kolaente
Copy link
Member Author

Looks like make check-vendor now passes - seems like there are differences between Go 1.12 and 1.13.

@kolaente
Copy link
Member Author

kolaente commented Jan 9, 2020

I'm not sure where all of these commits are coming from, looks like my local gitea master was outdated...

@kolaente
Copy link
Member Author

This seems to be ready for review now.

@kolaente kolaente changed the title WIP: Auto merge pull requests when all checks succeeded Auto merge pull requests when all checks succeeded Jan 27, 2020
6543
6543 reviewed Jan 30, 2020
View reviewed changes
modules/auth/repo_form.go Outdated Show resolved Hide resolved
lunny
lunny reviewed Jan 31, 2020
View reviewed changes
models/migrations/v126.go Outdated Show resolved Hide resolved
lunny
lunny reviewed Jan 31, 2020
View reviewed changes
models/pull.go Outdated Show resolved Hide resolved
@codecov-io

This comment was marked as outdated.

Sign in to view
@kolaente
Copy link
Member Author

kolaente commented Feb 4, 2020

Tests are now passing, so ready for review (again).

@kolaente
Copy link
Member Author

kolaente commented Feb 4, 2020

Should I try to remove the unneeded commits? It might not be necessary since we're squash merging anyway and there are no unrelated changes in this pr.

@6543
Copy link
Member

6543 commented Feb 9, 2020

@kolaente can you add some tests?

@6543
Copy link
Member

6543 commented Mar 31, 2020

conflicts :D

@lunny
Copy link
Member

lunny commented May 2, 2020

Please resolve the conflicts.

@kolaente kolaente force-pushed the feature/auto-merge branch 2 times, most recently from 7460ecc to a61f329 Compare May 4, 2020 20:09
@kolaente
Copy link
Member Author

kolaente commented May 4, 2020

@6543 @lunny Resolved the conflicts. Is there anything left to do?

(Now also with a clean commit history)

@kolaente
Copy link
Member Author

kolaente commented May 4, 2020

And now tests are failing again... I guess I'll fix those first

a1012112796
a1012112796 reviewed May 5, 2020
View reviewed changes
Copy link
Member

@a1012112796 a1012112796 left a comment

Choose a reason for hiding this comment

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

These are just my personal opinions, not necessarily correct. Thanks

models/pull.go Outdated Show resolved Hide resolved
routers/repo/pull.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
services/automerge/pull_auto_merge.go Outdated Show resolved Hide resolved
@a1012112796
Copy link
Member

I think it would be fun if you can add event comment on the pr time-line when add or remove a auto-merge task, which is similar with Assign , Labels, review .., Thanks

@kolaente
Copy link
Member Author

kolaente commented May 6, 2020

I think it would be fun if you can add event comment on the pr time-line when add or remove a auto-merge task, which is similar with Assign , Labels, review .., Thanks

Sounds like a good idea.

@a1012112796
Copy link
Member

500 error while viewing pull request.
log:

2020/05/07 15:47:48 ...outers/repo/issue.go:1111:ViewIssue() [E] GetReviewersByIssueID: no such column: created_unix
2020/05/07 15:47:48 ...m.io/xorm/core/db.go:154:QueryContext() [I] [SQL] SELECT count(*) FROM `notification` WHERE (user_id = ?) AND (status = ?) [3 1]
2020/05/07 15:47:48 ...m.io/xorm/core/db.go:154:QueryContext() [I] [SQL] SELECT `id`, `lower_name`, `name`, `full_name`, `email`, `keep_email_private`, `email_notifications_preference`, `passwd`, `passwd_hash_algo`, `must_change_password`, `login_type`, `login_source`, `login_name`, `type`, `location`, `website`, `rands`, `salt`, `language`, `description`, `created_unix`, `updated_unix`, `last_login_unix`, `last_repo_visibility`, `max_repo_creation`, `is_active`, `is_admin`, `is_restricted`, `allow_git_hook`, `allow_import_local`, `allow_create_organization`, `prohibit_login`, `avatar`, `avatar_email`, `use_custom_avatar`, `num_followers`, `num_following`, `num_stars`, `num_repos`, `num_teams`, `num_members`, `visibility`, `repo_admin_change_team_access`, `diff_view_style`, `theme` FROM `user` WHERE `id`=? LIMIT 1 [3]

a1012112796
a1012112796 reviewed May 7, 2020
View reviewed changes
routers/repo/issue.go Outdated Show resolved Hide resolved
delvh
delvh approved these changes May 7, 2022
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Nope

integrations/git_test.go Outdated Show resolved Hide resolved
delvh
delvh approved these changes May 7, 2022
View reviewed changes
integrations/git_test.go Outdated Show resolved Hide resolved
6543
6543 approved these changes May 7, 2022
View reviewed changes
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

ping bot?

zeripath
zeripath approved these changes May 7, 2022
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Approving to make lgtm work

@lunny
Copy link
Member

lunny commented May 7, 2022

We can just merge this with administrator permission after CI PASS.

@6543
Copy link
Member

6543 commented May 7, 2022

307 commits and 2½years (~76025940sec) later 🎉

@lunny lunny added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 7, 2022
@lunny lunny merged commit 59b30f0 into go-gitea:main May 7, 2022
@6543 6543 deleted the feature/auto-merge branch May 7, 2022 17:07
This was referenced May 7, 2022
Merged
Merged
@wxiaoguang wxiaoguang mentioned this pull request May 7, 2022
Merged
@kolaente
Copy link
Member Author

kolaente commented May 7, 2022

Wow awesome! Thanks @6543 and everyone else who picked this up and finished it!

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 7, 2022
@6543 6543 removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 7, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 8, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
96e8813 
* giteaofficial/main:
  Delete related PullAutoMerge and ReviewState on User/Repo Deletion (go-gitea#19649)
  Allow custom default merge message with .gitea/default_merge_message/<merge_style>_TEMPLATE.md (go-gitea#18177)
  Allow to mark files in a PR as viewed (go-gitea#19007)
  Auto merge pull requests when all checks succeeded via API (go-gitea#9307)
  Hide private repositories in packages (go-gitea#19584)
  Only show accessible teams in dashboard dropdown list (go-gitea#19642)
  prevent double click new issue/pull/comment button (go-gitea#16157)
  Improve reviewing PR UX (go-gitea#19612)
  [skip ci] Updated translations via Crowdin
  Add Changelog v1.16.7 (go-gitea#19575) (go-gitea#19644)
  Set safe dir for git operations in .drone.yml CI (go-gitea#19641)
  Add missing `sorting` column in `project_issue` table (go-gitea#19635)
@dirien
Copy link

dirien commented May 9, 2022

Really great work @6543 and all the other contributer!

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 9, 2022
@wxiaoguang wxiaoguang removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 9, 2022
@6543
Copy link
Member

6543 commented May 20, 2022 •
edited
Loading

(split out) WebUI part is ready: #19648

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 20, 2022
@6543 6543 removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 20, 2022
@go-gitea go-gitea locked as resolved and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@qwerty287 qwerty287 qwerty287 left review comments

@a1012112796 a1012112796 a1012112796 left review comments

@guillep2k guillep2k guillep2k left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@delvh delvh delvh approved these changes

@6543 6543 6543 approved these changes

@zeripath zeripath zeripath approved these changes

@jonasfranz jonasfranz Awaiting requested review from jonasfranz

@lunny lunny Awaiting requested review from lunny

Assignees

@6543 6543

Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Automatically maintain a repository of code that always passes all the tests.