-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Fix automerge will not work because of some events haven't been triggered #30780
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
Conversation
I think we need some tests. |
@@ -111,6 +104,33 @@ func MergeScheduledPullRequest(ctx context.Context, sha string, repo *repo_model | |||
return nil | |||
} | |||
|
|||
// StartPRCheckAndAutoMerge start an automerge check and auto merge task for a pull request | |||
func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make StartPRCheckAndAutoMergeBySHA
call StartPRCheckAndAutoMerge
to avoid duplicate code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are two different functions. I think less code are duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't see why sha
should be passed into addToQueue
It seems that the sha
is only used for logging. Ideally only call addToQueue(pr.ID)
should be enough, and a lot of code could be simplified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the SHA will prevent when both an automerge task and a newly push are triggered. So the previous triggered automerge task will be ignored.
if headCommitID != sha {
log.Warn("Head commit id of auto merge %-v does not match sha [%s], it may means the head branch has been updated. Just ignore this request because a new request expected in the queue", pr, sha)
return
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually such check doesn't seem right and only makes some problem harder to debug.
Because, the head commit ID could change again after that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not perfect. It needs more refactor like I said before. But since it's a bugfix, I will not do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem quite right, but just do it.
Co-authored-by: wxiaoguang <[email protected]>
…ered (go-gitea#30780) Replace go-gitea#25741 Close go-gitea#24445 Close go-gitea#30658 Close go-gitea#20646 ~Depends on go-gitea#30805~ Since go-gitea#25741 has been rewritten totally, to make the contribution easier, I will continue the work in this PR. Thanks @6543 --------- Co-authored-by: 6543 <[email protected]> Co-authored-by: wxiaoguang <[email protected]>
…ered (#30780) (#31039) Backport #30780 by @lunny Replace #25741 Close #24445 Close #30658 Close #20646 ~Depends on #30805~ Since #25741 has been rewritten totally, to make the contribution easier, I will continue the work in this PR. Thanks @6543 Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: 6543 <[email protected]> Co-authored-by: wxiaoguang <[email protected]>
* giteaofficial/main: Add nix flake for dev shell (go-gitea#30967) [skip ci] Updated translations via Crowdin Fix wrong display of recently pushed notification (go-gitea#25812) use existing oauth grant for public client (go-gitea#31015) Fix automerge will not work because of some events haven't been triggered (go-gitea#30780)
Replace #25741
Close #24445
Close #30658
Close #20646
Depends on #30805Since #25741 has been rewritten totally, to make the contribution easier, I will continue the work in this PR. Thanks @6543