From 4d680ceb5f71daf42fbe8272952255a1e8c93753 Mon Sep 17 00:00:00 2001 From: Aleksandar Aleksandrov Date: Tue, 14 Jun 2022 18:07:11 +0100 Subject: [PATCH] Fix multiple jobs running on PullRequest event --- .gitignore | 3 +- .../processPullRequestEvent_noRerun.json | 14 ++++++- .../processPullRequestEvent_rerun.golden | 27 ++++++++++++- .../processPullRequestEvent_rerun.json | 5 +-- plugins/github-integration/main.go | 40 +++++++++++-------- plugins/github-integration/main_test.go | 6 +-- 6 files changed, 68 insertions(+), 27 deletions(-) diff --git a/.gitignore b/.gitignore index 99356b1..5b81bb3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ dist/ -**/rice-box.go \ No newline at end of file +**/rice-box.go +.idea \ No newline at end of file diff --git a/plugins/github-integration/fixtures/processPullRequestEvent_noRerun.json b/plugins/github-integration/fixtures/processPullRequestEvent_noRerun.json index 1e9fa3a..512b85b 100644 --- a/plugins/github-integration/fixtures/processPullRequestEvent_noRerun.json +++ b/plugins/github-integration/fixtures/processPullRequestEvent_noRerun.json @@ -34,7 +34,7 @@ "type": "User", "site_admin": false }, - "body": "- [x] /werft with-preview", + "body": "- [ ] /werft with-preview", "created_at": "2021-07-06T13:03:07Z", "updated_at": "2022-06-01T19:37:58Z", "closed_at": null, @@ -354,7 +354,7 @@ }, "changes": { "body": { - "from": "- [x] /werft with-preview" + "from": "- [ ] /werft with-preview" } }, "repository": { @@ -481,6 +481,16 @@ } }, "ListResponse": [ + { + "Metadata": { + "Repository": { + "Revision": "we're-skipping-this-one" + }, + "Annotations": [ + {"Key": "and-this", "Value": "doesn't-matter"} + ] + } + }, { "Metadata": { "Repository": { diff --git a/plugins/github-integration/fixtures/processPullRequestEvent_rerun.golden b/plugins/github-integration/fixtures/processPullRequestEvent_rerun.golden index 5aae393..01d74e9 100644 --- a/plugins/github-integration/fixtures/processPullRequestEvent_rerun.golden +++ b/plugins/github-integration/fixtures/processPullRequestEvent_rerun.golden @@ -1 +1,26 @@ -{"metadata":{"owner":"csweichel","repository":{"host":"github.com","owner":"csweichel","repo":"test-repo","ref":"refs/heads/cannot-init","revision":"eed6dfe4dad7e3519acf52679b9880c5d2a2afbf","defaultBranch":"master"},"trigger":"TRIGGER_MANUAL","annotations":[{"key":"updateGitHubStatus","value":"csweichel/test-repo"},{"key":"with-preview"}]},"spec":{"jobPath":""}} \ No newline at end of file +{ + "metadata": { + "owner": "csweichel", + "repository": { + "host": "github.com", + "owner": "csweichel", + "repo": "test-repo", + "ref": "refs/heads/cannot-init", + "revision": "eed6dfe4dad7e3519acf52679b9880c5d2a2afbf", + "defaultBranch": "master" + }, + "trigger": "TRIGGER_MANUAL", + "annotations": [ + { + "key": "updateGitHubStatus", + "value": "csweichel/test-repo" + }, + { + "key": "with-preview" + } + ] + }, + "spec": { + "jobPath": "" + } +} \ No newline at end of file diff --git a/plugins/github-integration/fixtures/processPullRequestEvent_rerun.json b/plugins/github-integration/fixtures/processPullRequestEvent_rerun.json index b930655..1e9fa3a 100644 --- a/plugins/github-integration/fixtures/processPullRequestEvent_rerun.json +++ b/plugins/github-integration/fixtures/processPullRequestEvent_rerun.json @@ -485,10 +485,7 @@ "Metadata": { "Repository": { "Revision": "eed6dfe4dad7e3519acf52679b9880c5d2a2afbf" - }, - "Annotations": [ - {"Key": "with-preview", "Value": ""} - ] + } } } ] diff --git a/plugins/github-integration/main.go b/plugins/github-integration/main.go index e6ed845..da8b3c1 100644 --- a/plugins/github-integration/main.go +++ b/plugins/github-integration/main.go @@ -283,7 +283,7 @@ func (p *githubTriggerPlugin) HandleGithubWebhook(w http.ResponseWriter, r *http case *github.IssueCommentEvent: p.processIssueCommentEvent(r.Context(), event) case *github.PullRequestEvent: - p.processPullRequestEvent(r.Context(), event) + p.processPullRequestEditedEvent(r.Context(), event) case *github.DeleteEvent: // handled by the push event already default: @@ -292,8 +292,15 @@ func (p *githubTriggerPlugin) HandleGithubWebhook(w http.ResponseWriter, r *http } } -func (p *githubTriggerPlugin) processPullRequestEvent(ctx context.Context, event *github.PullRequestEvent) { +func (p *githubTriggerPlugin) processPullRequestEditedEvent(ctx context.Context, event *github.PullRequestEvent) { pr := event.PullRequest + // Potentially (depending on the webhook configuration) we'll get multiple PR events - when pushing (every push generates a "synchronize" pull_request event if there's a PR open), + // adding/removing a label,reviewer, assigning/unassigning, etc. The only time annotations are relevant are when a PRs description is edited (opening a PR is handled by a different flow) + // So this is the only one we process at this time, the rest we discard. + if event.GetAction() != "edited" { + return + } + ref := pr.GetHead().GetRef() if !strings.HasPrefix(ref, "refs/") { // we assume this is a branch @@ -317,26 +324,27 @@ func (p *githubTriggerPlugin) processPullRequestEvent(ctx context.Context, event return } - annotations := repo.ParseAnnotations(pr.GetBody()) + prAnnotations := repo.ParseAnnotations(pr.GetBody()) - var noReRun bool - for _, j := range lastJobs.Result { - if j.Metadata.Repository.Revision != rev { + // We only care about the last job that ran for the same commit as that of the PR Event + for _, lastJob := range lastJobs.Result { + if lastJob.Metadata.Repository.Revision != rev { continue } - ja := make(map[string]string, len(j.Metadata.Annotations)) - for _, a := range j.Metadata.Annotations { - ja[a.Key] = a.Value + jobAnnotations := make(map[string]string, len(lastJob.Metadata.Annotations)) + for _, a := range lastJob.Metadata.Annotations { + jobAnnotations[a.Key] = a.Value } - if !mapEQ(annotations, ja) { - noReRun = true - break + // If the annotations didn't change between the last job and the event we're currently processing we have nothing to do + if mapEQ(prAnnotations, jobAnnotations) { + return } - } - if noReRun { - return + + // If we got here, it means that the annotations changed, so we should continue and launch a job with the new annotations + // Also we don't care for the rest of the "previous" jobs, so we discard by breaking out of the loop + break } var ( @@ -347,7 +355,7 @@ func (p *githubTriggerPlugin) processPullRequestEvent(ctx context.Context, event ) if p.userIsAllowedToStartJob(ctx, prDstOwner, prDstRepo, event.GetSender().GetLogin()) { req := p.prepareStartJobRequest(event.Sender, event.Repo.Owner, event.Repo.Owner, event.Repo, event.Repo, ref, rev, v1.JobTrigger_TRIGGER_MANUAL) - for k, v := range annotations { + for k, v := range prAnnotations { req.Metadata.Annotations = append(req.Metadata.Annotations, &v1.Annotation{ Key: k, Value: v, diff --git a/plugins/github-integration/main_test.go b/plugins/github-integration/main_test.go index 817e1a7..7a80e35 100644 --- a/plugins/github-integration/main_test.go +++ b/plugins/github-integration/main_test.go @@ -257,7 +257,7 @@ func TestProcessPushEvent(t *testing.T) { } } -func TestProcessPullRequestEvent(t *testing.T) { +func TestProcessPullRequestEditedEvent(t *testing.T) { type Fixture struct { Event *github.PullRequestEvent ListResponse []*v1.JobStatus @@ -319,7 +319,7 @@ func TestProcessPullRequestEvent(t *testing.T) { if fixture.Event == nil { t.Fatal("broken fixture: no event") } - plg.processPullRequestEvent(context.Background(), fixture.Event) + plg.processPullRequestEditedEvent(context.Background(), fixture.Event) var expectation *v1.StartJobRequest2 goldenFN := strings.TrimSuffix(fn, filepath.Ext(fn)) + ".golden" @@ -349,7 +349,7 @@ func TestProcessPullRequestEvent(t *testing.T) { } if diff := cmp.Diff(expectation, startReq); diff != "" { - t.Errorf("processPullRequestEvent() mismatch (-want +got):\n%s", diff) + t.Errorf("processPullRequestEditedEvent() mismatch (-want +got):\n%s", diff) } }) }