From 6390b12925448a264ab8e2ee042f1fa252964a1a Mon Sep 17 00:00:00 2001 From: Easton Crupper <65553218+ecrupper@users.noreply.github.com> Date: Tue, 28 Nov 2023 10:44:06 -0500 Subject: [PATCH] fix(auto_cancel): support canceling pull_request:opened and abstract determination logic (#1012) * init commit * add tests --- api/build/auto_cancel.go | 49 ++++++- api/build/auto_cancel_test.go | 268 ++++++++++++++++++++++++++++++++++ api/webhook/post.go | 15 +- 3 files changed, 311 insertions(+), 21 deletions(-) create mode 100644 api/build/auto_cancel_test.go diff --git a/api/build/auto_cancel.go b/api/build/auto_cancel.go index 9ffb32b26..1b9552389 100644 --- a/api/build/auto_cancel.go +++ b/api/build/auto_cancel.go @@ -27,13 +27,8 @@ func AutoCancel(c *gin.Context, b *library.Build, rB *library.Build, r *library. return false, nil } - // ensure criteria is met before auto canceling (push to same branch, or pull with same action from same head_ref) - if (strings.EqualFold(rB.GetEvent(), constants.EventPush) && - strings.EqualFold(b.GetEvent(), constants.EventPush) && - strings.EqualFold(b.GetBranch(), rB.GetBranch())) || - (strings.EqualFold(rB.GetEvent(), constants.EventPull) && - strings.EqualFold(b.GetEventAction(), rB.GetEventAction()) && - strings.EqualFold(b.GetHeadRef(), rB.GetHeadRef())) { + // ensure criteria is met + if isCancelable(rB, b) { switch { case strings.EqualFold(rB.GetStatus(), constants.StatusPending) && cancelOpts.Pending: // pending build will be handled gracefully by worker once pulled off queue @@ -185,3 +180,43 @@ func cancelRunning(c *gin.Context, b *library.Build, r *library.Repo) error { return nil } + +// isCancelable is a helper function that determines whether a `target` build should be auto-canceled +// given a current build that intends to supersede it. +func isCancelable(target *library.Build, current *library.Build) bool { + switch target.GetEvent() { + case constants.EventPush: + // target is cancelable if current build is also a push event and the branches are the same + return strings.EqualFold(current.GetEvent(), constants.EventPush) && strings.EqualFold(current.GetBranch(), target.GetBranch()) + case constants.EventPull: + cancelableAction := strings.EqualFold(target.GetEventAction(), constants.ActionOpened) || strings.EqualFold(target.GetEventAction(), constants.ActionSynchronize) + + // target is cancelable if current build is also a pull event, target is an opened / synchronize action, and the current head ref matches target head ref + return strings.EqualFold(current.GetEvent(), constants.EventPull) && cancelableAction && strings.EqualFold(current.GetHeadRef(), target.GetHeadRef()) + default: + return false + } +} + +// ShouldAutoCancel is a helper function that determines whether or not a build should be eligible to +// auto cancel currently running / pending builds. +func ShouldAutoCancel(opts *pipeline.CancelOptions, b *library.Build, defaultBranch string) bool { + // if anything is provided in the auto_cancel metadata, then we start with true + runAutoCancel := opts.Running || opts.Pending || opts.DefaultBranch + + switch b.GetEvent() { + case constants.EventPush: + // pushes to the default branch should only auto cancel if pipeline specifies default_branch: true + if !opts.DefaultBranch && strings.EqualFold(b.GetBranch(), defaultBranch) { + runAutoCancel = false + } + + return runAutoCancel + + case constants.EventPull: + // only synchronize actions of the pull_request event are eligible to auto cancel + return runAutoCancel && (strings.EqualFold(b.GetEventAction(), constants.ActionSynchronize)) + default: + return false + } +} diff --git a/api/build/auto_cancel_test.go b/api/build/auto_cancel_test.go new file mode 100644 index 000000000..0c90c091d --- /dev/null +++ b/api/build/auto_cancel_test.go @@ -0,0 +1,268 @@ +// SPDX-License-Identifier: Apache-2.0 + +package build + +import ( + "testing" + + "github.com/go-vela/types/constants" + "github.com/go-vela/types/library" + "github.com/go-vela/types/pipeline" +) + +func Test_isCancelable(t *testing.T) { + // setup types + pushEvent := constants.EventPush + pullEvent := constants.EventPull + tagEvent := constants.EventTag + + branchDev := "dev" + branchPatch := "patch-1" + + actionOpened := constants.ActionOpened + actionSync := constants.ActionSynchronize + actionEdited := constants.ActionEdited + + tests := []struct { + name string + target *library.Build + current *library.Build + want bool + }{ + { + name: "Wrong Event", + target: &library.Build{ + Event: &tagEvent, + Branch: &branchDev, + }, + current: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + want: false, + }, + { + name: "Cancelable Push", + target: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + current: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + want: true, + }, + { + name: "Push Branch Mismatch", + target: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + current: &library.Build{ + Event: &pushEvent, + Branch: &branchPatch, + }, + want: false, + }, + { + name: "Event Mismatch", + target: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + current: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchPatch, + }, + want: false, + }, + { + name: "Cancelable Pull", + target: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchPatch, + EventAction: &actionOpened, + }, + current: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchPatch, + EventAction: &actionSync, + }, + want: true, + }, + { + name: "Pull Head Ref Mismatch", + target: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchPatch, + EventAction: &actionSync, + }, + current: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchDev, + EventAction: &actionSync, + }, + want: false, + }, + { + name: "Pull Ineligible Action", + target: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchPatch, + EventAction: &actionEdited, + }, + current: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + HeadRef: &branchDev, + EventAction: &actionSync, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isCancelable(tt.target, tt.current); got != tt.want { + t.Errorf("test %s: isCancelable() = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + +func Test_ShouldAutoCancel(t *testing.T) { + // setup types + pushEvent := constants.EventPush + pullEvent := constants.EventPull + tagEvent := constants.EventTag + + branchDev := "dev" + branchPatch := "patch-1" + + actionOpened := constants.ActionOpened + actionSync := constants.ActionSynchronize + + tests := []struct { + name string + opts *pipeline.CancelOptions + build *library.Build + branch string + want bool + }{ + { + name: "Wrong Event", + opts: &pipeline.CancelOptions{ + Running: true, + Pending: true, + DefaultBranch: true, + }, + build: &library.Build{ + Event: &tagEvent, + Branch: &branchPatch, + }, + branch: branchDev, + want: false, + }, + { + name: "Auto Cancel Disabled", + opts: &pipeline.CancelOptions{ + Running: false, + Pending: false, + DefaultBranch: false, + }, + build: &library.Build{ + Event: &pushEvent, + Branch: &branchPatch, + }, + branch: branchDev, + want: false, + }, + { + name: "Eligible Push", + opts: &pipeline.CancelOptions{ + Running: true, + Pending: true, + DefaultBranch: false, + }, + build: &library.Build{ + Event: &pushEvent, + Branch: &branchPatch, + }, + branch: branchDev, + want: true, + }, + { + name: "Eligible Push - Default Branch", + opts: &pipeline.CancelOptions{ + Running: true, + Pending: true, + DefaultBranch: true, + }, + build: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + branch: branchDev, + want: true, + }, + { + name: "Push Mismatch - Default Branch", + opts: &pipeline.CancelOptions{ + Running: true, + Pending: true, + DefaultBranch: false, + }, + build: &library.Build{ + Event: &pushEvent, + Branch: &branchDev, + }, + branch: branchDev, + want: false, + }, + { + name: "Eligible Pull", + opts: &pipeline.CancelOptions{ + Running: true, + Pending: true, + DefaultBranch: false, + }, + build: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + EventAction: &actionSync, + }, + branch: branchDev, + want: true, + }, + { + name: "Pull Mismatch - Action", + opts: &pipeline.CancelOptions{ + Running: true, + Pending: true, + DefaultBranch: false, + }, + build: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + EventAction: &actionOpened, + }, + branch: branchDev, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ShouldAutoCancel(tt.opts, tt.build, tt.branch); got != tt.want { + t.Errorf("test %s: ShouldAutoCancel() = %v, want %v", tt.name, got, tt.want) + } + }) + } +} diff --git a/api/webhook/post.go b/api/webhook/post.go index c7b3dad9d..d816431e0 100644 --- a/api/webhook/post.go +++ b/api/webhook/post.go @@ -682,20 +682,7 @@ func PostWebhook(c *gin.Context) { u, ) - // if anything is provided in the auto_cancel metadata, then we start with true - runAutoCancel := p.Metadata.AutoCancel.Running || p.Metadata.AutoCancel.Pending || p.Metadata.AutoCancel.DefaultBranch - - // if the event is a push to the default branch and the AutoCancel.DefaultBranch value is false, bypass auto cancel - if strings.EqualFold(b.GetEvent(), constants.EventPush) && strings.EqualFold(b.GetBranch(), repo.GetBranch()) && !p.Metadata.AutoCancel.DefaultBranch { - runAutoCancel = false - } - - // if event is push or pull_request:synchronize, there is a chance this build could be superceding a stale build - // - // fetch pending and running builds for this repo in order to validate their merit to continue running. - if runAutoCancel && - ((strings.EqualFold(b.GetEvent(), constants.EventPull) && strings.EqualFold(b.GetEventAction(), constants.ActionSynchronize)) || - strings.EqualFold(b.GetEvent(), constants.EventPush)) { + if build.ShouldAutoCancel(p.Metadata.AutoCancel, b, repo.GetBranch()) { // fetch pending and running builds rBs, err := database.FromContext(c).ListPendingAndRunningBuildsForRepo(c, repo) if err != nil {