From 58e3c1fbdba0832dc8cbe3f9adca3674f33f70c7 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. --- 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 56afed6ee6..a41e172253 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -363,6 +363,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. @@ -400,6 +401,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 66c85b7d8b40d4c1c504b01ab70b6282059b1c21 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). --- 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 f56fc51c74af9cff436abc217309dadaa610dc30 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 --- 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).