From 4a5ce4adaccdc7de9f010508b11e4755efd5cd6e Mon Sep 17 00:00:00 2001 From: Easton Crupper <65553218+ecrupper@users.noreply.github.com> Date: Fri, 23 Feb 2024 09:54:55 -0500 Subject: [PATCH] enhance: auto cancel builds pending approval when they become obsolete (#1066) --- api/build/auto_cancel.go | 15 +++++-- api/build/auto_cancel_test.go | 18 ++++++++ api/webhook/post.go | 45 ++++++++++--------- database/build/list_pending_running_repo.go | 2 +- .../build/list_pending_running_repo_test.go | 2 +- 5 files changed, 56 insertions(+), 26 deletions(-) diff --git a/api/build/auto_cancel.go b/api/build/auto_cancel.go index 1b9552389..0f200498c 100644 --- a/api/build/auto_cancel.go +++ b/api/build/auto_cancel.go @@ -27,10 +27,14 @@ func AutoCancel(c *gin.Context, b *library.Build, rB *library.Build, r *library. return false, nil } + status := rB.GetStatus() + // ensure criteria is met if isCancelable(rB, b) { switch { - case strings.EqualFold(rB.GetStatus(), constants.StatusPending) && cancelOpts.Pending: + case strings.EqualFold(status, constants.StatusPendingApproval) || + (strings.EqualFold(status, constants.StatusPending) && + cancelOpts.Pending): // pending build will be handled gracefully by worker once pulled off queue rB.SetStatus(constants.StatusCanceled) @@ -44,7 +48,7 @@ func AutoCancel(c *gin.Context, b *library.Build, rB *library.Build, r *library. if err != nil { return true, err } - case strings.EqualFold(rB.GetStatus(), constants.StatusRunning) && cancelOpts.Running: + case strings.EqualFold(status, constants.StatusRunning) && cancelOpts.Running: // call cancelRunning routine for builds already running on worker err := cancelRunning(c, rB, r) if err != nil { @@ -55,7 +59,7 @@ func AutoCancel(c *gin.Context, b *library.Build, rB *library.Build, r *library. } // set error message that references current build - rB.SetError(fmt.Sprintf("build was auto canceled in favor of build %d", b.GetNumber())) + rB.SetError(fmt.Sprintf("%s build was auto canceled in favor of build %d", status, b.GetNumber())) _, err := database.FromContext(c).UpdateBuild(c, rB) if err != nil { @@ -201,6 +205,11 @@ func isCancelable(target *library.Build, current *library.Build) bool { // 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 the build is pending approval, it should always be eligible to auto cancel + if strings.EqualFold(b.GetStatus(), constants.StatusPendingApproval) { + return true + } + // if anything is provided in the auto_cancel metadata, then we start with true runAutoCancel := opts.Running || opts.Pending || opts.DefaultBranch diff --git a/api/build/auto_cancel_test.go b/api/build/auto_cancel_test.go index 0c90c091d..23879b923 100644 --- a/api/build/auto_cancel_test.go +++ b/api/build/auto_cancel_test.go @@ -146,6 +146,8 @@ func Test_ShouldAutoCancel(t *testing.T) { branchDev := "dev" branchPatch := "patch-1" + statusPendingApproval := constants.StatusPendingApproval + actionOpened := constants.ActionOpened actionSync := constants.ActionSynchronize @@ -256,6 +258,22 @@ func Test_ShouldAutoCancel(t *testing.T) { branch: branchDev, want: false, }, + { + name: "Pending Approval Build", + opts: &pipeline.CancelOptions{ + Running: false, + Pending: false, + DefaultBranch: false, + }, + build: &library.Build{ + Event: &pullEvent, + Branch: &branchDev, + EventAction: &actionOpened, + Status: &statusPendingApproval, + }, + branch: branchDev, + want: true, + }, } for _, tt := range tests { diff --git a/api/webhook/post.go b/api/webhook/post.go index 965afeb72..038d53fe7 100644 --- a/api/webhook/post.go +++ b/api/webhook/post.go @@ -736,6 +736,30 @@ func PostWebhook(c *gin.Context) { return } + // regardless of whether the build is published to queue, we want to attempt to auto-cancel if no errors + defer func() { + if err == nil && build.ShouldAutoCancel(p.Metadata.AutoCancel, b, repo.GetBranch()) { + // fetch pending and running builds + rBs, err := database.FromContext(c).ListPendingAndRunningBuildsForRepo(c, repo) + if err != nil { + logrus.Errorf("unable to fetch pending and running builds for %s: %v", repo.GetFullName(), err) + } + + for _, rB := range rBs { + // call auto cancel routine + canceled, err := build.AutoCancel(c, b, rB, repo, p.Metadata.AutoCancel) + if err != nil { + // continue cancel loop if error, but log based on type of error + if canceled { + logrus.Errorf("unable to update canceled build error message: %v", err) + } else { + logrus.Errorf("unable to cancel running build: %v", err) + } + } + } + } + }() + // if the webhook was from a Pull event from a forked repository, verify it is allowed to run if webhook.PullRequest.IsFromFork { switch repo.GetApproveBuild() { @@ -802,27 +826,6 @@ func PostWebhook(c *gin.Context) { u, route, ) - - 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 { - logrus.Errorf("unable to fetch pending and running builds for %s: %v", repo.GetFullName(), err) - } - - for _, rB := range rBs { - // call auto cancel routine - canceled, err := build.AutoCancel(c, b, rB, repo, p.Metadata.AutoCancel) - if err != nil { - // continue cancel loop if error, but log based on type of error - if canceled { - logrus.Errorf("unable to update canceled build error message: %v", err) - } else { - logrus.Errorf("unable to cancel running build: %v", err) - } - } - } - } } // handleRepositoryEvent is a helper function that processes repository events from the SCM and updates diff --git a/database/build/list_pending_running_repo.go b/database/build/list_pending_running_repo.go index 3c82ca419..ad0963aee 100644 --- a/database/build/list_pending_running_repo.go +++ b/database/build/list_pending_running_repo.go @@ -23,7 +23,7 @@ func (e *engine) ListPendingAndRunningBuildsForRepo(ctx context.Context, repo *l Table(constants.TableBuild). Select("*"). Where("repo_id = ?", repo.GetID()). - Where("status = 'running' OR status = 'pending'"). + Where("status = 'running' OR status = 'pending' OR status = 'pending approval'"). Find(&b). Error if err != nil { diff --git a/database/build/list_pending_running_repo_test.go b/database/build/list_pending_running_repo_test.go index f761b2ddb..34993713b 100644 --- a/database/build/list_pending_running_repo_test.go +++ b/database/build/list_pending_running_repo_test.go @@ -67,7 +67,7 @@ func TestBuild_Engine_ListPendingAndRunningBuildsForRepo(t *testing.T) { AddRow(1, 1, nil, 1, 0, "", "", "running", "", 0, 1, 0, 0, "", nil, "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", 0, "", 0) // ensure the mock expects the name query - _mock.ExpectQuery(`SELECT * FROM "builds" WHERE repo_id = $1 AND (status = 'running' OR status = 'pending')`).WithArgs(1).WillReturnRows(_rows) + _mock.ExpectQuery(`SELECT * FROM "builds" WHERE repo_id = $1 AND (status = 'running' OR status = 'pending' OR status = 'pending approval')`).WithArgs(1).WillReturnRows(_rows) _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }()