From ff5df5fc97463079915e0f43682dd747860dae61 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Thu, 22 Feb 2024 22:47:35 +0800 Subject: [PATCH] Improve the `issue_comment` workflow trigger event (#29277) Fix #29175 Replace #29207 This PR makes some improvements to the `issue_comment` workflow trigger event. 1. Fix the bug that pull requests cannot trigger `issue_comment` workflows 2. Previously the `issue_comment` event only supported the `created` activity type. This PR adds support for the missing `edited` and `deleted` activity types. 3. Some events (including `issue_comment`, `issues`, etc. ) only trigger workflows that belong to the workflow file on the default branch. This PR introduces the `IsDefaultBranchWorkflow` function to check for these events. (cherry picked from commit a4fe1cdf38f9a063e44b197ef07e4260f731c919) Conflicts: modules/actions/github.go context --- modules/actions/github.go | 44 +++++++++++++ modules/actions/github_test.go | 6 ++ services/actions/notifier.go | 96 ++++++++++++++++++++++------- services/actions/notifier_helper.go | 11 ++-- 4 files changed, 130 insertions(+), 27 deletions(-) diff --git a/modules/actions/github.go b/modules/actions/github.go index d4e559408..cc04ca5c5 100644 --- a/modules/actions/github.go +++ b/modules/actions/github.go @@ -25,6 +25,45 @@ const ( GithubEventSchedule = "schedule" ) +// IsDefaultBranchWorkflow returns true if the event only triggers workflows on the default branch +func IsDefaultBranchWorkflow(triggedEvent webhook_module.HookEventType) bool { + switch triggedEvent { + case webhook_module.HookEventDelete: + // GitHub "delete" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#delete + return true + case webhook_module.HookEventFork: + // GitHub "fork" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#fork + return true + case webhook_module.HookEventIssueComment: + // GitHub "issue_comment" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment + return true + case webhook_module.HookEventPullRequestComment: + // GitHub "pull_request_comment" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_comment-use-issue_comment + return true + case webhook_module.HookEventWiki: + // GitHub "gollum" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#gollum + return true + case webhook_module.HookEventSchedule: + // GitHub "schedule" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule + return true + case webhook_module.HookEventIssues, + webhook_module.HookEventIssueAssign, + webhook_module.HookEventIssueLabel, + webhook_module.HookEventIssueMilestone: + // Github "issues" event + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issues + return true + } + + return false +} + // canGithubEventMatch check if the input Github event can match any Gitea event. func canGithubEventMatch(eventName string, triggedEvent webhook_module.HookEventType) bool { switch eventName { @@ -75,6 +114,11 @@ func canGithubEventMatch(eventName string, triggedEvent webhook_module.HookEvent return false } + case GithubEventIssueComment: + // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_comment-use-issue_comment + return triggedEvent == webhook_module.HookEventIssueComment || + triggedEvent == webhook_module.HookEventPullRequestComment + default: return eventName == string(triggedEvent) } diff --git a/modules/actions/github_test.go b/modules/actions/github_test.go index 4bf55ae03..6652ff6ea 100644 --- a/modules/actions/github_test.go +++ b/modules/actions/github_test.go @@ -103,6 +103,12 @@ func TestCanGithubEventMatch(t *testing.T) { webhook_module.HookEventCreate, true, }, + { + "create pull request comment", + GithubEventIssueComment, + webhook_module.HookEventPullRequestComment, + true, + }, } for _, tc := range testCases { diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 77848a3f5..e144484da 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -224,37 +224,88 @@ func (n *actionsNotifier) CreateIssueComment(ctx context.Context, doer *user_mod ) { ctx = withMethod(ctx, "CreateIssueComment") - permission, _ := access_model.GetUserRepoPermission(ctx, repo, doer) - if issue.IsPull { - if err := issue.LoadPullRequest(ctx); err != nil { + notifyIssueCommentChange(ctx, doer, comment, "", webhook_module.HookEventPullRequestComment, api.HookIssueCommentCreated) + return + } + notifyIssueCommentChange(ctx, doer, comment, "", webhook_module.HookEventIssueComment, api.HookIssueCommentCreated) +} + +func (n *actionsNotifier) UpdateComment(ctx context.Context, doer *user_model.User, c *issues_model.Comment, oldContent string) { + ctx = withMethod(ctx, "UpdateComment") + + if err := c.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return + } + + if c.Issue.IsPull { + notifyIssueCommentChange(ctx, doer, c, oldContent, webhook_module.HookEventPullRequestComment, api.HookIssueCommentEdited) + return + } + notifyIssueCommentChange(ctx, doer, c, oldContent, webhook_module.HookEventIssueComment, api.HookIssueCommentEdited) +} + +func (n *actionsNotifier) DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) { + ctx = withMethod(ctx, "DeleteComment") + + if err := comment.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return + } + + if comment.Issue.IsPull { + notifyIssueCommentChange(ctx, doer, comment, "", webhook_module.HookEventPullRequestComment, api.HookIssueCommentDeleted) + return + } + notifyIssueCommentChange(ctx, doer, comment, "", webhook_module.HookEventIssueComment, api.HookIssueCommentDeleted) +} + +func notifyIssueCommentChange(ctx context.Context, doer *user_model.User, comment *issues_model.Comment, oldContent string, event webhook_module.HookEventType, action api.HookIssueCommentAction) { + if err := comment.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return + } + if err := comment.Issue.LoadAttributes(ctx); err != nil { + log.Error("LoadAttributes: %v", err) + return + } + + permission, _ := access_model.GetUserRepoPermission(ctx, comment.Issue.Repo, doer) + + payload := &api.IssueCommentPayload{ + Action: action, + Issue: convert.ToAPIIssue(ctx, comment.Issue), + Comment: convert.ToAPIComment(ctx, comment.Issue.Repo, comment), + Repository: convert.ToRepo(ctx, comment.Issue.Repo, permission), + Sender: convert.ToUser(ctx, doer, nil), + IsPull: comment.Issue.IsPull, + } + + if action == api.HookIssueCommentEdited { + payload.Changes = &api.ChangesPayload{ + Body: &api.ChangesFromPayload{ + From: oldContent, + }, + } + } + + if comment.Issue.IsPull { + if err := comment.Issue.LoadPullRequest(ctx); err != nil { log.Error("LoadPullRequest: %v", err) return } - newNotifyInputFromIssue(issue, webhook_module.HookEventPullRequestComment). + newNotifyInputFromIssue(comment.Issue, event). WithDoer(doer). - WithPayload(&api.IssueCommentPayload{ - Action: api.HookIssueCommentCreated, - Issue: convert.ToAPIIssue(ctx, issue), - Comment: convert.ToAPIComment(ctx, repo, comment), - Repository: convert.ToRepo(ctx, repo, permission), - Sender: convert.ToUser(ctx, doer, nil), - IsPull: true, - }). - WithPullRequest(issue.PullRequest). + WithPayload(payload). + WithPullRequest(comment.Issue.PullRequest). Notify(ctx) return } - newNotifyInputFromIssue(issue, webhook_module.HookEventIssueComment). + + newNotifyInputFromIssue(comment.Issue, event). WithDoer(doer). - WithPayload(&api.IssueCommentPayload{ - Action: api.HookIssueCommentCreated, - Issue: convert.ToAPIIssue(ctx, issue), - Comment: convert.ToAPIComment(ctx, repo, comment), - Repository: convert.ToRepo(ctx, repo, permission), - Sender: convert.ToUser(ctx, doer, nil), - IsPull: false, - }). + WithPayload(payload). Notify(ctx) } @@ -496,7 +547,6 @@ func (n *actionsNotifier) DeleteRef(ctx context.Context, pusher *user_model.User apiRepo := convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm_model.AccessModeNone}) newNotifyInput(repo, pusher, webhook_module.HookEventDelete). - WithRef(refFullName.ShortName()). // FIXME: should we use a full ref name WithPayload(&api.DeletePayload{ Ref: refFullName.ShortName(), RefType: refFullName.RefType(), diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 46be4ac0a..2d138aaf3 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -133,12 +133,15 @@ func notify(ctx context.Context, input *notifyInput) error { defer gitRepo.Close() ref := input.Ref - if input.Event == webhook_module.HookEventDelete { - // The event is deleting a reference, so it will fail to get the commit for a deleted reference. - // Set ref to empty string to fall back to the default branch. - ref = "" + if ref != input.Repo.DefaultBranch && actions_module.IsDefaultBranchWorkflow(input.Event) { + if ref != "" { + log.Warn("Event %q should only trigger workflows on the default branch, but its ref is %q. Will fall back to the default branch", + input.Event, ref) + } + ref = input.Repo.DefaultBranch } if ref == "" { + log.Warn("Ref of event %q is empty, will fall back to the default branch", input.Event) ref = input.Repo.DefaultBranch }