From bb974377a75d87a5de3b0dcd4b7c4ceedbaa3f0b Mon Sep 17 00:00:00 2001 From: Brad Rydzewski Date: Tue, 30 Jul 2019 21:20:28 -0700 Subject: [PATCH] improve cancel to force cancel dangling stages --- CHANGELOG.md | 3 + handler/api/repos/builds/cancel.go | 124 +++++++++++++++-------------- version/version.go | 2 +- version/version_test.go | 2 +- 4 files changed, 68 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7966c01..755fa83e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## Unreleased + +## [1.2.3] - 2019-07-30 ### Added - disable github status for cron jobs @@ -12,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- improve cancel logic for dangling stages, by [@bradrydzewski](https://github.com/bradrydzewski). - improve error when kubernetes malforms the port configuration, by [@bradrydzewski](https://github.com/bradrydzewski). [#2742](https://github.com/drone/drone/issues/2742). - copy parameters from parent build when promoting, by [@bradrydzewski](https://github.com/bradrydzewski). [#2748](https://github.com/drone/drone/issues/2748). diff --git a/handler/api/repos/builds/cancel.go b/handler/api/repos/builds/cancel.go index b1257232..04c9943c 100644 --- a/handler/api/repos/builds/cancel.go +++ b/handler/api/repos/builds/cancel.go @@ -16,7 +16,6 @@ package builds import ( "context" - "errors" "net/http" "strconv" "time" @@ -75,64 +74,61 @@ func HandleCancel( return } - if build.Status != core.StatusPending && - build.Status != core.StatusRunning { - logger.FromRequest(r). - WithField("build", build.Number). - WithField("namespace", namespace). - WithField("name", name). - Debugln("api: cannot cancel completed build") - render.InternalError(w, errors.New("Build must be pending or running to cancel")) - return - } + done := build.Status != core.StatusPending && + build.Status != core.StatusRunning - build.Status = core.StatusKilled - build.Finished = time.Now().Unix() - if build.Started == 0 { - build.Started = time.Now().Unix() - } + // do not cancel the build if the build status is + // complete. only cancel the build if the status is + // running or pending. + if !done { + build.Status = core.StatusKilled + build.Finished = time.Now().Unix() + if build.Started == 0 { + build.Started = time.Now().Unix() + } - err = builds.Update(r.Context(), build) - if err != nil { - logger.FromRequest(r). - WithError(err). - WithField("build", build.Number). - WithField("namespace", namespace). - WithField("name", name). - Warnln("api: cannot update build status to cancelled") - render.ErrorCode(w, err, http.StatusConflict) - return - } - - err = scheduler.Cancel(r.Context(), build.ID) - if err != nil { - logger.FromRequest(r). - WithError(err). - WithField("build", build.Number). - WithField("namespace", namespace). - WithField("name", name). - Warnln("api: cannot signal cancelled build is complete") - } - - user, err := users.Find(r.Context(), repo.UserID) - if err != nil { - logger.FromRequest(r). - WithError(err). - WithField("namespace", namespace). - WithField("name", name). - Debugln("api: cannot repository owner") - } else { - err := status.Send(r.Context(), user, &core.StatusInput{ - Repo: repo, - Build: build, - }) + err = builds.Update(r.Context(), build) if err != nil { logger.FromRequest(r). WithError(err). WithField("build", build.Number). WithField("namespace", namespace). WithField("name", name). - Debugln("api: cannot set status") + Warnln("api: cannot update build status to cancelled") + render.ErrorCode(w, err, http.StatusConflict) + return + } + + err = scheduler.Cancel(r.Context(), build.ID) + if err != nil { + logger.FromRequest(r). + WithError(err). + WithField("build", build.Number). + WithField("namespace", namespace). + WithField("name", name). + Warnln("api: cannot signal cancelled build is complete") + } + + user, err := users.Find(r.Context(), repo.UserID) + if err != nil { + logger.FromRequest(r). + WithError(err). + WithField("namespace", namespace). + WithField("name", name). + Debugln("api: cannot repository owner") + } else { + err := status.Send(r.Context(), user, &core.StatusInput{ + Repo: repo, + Build: build, + }) + if err != nil { + logger.FromRequest(r). + WithError(err). + WithField("build", build.Number). + WithField("namespace", namespace). + WithField("name", name). + Debugln("api: cannot set status") + } } } @@ -201,16 +197,22 @@ func HandleCancel( Debugln("api: successfully cancelled build") build.Stages = stagez - payload := &core.WebhookData{ - Event: core.WebhookEventBuild, - Action: core.WebhookActionUpdated, - Repo: repo, - Build: build, - } - err = webhooks.Send(context.Background(), payload) - if err != nil { - logger.FromRequest(r).WithError(err). - Warnln("manager: cannot send global webhook") + + // do not trigger a webhook if the build was already + // complete. only trigger a webhook if the build was + // pending or running and then cancelled. + if !done { + payload := &core.WebhookData{ + Event: core.WebhookEventBuild, + Action: core.WebhookActionUpdated, + Repo: repo, + Build: build, + } + err = webhooks.Send(context.Background(), payload) + if err != nil { + logger.FromRequest(r).WithError(err). + Warnln("manager: cannot send global webhook") + } } render.JSON(w, build, 200) diff --git a/version/version.go b/version/version.go index e92c6a1c..dc5e246c 100644 --- a/version/version.go +++ b/version/version.go @@ -27,7 +27,7 @@ var ( // VersionMinor is for functionality in a backwards-compatible manner. VersionMinor int64 = 2 // VersionPatch is for backwards-compatible bug fixes. - VersionPatch int64 = 2 + VersionPatch int64 = 3 // VersionPre indicates prerelease. VersionPre = "" // VersionDev indicates development branch. Releases will be empty string. diff --git a/version/version_test.go b/version/version_test.go index d7e2ea7a..561dbf18 100644 --- a/version/version_test.go +++ b/version/version_test.go @@ -9,7 +9,7 @@ package version import "testing" func TestVersion(t *testing.T) { - if got, want := Version.String(), "1.2.2"; got != want { + if got, want := Version.String(), "1.2.3"; got != want { t.Errorf("Want version %s, got %s", want, got) } }