From 11f71dcb09f8657de62c292416b2f50e2a770679 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 3 Nov 2024 22:55:06 +0100 Subject: [PATCH 1/3] fix: add label to issues and PR labeled/unlabeled events When a workflow has on: pull_request: types: - labeled - unlabeled The payload misses the label field describing the added or removed label. The unlabeled event type was also incorrectly mapped to the labeled event type. (cherry picked from commit 58e3c1fbdba0832dc8cbe3f9adca3674f33f70c7) --- modules/structs/hook.go | 2 ++ services/actions/notifier.go | 22 +++++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/modules/structs/hook.go b/modules/structs/hook.go index b7f8861b76..1940fd8dc3 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -362,6 +362,7 @@ type IssuePayload struct { Repository *Repository `json:"repository"` Sender *User `json:"sender"` CommitID string `json:"commit_id"` + Label *Label `json:"label,omitempty"` } // JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces. @@ -399,6 +400,7 @@ type PullRequestPayload struct { Sender *User `json:"sender"` CommitID string `json:"commit_id"` Review *ReviewPayload `json:"review"` + Label *Label `json:"label,omitempty"` } // JSONPayload FIXME diff --git a/services/actions/notifier.go b/services/actions/notifier.go index e97afad990..2dd81158a7 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -168,7 +168,7 @@ func (n *actionsNotifier) IssueChangeAssignee(ctx context.Context, doer *user_mo hookEvent = webhook_module.HookEventPullRequestAssign } - notifyIssueChange(ctx, doer, issue, hookEvent, action) + notifyIssueChange(ctx, doer, issue, hookEvent, action, nil) } // IssueChangeMilestone notifies assignee to notifiers @@ -187,11 +187,11 @@ func (n *actionsNotifier) IssueChangeMilestone(ctx context.Context, doer *user_m hookEvent = webhook_module.HookEventPullRequestMilestone } - notifyIssueChange(ctx, doer, issue, hookEvent, action) + notifyIssueChange(ctx, doer, issue, hookEvent, action, nil) } func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, - _, _ []*issues_model.Label, + addedLabels, removedLabels []*issues_model.Label, ) { ctx = withMethod(ctx, "IssueChangeLabels") @@ -200,10 +200,15 @@ func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_mode hookEvent = webhook_module.HookEventPullRequestLabel } - notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated) + for _, added := range addedLabels { + notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated, added) + } + for _, removed := range removedLabels { + notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelCleared, removed) + } } -func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, event webhook_module.HookEventType, action api.HookIssueAction) { +func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, event webhook_module.HookEventType, action api.HookIssueAction, label *issues_model.Label) { var err error if err = issue.LoadRepo(ctx); err != nil { log.Error("LoadRepo: %v", err) @@ -215,6 +220,11 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues return } + var apiLabel *api.Label + if action == api.HookIssueLabelUpdated || action == api.HookIssueLabelCleared { + apiLabel = convert.ToLabel(label, issue.Repo, nil) + } + if issue.IsPull { if err = issue.LoadPullRequest(ctx); err != nil { log.Error("loadPullRequest: %v", err) @@ -228,6 +238,7 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, nil), Repository: convert.ToRepo(ctx, issue.Repo, access_model.Permission{AccessMode: perm_model.AccessModeNone}), Sender: convert.ToUser(ctx, doer, nil), + Label: apiLabel, }). WithPullRequest(issue.PullRequest). Notify(ctx) @@ -242,6 +253,7 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues Issue: convert.ToAPIIssue(ctx, doer, issue), Repository: convert.ToRepo(ctx, issue.Repo, permission), Sender: convert.ToUser(ctx, doer, nil), + Label: apiLabel, }). Notify(ctx) } From d624a5edd6a34cecb0109165e9d3fd400b4a7615 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 3 Nov 2024 23:03:19 +0100 Subject: [PATCH 2/3] fix: Actions PR workflows must update the commit status When a workflow has on: pull_request: types: - labeled - unlabeled The outcome of the workflow (success or failure) must be associated with the head sha commit status. Otherwise it cannot be used as a requirement for merging the pull request (branch protections). (cherry picked from commit 66c85b7d8b40d4c1c504b01ab70b6282059b1c21) --- models/actions/run.go | 6 +- services/actions/commit_status.go | 2 +- tests/integration/actions_trigger_test.go | 236 ++++++++++++++++++++++ 3 files changed, 242 insertions(+), 2 deletions(-) diff --git a/models/actions/run.go b/models/actions/run.go index 8b40cb7ba8..f637634575 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -146,7 +146,11 @@ func (run *ActionRun) GetPushEventPayload() (*api.PushPayload, error) { } func (run *ActionRun) GetPullRequestEventPayload() (*api.PullRequestPayload, error) { - if run.Event == webhook_module.HookEventPullRequest || run.Event == webhook_module.HookEventPullRequestSync { + if run.Event == webhook_module.HookEventPullRequest || + run.Event == webhook_module.HookEventPullRequestSync || + run.Event == webhook_module.HookEventPullRequestAssign || + run.Event == webhook_module.HookEventPullRequestMilestone || + run.Event == webhook_module.HookEventPullRequestLabel { var payload api.PullRequestPayload if err := json.Unmarshal([]byte(run.EventPayload), &payload); err != nil { return nil, err diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 2698059e94..04dffbac88 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -53,7 +53,7 @@ func createCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er return fmt.Errorf("head commit is missing in event payload") } sha = payload.HeadCommit.ID - case webhook_module.HookEventPullRequest, webhook_module.HookEventPullRequestSync: + case webhook_module.HookEventPullRequest, webhook_module.HookEventPullRequestSync, webhook_module.HookEventPullRequestLabel, webhook_module.HookEventPullRequestAssign, webhook_module.HookEventPullRequestMilestone: if run.TriggerEvent == actions_module.GithubEventPullRequestTarget { event = "pull_request_target" } else { diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index de646f05b5..c57a64e7dd 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -5,12 +5,14 @@ package integration import ( "fmt" + "net/http" "net/url" "strings" "testing" "time" actions_model "code.gitea.io/gitea/models/actions" + auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" @@ -22,9 +24,11 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/setting" + api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" webhook_module "code.gitea.io/gitea/modules/webhook" actions_service "code.gitea.io/gitea/services/actions" + issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" release_service "code.gitea.io/gitea/services/release" repo_service "code.gitea.io/gitea/services/repository" @@ -35,6 +39,238 @@ import ( "github.com/stretchr/testify/require" ) +func TestPullRequestCommitStatus(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the base repo + session := loginUser(t, "user2") + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) + + // prepare the repository + files := make([]*files_service.ChangeRepoFile, 0, 10) + for _, onType := range []string{ + "opened", + "synchronize", + "labeled", + "unlabeled", + "assigned", + "unassigned", + "milestoned", + "demilestoned", + "closed", + "reopened", + } { + files = append(files, &files_service.ChangeRepoFile{ + Operation: "create", + TreePath: fmt.Sprintf(".forgejo/workflows/%s.yml", onType), + ContentReader: strings.NewReader(fmt.Sprintf(`name: %[1]s +on: + pull_request: + types: + - %[1]s +jobs: + %[1]s: + runs-on: docker + steps: + - run: true +`, onType)), + }) + } + baseRepo, _, f := tests.CreateDeclarativeRepo(t, user2, "repo-pull-request", + []unit_model.Type{unit_model.TypeActions}, nil, files) + defer f() + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) + require.NoError(t, err) + defer func() { + baseGitRepo.Close() + }() + + // prepare the pull request + testEditFileToNewBranch(t, session, "user2", "repo-pull-request", "main", "wip-something", "README.md", "Hello, world 1") + testPullCreate(t, session, "user2", "repo-pull-request", true, "main", "wip-something", "Commit status PR") + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: baseRepo.ID}) + require.NoError(t, pr.LoadIssue(db.DefaultContext)) + + // prepare the assignees + issueURL := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%s", "user2", "repo-pull-request", fmt.Sprintf("%d", pr.Issue.Index)) + + // prepare the labels + labelStr := "/api/v1/repos/user2/repo-pull-request/labels" + req := NewRequestWithJSON(t, "POST", labelStr, &api.CreateLabelOption{ + Name: "mylabel", + Color: "abcdef", + Description: "description mylabel", + }).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusCreated) + label := new(api.Label) + DecodeJSON(t, resp, &label) + labelURL := fmt.Sprintf("%s/labels", issueURL) + + // prepare the milestone + milestoneStr := "/api/v1/repos/user2/repo-pull-request/milestones" + req = NewRequestWithJSON(t, "POST", milestoneStr, &api.CreateMilestoneOption{ + Title: "mymilestone", + State: "open", + }).AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusCreated) + milestone := new(api.Milestone) + DecodeJSON(t, resp, &milestone) + + // check that one of the status associated with the commit sha matches both + // context & state + checkCommitStatus := func(sha, context string, state api.CommitStatusState) bool { + commitStatuses, _, err := git_model.GetLatestCommitStatus(db.DefaultContext, pr.BaseRepoID, sha, db.ListOptionsAll) + require.NoError(t, err) + for _, commitStatus := range commitStatuses { + if state == commitStatus.State && context == commitStatus.Context { + return true + } + } + return false + } + + count := 0 + for _, testCase := range []struct { + onType string + jobID string + doSomething func() + action api.HookIssueAction + hasLabel bool + }{ + { + onType: "opened", + doSomething: func() {}, + action: api.HookIssueOpened, + }, + { + onType: "synchronize", + doSomething: func() { + testEditFile(t, session, "user2", "repo-pull-request", "wip-something", "README.md", "Hello, world 2") + }, + action: api.HookIssueSynchronized, + }, + { + onType: "labeled", + doSomething: func() { + req := NewRequestWithJSON(t, "POST", labelURL, &api.IssueLabelsOption{ + Labels: []any{label.ID}, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusOK) + }, + action: api.HookIssueLabelUpdated, + hasLabel: true, + }, + { + onType: "unlabeled", + doSomething: func() { + req := NewRequestWithJSON(t, "PUT", labelURL, &api.IssueLabelsOption{ + Labels: []any{}, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusOK) + }, + action: api.HookIssueLabelCleared, + hasLabel: true, + }, + { + onType: "assigned", + doSomething: func() { + req := NewRequestWithJSON(t, "PATCH", issueURL, &api.EditIssueOption{ + Assignees: []string{"user2"}, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + }, + action: api.HookIssueAssigned, + }, + { + onType: "unassigned", + doSomething: func() { + req := NewRequestWithJSON(t, "PATCH", issueURL, &api.EditIssueOption{ + Assignees: []string{}, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + }, + action: api.HookIssueUnassigned, + }, + { + onType: "milestoned", + doSomething: func() { + req := NewRequestWithJSON(t, "PATCH", issueURL, &api.EditIssueOption{ + Milestone: &milestone.ID, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + }, + action: api.HookIssueMilestoned, + }, + { + onType: "demilestoned", + doSomething: func() { + var zero int64 + req := NewRequestWithJSON(t, "PATCH", issueURL, &api.EditIssueOption{ + Milestone: &zero, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + }, + action: api.HookIssueDemilestoned, + }, + { + onType: "closed", + doSomething: func() { + sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + require.NoError(t, err) + err = issue_service.ChangeStatus(db.DefaultContext, pr.Issue, user2, sha, true) + require.NoError(t, err) + }, + action: api.HookIssueClosed, + }, + { + onType: "reopened", + doSomething: func() { + sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + require.NoError(t, err) + err = issue_service.ChangeStatus(db.DefaultContext, pr.Issue, user2, sha, false) + require.NoError(t, err) + }, + action: api.HookIssueReOpened, + }, + } { + t.Run(testCase.onType, func(t *testing.T) { + // trigger the onType event + testCase.doSomething() + count++ + context := fmt.Sprintf("%[1]s / %[1]s (pull_request)", testCase.onType) + + // wait for a new ActionRun to be created + assert.Eventually(t, func() bool { + return count == unittest.GetCount(t, &actions_model.ActionRun{RepoID: baseRepo.ID}) + }, 30*time.Second, 1*time.Second) + + // verify the expected ActionRun was created + sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + require.NoError(t, err) + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, WorkflowID: fmt.Sprintf("%s.yml", testCase.onType)}) + + assert.Equal(t, sha, actionRun.CommitSHA) + assert.Equal(t, actions_module.GithubEventPullRequest, actionRun.TriggerEvent) + event, err := actionRun.GetPullRequestEventPayload() + if testCase.hasLabel { + assert.NotNil(t, event.Label) + } + require.NoError(t, err) + assert.Equal(t, testCase.action, event.Action) + + // verify the expected ActionRunJob was created and is StatusWaiting + job := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{JobID: testCase.onType, CommitSHA: sha}) + assert.Equal(t, actions_model.StatusWaiting, job.Status) + + // verify the commit status changes to CommitStatusSuccess when the job changes to StatusSuccess + assert.True(t, checkCommitStatus(sha, context, api.CommitStatusPending)) + job.Status = actions_model.StatusSuccess + actions_service.CreateCommitStatus(db.DefaultContext, job) + assert.True(t, checkCommitStatus(sha, context, api.CommitStatusSuccess)) + }) + } + }) +} + func TestPullRequestTargetEvent(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the base repo From 8a65c4d28d448e467cbec63ed24db73d1747ddb1 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 4 Nov 2024 12:10:15 +0100 Subject: [PATCH 3/3] chore(release-notes): related pull requests workflow fixes (cherry picked from commit f56fc51c74af9cff436abc217309dadaa610dc30) --- release-notes/5778.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 release-notes/5778.md diff --git a/release-notes/5778.md b/release-notes/5778.md new file mode 100644 index 0000000000..a3c6305c57 --- /dev/null +++ b/release-notes/5778.md @@ -0,0 +1,3 @@ +fix: In a Forgejo Actions workflow, the `unlabeled` event type for pull requests was incorrectly mapped to the labeled event type. +fix: When a Forgejo Actions issue or pull request workflow is triggered by an `labeled` or `unlabeled` event type, it misses information about the label added or removed. It is now available in the `label` data member of the event payload. +fix: The pull request workflow must always update the head SHA commit status. Not just when the PR is synchronized, opened or closed. Otherwise it makes it impossible to define a job to be a required check (for instance a job that is triggered when labels are modified and verifies that a given combination is present).