From 2d91afaa92609d31b04aea9ad909cec8a574cacd Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Thu, 13 Apr 2023 00:16:47 +0800 Subject: [PATCH] Fix mismatch between hook events and github event types (#24048) Some workflow trigger events can have multiple activity types, such as `issues` and `pull_request`, and user can specify which types can trigger the workflow. See GitHub documentation: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows Now some hook events cannot match the workflow trigger events correctly because we don't check the activity types. For example, `pull_request_label` is an individual hook event. But there isn't a `pull_request_label` workflow trigger event, we can only use `pull_request` event's `label` activity type. If we don't check the activity types, the workflows without the `label` activity type may be triggered by the `pull_request_label` event by mistake. We need to improve the match logic. - [x] [`issues` ](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issues) - [x] [`issue_comment`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment) - [x] [`pull_request`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request) - [x] [`pull_request_review`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review) - [x] [`pull_request_review_comment`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review_comment) - [x] [`release`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#release) - [x] [`registry_package`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#registry_package) --- modules/actions/workflows.go | 282 +++++++++++++++++++++++++++--- modules/actions/workflows_test.go | 105 +++++++++++ 2 files changed, 362 insertions(+), 25 deletions(-) create mode 100644 modules/actions/workflows_test.go diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index d30982a46..d21dc1d80 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -115,40 +115,60 @@ func detectMatched(commit *git.Commit, triggedEvent webhook_module.HookEventType } switch triggedEvent { - case webhook_module.HookEventCreate, + case // events with no activity types + webhook_module.HookEventCreate, webhook_module.HookEventDelete, webhook_module.HookEventFork, - webhook_module.HookEventIssueAssign, - webhook_module.HookEventIssueLabel, - webhook_module.HookEventIssueMilestone, - webhook_module.HookEventPullRequestAssign, - webhook_module.HookEventPullRequestLabel, - webhook_module.HookEventPullRequestMilestone, - webhook_module.HookEventPullRequestComment, - webhook_module.HookEventPullRequestReviewApproved, - webhook_module.HookEventPullRequestReviewRejected, - webhook_module.HookEventPullRequestReviewComment, - webhook_module.HookEventWiki, - webhook_module.HookEventRepository, - webhook_module.HookEventRelease, - webhook_module.HookEventPackage: + // FIXME: `wiki` event should match `gollum` event + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#gollum + webhook_module.HookEventWiki: if len(evt.Acts()) != 0 { log.Warn("Ignore unsupported %s event arguments %v", triggedEvent, evt.Acts()) } // no special filter parameters for these events, just return true if name matched return true - case webhook_module.HookEventPush: + case // push + webhook_module.HookEventPush: return matchPushEvent(commit, payload.(*api.PushPayload), evt) - case webhook_module.HookEventIssues: + case // issues + webhook_module.HookEventIssues, + webhook_module.HookEventIssueAssign, + webhook_module.HookEventIssueLabel, + webhook_module.HookEventIssueMilestone: return matchIssuesEvent(commit, payload.(*api.IssuePayload), evt) - case webhook_module.HookEventPullRequest, webhook_module.HookEventPullRequestSync: + case // issue_comment + webhook_module.HookEventIssueComment, + // `pull_request_comment` is same as `issue_comment` + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_comment-use-issue_comment + webhook_module.HookEventPullRequestComment: + return matchIssueCommentEvent(commit, payload.(*api.IssueCommentPayload), evt) + + case // pull_request + webhook_module.HookEventPullRequest, + webhook_module.HookEventPullRequestSync, + webhook_module.HookEventPullRequestAssign, + webhook_module.HookEventPullRequestLabel: return matchPullRequestEvent(commit, payload.(*api.PullRequestPayload), evt) - case webhook_module.HookEventIssueComment: - return matchIssueCommentEvent(commit, payload.(*api.IssueCommentPayload), evt) + case // pull_request_review + webhook_module.HookEventPullRequestReviewApproved, + webhook_module.HookEventPullRequestReviewRejected: + return matchPullRequestReviewEvent(commit, payload.(*api.PullRequestPayload), evt) + + case // pull_request_review_comment + webhook_module.HookEventPullRequestReviewComment: + return matchPullRequestReviewCommentEvent(commit, payload.(*api.PullRequestPayload), evt) + + case // release + webhook_module.HookEventRelease: + return matchReleaseEvent(commit, payload.(*api.ReleasePayload), evt) + + case // registry_package + webhook_module.HookEventPackage: + return matchPackageEvent(commit, payload.(*api.PackagePayload), evt) default: log.Warn("unsupported event %q", triggedEvent) @@ -265,8 +285,24 @@ func matchIssuesEvent(commit *git.Commit, issuePayload *api.IssuePayload, evt *j for cond, vals := range evt.Acts() { switch cond { case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issues + // Actions with the same name: + // opened, edited, closed, reopened, assigned, unassigned, milestoned, demilestoned + // Actions need to be converted: + // label_updated -> labeled + // label_cleared -> unlabeled + // Unsupported activity types: + // deleted, transferred, pinned, unpinned, locked, unlocked + + action := issuePayload.Action + switch action { + case api.HookIssueLabelUpdated: + action = "labeled" + case api.HookIssueLabelCleared: + action = "unlabeled" + } for _, val := range vals { - if glob.MustCompile(val, '/').Match(string(issuePayload.Action)) { + if glob.MustCompile(val, '/').Match(string(action)) { matchTimes++ break } @@ -281,8 +317,9 @@ func matchIssuesEvent(commit *git.Commit, issuePayload *api.IssuePayload, evt *j func matchPullRequestEvent(commit *git.Commit, prPayload *api.PullRequestPayload, evt *jobparser.Event) bool { // with no special filter parameters if len(evt.Acts()) == 0 { - // defaultly, only pull request opened and synchronized will trigger workflow - return prPayload.Action == api.HookIssueSynchronized || prPayload.Action == api.HookIssueOpened + // defaultly, only pull request `opened`, `reopened` and `synchronized` will trigger workflow + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request + return prPayload.Action == api.HookIssueSynchronized || prPayload.Action == api.HookIssueOpened || prPayload.Action == api.HookIssueReOpened } matchTimes := 0 @@ -290,9 +327,24 @@ func matchPullRequestEvent(commit *git.Commit, prPayload *api.PullRequestPayload for cond, vals := range evt.Acts() { switch cond { case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request + // Actions with the same name: + // opened, edited, closed, reopened, assigned, unassigned + // Actions need to be converted: + // synchronized -> synchronize + // label_updated -> labeled + // label_cleared -> unlabeled + // Unsupported activity types: + // converted_to_draft, ready_for_review, locked, unlocked, review_requested, review_request_removed, auto_merge_enabled, auto_merge_disabled + action := prPayload.Action - if prPayload.Action == api.HookIssueSynchronized { + switch action { + case api.HookIssueSynchronized: action = "synchronize" + case api.HookIssueLabelUpdated: + action = "labeled" + case api.HookIssueLabelCleared: + action = "unlabeled" } log.Trace("matching pull_request %s with %v", action, vals) for _, val := range vals { @@ -363,6 +415,14 @@ func matchIssueCommentEvent(commit *git.Commit, issueCommentPayload *api.IssueCo for cond, vals := range evt.Acts() { switch cond { case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment + // Actions with the same name: + // created, edited, deleted + // Actions need to be converted: + // NONE + // Unsupported activity types: + // NONE + for _, val := range vals { if glob.MustCompile(val, '/').Match(string(issueCommentPayload.Action)) { matchTimes++ @@ -370,7 +430,179 @@ func matchIssueCommentEvent(commit *git.Commit, issueCommentPayload *api.IssueCo } } default: - log.Warn("issue comment unsupported condition %q", cond) + log.Warn("issue comment event unsupported condition %q", cond) + } + } + return matchTimes == len(evt.Acts()) +} + +func matchPullRequestReviewEvent(commit *git.Commit, prPayload *api.PullRequestPayload, evt *jobparser.Event) bool { + // with no special filter parameters + if len(evt.Acts()) == 0 { + return true + } + + matchTimes := 0 + // all acts conditions should be satisfied + for cond, vals := range evt.Acts() { + switch cond { + case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review + // Activity types with the same name: + // NONE + // Activity types need to be converted: + // reviewed -> submitted + // reviewed -> edited + // Unsupported activity types: + // dismissed + + actions := make([]string, 0) + if prPayload.Action == api.HookIssueReviewed { + // the `reviewed` HookIssueAction can match the two activity types: `submitted` and `edited` + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review + actions = append(actions, "submitted", "edited") + } + + matched := false + for _, val := range vals { + for _, action := range actions { + if glob.MustCompile(val, '/').Match(action) { + matched = true + break + } + } + if matched { + break + } + } + if matched { + matchTimes++ + } + default: + log.Warn("pull request review event unsupported condition %q", cond) + } + } + return matchTimes == len(evt.Acts()) +} + +func matchPullRequestReviewCommentEvent(commit *git.Commit, prPayload *api.PullRequestPayload, evt *jobparser.Event) bool { + // with no special filter parameters + if len(evt.Acts()) == 0 { + return true + } + + matchTimes := 0 + // all acts conditions should be satisfied + for cond, vals := range evt.Acts() { + switch cond { + case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review_comment + // Activity types with the same name: + // NONE + // Activity types need to be converted: + // reviewed -> created + // reviewed -> edited + // Unsupported activity types: + // deleted + + actions := make([]string, 0) + if prPayload.Action == api.HookIssueReviewed { + // the `reviewed` HookIssueAction can match the two activity types: `created` and `edited` + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review_comment + actions = append(actions, "created", "edited") + } + + matched := false + for _, val := range vals { + for _, action := range actions { + if glob.MustCompile(val, '/').Match(action) { + matched = true + break + } + } + if matched { + break + } + } + if matched { + matchTimes++ + } + default: + log.Warn("pull request review comment event unsupported condition %q", cond) + } + } + return matchTimes == len(evt.Acts()) +} + +func matchReleaseEvent(commit *git.Commit, payload *api.ReleasePayload, evt *jobparser.Event) bool { + // with no special filter parameters + if len(evt.Acts()) == 0 { + return true + } + + matchTimes := 0 + // all acts conditions should be satisfied + for cond, vals := range evt.Acts() { + switch cond { + case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#release + // Activity types with the same name: + // published + // Activity types need to be converted: + // updated -> edited + // Unsupported activity types: + // unpublished, created, deleted, prereleased, released + + action := payload.Action + switch action { + case api.HookReleaseUpdated: + action = "edited" + } + for _, val := range vals { + if glob.MustCompile(val, '/').Match(string(action)) { + matchTimes++ + break + } + } + default: + log.Warn("release event unsupported condition %q", cond) + } + } + return matchTimes == len(evt.Acts()) +} + +func matchPackageEvent(commit *git.Commit, payload *api.PackagePayload, evt *jobparser.Event) bool { + // with no special filter parameters + if len(evt.Acts()) == 0 { + return true + } + + matchTimes := 0 + // all acts conditions should be satisfied + for cond, vals := range evt.Acts() { + switch cond { + case "types": + // See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#registry_package + // Activity types with the same name: + // NONE + // Activity types need to be converted: + // created -> published + // Unsupported activity types: + // updated + + action := payload.Action + switch action { + case api.HookPackageCreated: + action = "published" + } + for _, val := range vals { + if glob.MustCompile(val, '/').Match(string(action)) { + matchTimes++ + break + } + } + default: + log.Warn("package event unsupported condition %q", cond) } } return matchTimes == len(evt.Acts()) diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go new file mode 100644 index 000000000..6724abafd --- /dev/null +++ b/modules/actions/workflows_test.go @@ -0,0 +1,105 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "testing" + + "code.gitea.io/gitea/modules/git" + api "code.gitea.io/gitea/modules/structs" + webhook_module "code.gitea.io/gitea/modules/webhook" + + "github.com/stretchr/testify/assert" +) + +func TestDetectMatched(t *testing.T) { + testCases := []struct { + desc string + commit *git.Commit + triggedEvent webhook_module.HookEventType + payload api.Payloader + yamlOn string + expected bool + }{ + { + desc: "HookEventCreate(create) matches githubEventCreate(create)", + triggedEvent: webhook_module.HookEventCreate, + payload: nil, + yamlOn: "on: create", + expected: true, + }, + { + desc: "HookEventIssues(issues) `opened` action matches githubEventIssues(issues)", + triggedEvent: webhook_module.HookEventIssues, + payload: &api.IssuePayload{Action: api.HookIssueOpened}, + yamlOn: "on: issues", + expected: true, + }, + { + desc: "HookEventIssues(issues) `milestoned` action matches githubEventIssues(issues)", + triggedEvent: webhook_module.HookEventIssues, + payload: &api.IssuePayload{Action: api.HookIssueMilestoned}, + yamlOn: "on: issues", + expected: true, + }, + { + desc: "HookEventPullRequestSync(pull_request_sync) matches githubEventPullRequest(pull_request)", + triggedEvent: webhook_module.HookEventPullRequestSync, + payload: &api.PullRequestPayload{Action: api.HookIssueSynchronized}, + yamlOn: "on: pull_request", + expected: true, + }, + { + desc: "HookEventPullRequest(pull_request) `label_updated` action doesn't match githubEventPullRequest(pull_request) with no activity type", + triggedEvent: webhook_module.HookEventPullRequest, + payload: &api.PullRequestPayload{Action: api.HookIssueLabelUpdated}, + yamlOn: "on: pull_request", + expected: false, + }, + { + desc: "HookEventPullRequest(pull_request) `label_updated` action matches githubEventPullRequest(pull_request) with `label` activity type", + triggedEvent: webhook_module.HookEventPullRequest, + payload: &api.PullRequestPayload{Action: api.HookIssueLabelUpdated}, + yamlOn: "on:\n pull_request:\n types: [labeled]", + expected: true, + }, + { + desc: "HookEventPullRequestReviewComment(pull_request_review_comment) matches githubEventPullRequestReviewComment(pull_request_review_comment)", + triggedEvent: webhook_module.HookEventPullRequestReviewComment, + payload: &api.PullRequestPayload{Action: api.HookIssueReviewed}, + yamlOn: "on:\n pull_request_review_comment:\n types: [created]", + expected: true, + }, + { + desc: "HookEventPullRequestReviewRejected(pull_request_review_rejected) doesn't match githubEventPullRequestReview(pull_request_review) with `dismissed` activity type (we don't support `dismissed` at present)", + triggedEvent: webhook_module.HookEventPullRequestReviewRejected, + payload: &api.PullRequestPayload{Action: api.HookIssueReviewed}, + yamlOn: "on:\n pull_request_review:\n types: [dismissed]", + expected: false, + }, + { + desc: "HookEventRelease(release) `published` action matches githubEventRelease(release) with `published` activity type", + triggedEvent: webhook_module.HookEventRelease, + payload: &api.ReleasePayload{Action: api.HookReleasePublished}, + yamlOn: "on:\n release:\n types: [published]", + expected: true, + }, + { + desc: "HookEventPackage(package) `created` action doesn't match githubEventRegistryPackage(registry_package) with `updated` activity type", + triggedEvent: webhook_module.HookEventPackage, + payload: &api.PackagePayload{Action: api.HookPackageCreated}, + yamlOn: "on:\n registry_package:\n types: [updated]", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + evts, err := GetEventsFromContent([]byte(tc.yamlOn)) + assert.NoError(t, err) + assert.Len(t, evts, 1) + assert.Equal(t, tc.expected, detectMatched(tc.commit, tc.triggedEvent, tc.payload, evts[0])) + }) + } +}