From 7263f25533bfbe2dc79c9abf1805b519f1f9d7f5 Mon Sep 17 00:00:00 2001 From: Easton Crupper <65553218+ecrupper@users.noreply.github.com> Date: Tue, 16 Apr 2024 10:40:33 -0400 Subject: [PATCH] fix: return code in CompileAndPublish rather than handleError (#1107) * fix: return code in CompileAndPublish rather than handleError * style check * add return on errors for create and restart * update responses for create and restart to not mention webhooks --- api/build/compile_publish.go | 77 +++++++++++------------------------- api/build/create.go | 27 ++++++++----- api/build/restart.go | 24 +++++++---- api/webhook/post.go | 44 ++++++++++++--------- cmd/vela-server/schedule.go | 3 +- 5 files changed, 82 insertions(+), 93 deletions(-) diff --git a/api/build/compile_publish.go b/api/build/compile_publish.go index a9d52645f..cc6d121e4 100644 --- a/api/build/compile_publish.go +++ b/api/build/compile_publish.go @@ -20,7 +20,6 @@ import ( "github.com/go-vela/server/queue" "github.com/go-vela/server/queue/models" "github.com/go-vela/server/scm" - "github.com/go-vela/server/util" "github.com/go-vela/types/constants" "github.com/go-vela/types/library" "github.com/go-vela/types/pipeline" @@ -50,7 +49,7 @@ func CompileAndPublish( scm scm.Service, compiler compiler.Engine, queue queue.Service, -) (*pipeline.Build, *models.Item, error) { +) (*pipeline.Build, *models.Item, int, error) { logrus.Debugf("generating queue items for build %s/%d", cfg.Repo.GetFullName(), cfg.Build.GetNumber()) // assign variables from form for readibility @@ -63,9 +62,8 @@ func CompileAndPublish( _, err := scm.RepoAccess(c, u.GetName(), u.GetToken(), r.GetOrg(), r.GetName()) if err != nil { retErr := fmt.Errorf("unable to publish build to queue: repository owner %s no longer has write access to repository %s", u.GetName(), r.GetFullName()) - util.HandleError(c, http.StatusUnauthorized, retErr) - return nil, nil, retErr + return nil, nil, http.StatusUnauthorized, retErr } // get pull request number from build if event is pull_request or issue_comment @@ -74,28 +72,19 @@ func CompileAndPublish( prNum, err = getPRNumberFromBuild(b) if err != nil { retErr := fmt.Errorf("%s: failed to get pull request number for %s: %w", baseErr, r.GetFullName(), err) - util.HandleError(c, http.StatusBadRequest, retErr) - return nil, nil, retErr + return nil, nil, http.StatusBadRequest, retErr } } // if the event is issue_comment and the issue is a pull request, // call SCM for more data not provided in webhook payload if strings.EqualFold(cfg.Source, "webhook") && strings.EqualFold(b.GetEvent(), constants.EventComment) { - if err != nil { - retErr := fmt.Errorf("%s: failed to get pull request number for %s: %w", baseErr, r.GetFullName(), err) - util.HandleError(c, http.StatusBadRequest, retErr) - - return nil, nil, retErr - } - commit, branch, baseref, headref, err := scm.GetPullRequest(c, r, prNum) if err != nil { retErr := fmt.Errorf("%s: failed to get pull request info for %s: %w", baseErr, r.GetFullName(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } b.SetCommit(commit) @@ -110,9 +99,8 @@ func CompileAndPublish( _, commit, err := scm.GetBranch(c, r, b.GetBranch()) if err != nil { retErr := fmt.Errorf("failed to get commit for repo %s on %s branch: %w", r.GetFullName(), r.GetBranch(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } b.SetCommit(commit) @@ -127,9 +115,8 @@ func CompileAndPublish( builds, err := database.CountBuildsForRepo(c, r, filters) if err != nil { retErr := fmt.Errorf("%s: unable to get count of builds for repo %s", baseErr, r.GetFullName()) - util.HandleError(c, http.StatusBadRequest, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } logrus.Debugf("currently %d builds running on repo %s", builds, r.GetFullName()) @@ -137,9 +124,8 @@ func CompileAndPublish( // check if the number of pending and running builds exceeds the limit for the repo if builds >= r.GetBuildLimit() { retErr := fmt.Errorf("%s: repo %s has exceeded the concurrent build limit of %d", baseErr, r.GetFullName(), r.GetBuildLimit()) - util.HandleError(c, http.StatusBadRequest, retErr) - return nil, nil, retErr + return nil, nil, http.StatusTooManyRequests, retErr } // update fields in build object @@ -166,9 +152,8 @@ func CompileAndPublish( files, err = scm.Changeset(c, r, b.GetCommit()) if err != nil { retErr := fmt.Errorf("%s: failed to get changeset for %s: %w", baseErr, r.GetFullName(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } } @@ -178,9 +163,8 @@ func CompileAndPublish( files, err = scm.ChangesetPR(c, r, prNum) if err != nil { retErr := fmt.Errorf("%s: failed to get changeset for %s: %w", baseErr, r.GetFullName(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } } @@ -218,9 +202,7 @@ func CompileAndPublish( if err != nil { retErr := fmt.Errorf("%s: unable to get pipeline configuration for %s: %w", baseErr, r.GetFullName(), err) - util.HandleError(c, http.StatusNotFound, retErr) - - return nil, nil, retErr + return nil, nil, http.StatusNotFound, retErr } } else { pipelineFile = pipeline.GetData() @@ -239,9 +221,7 @@ func CompileAndPublish( continue } - util.HandleError(c, http.StatusBadRequest, retErr) - - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } // update DB record of repo (repo) with any changes captured from webhook payload (r) @@ -290,10 +270,7 @@ func CompileAndPublish( // log the error for traceability logrus.Error(err.Error()) - retErr := fmt.Errorf("%s: %w", baseErr, err) - util.HandleError(c, http.StatusInternalServerError, retErr) - - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, fmt.Errorf("%s: %w", baseErr, err) } // reset the pipeline type for the repo @@ -320,6 +297,7 @@ func CompileAndPublish( &models.Item{ Build: b, }, + http.StatusOK, errors.New(skip) } @@ -343,9 +321,7 @@ func CompileAndPublish( continue } - util.HandleError(c, http.StatusBadRequest, retErr) - - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } } @@ -375,9 +351,7 @@ func CompileAndPublish( continue } - util.HandleError(c, http.StatusInternalServerError, retErr) - - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } // break the loop because everything was successful @@ -388,34 +362,30 @@ func CompileAndPublish( repo, err = database.UpdateRepo(c, repo) if err != nil { retErr := fmt.Errorf("%s: failed to update repo %s: %w", baseErr, repo.GetFullName(), err) - util.HandleError(c, http.StatusBadRequest, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } // return error if pipeline didn't get populated if p == nil { retErr := fmt.Errorf("%s: failed to set pipeline for %s: %w", baseErr, repo.GetFullName(), err) - util.HandleError(c, http.StatusBadRequest, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } // return error if build didn't get populated if b == nil { retErr := fmt.Errorf("%s: failed to set build for %s: %w", baseErr, repo.GetFullName(), err) - util.HandleError(c, http.StatusBadRequest, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } // send API call to capture the triggered build b, err = database.GetBuildForRepo(c, repo, b.GetNumber()) if err != nil { retErr := fmt.Errorf("%s: failed to get new build %s/%d: %w", baseErr, repo.GetFullName(), b.GetNumber(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } // determine queue route @@ -426,9 +396,7 @@ func CompileAndPublish( // error out the build CleanBuild(c, database, b, nil, nil, retErr) - util.HandleError(c, http.StatusBadRequest, retErr) - - return nil, nil, retErr + return nil, nil, http.StatusBadRequest, retErr } // temporarily set host to the route before it gets picked up by a worker @@ -438,12 +406,11 @@ func CompileAndPublish( err = PublishBuildExecutable(c, database, p, b) if err != nil { retErr := fmt.Errorf("unable to publish build executable for %s/%d: %w", repo.GetFullName(), b.GetNumber(), err) - util.HandleError(c, http.StatusInternalServerError, retErr) - return nil, nil, retErr + return nil, nil, http.StatusInternalServerError, retErr } - return p, models.ToItem(b, repo), nil + return p, models.ToItem(b, repo), http.StatusCreated, nil } // getPRNumberFromBuild is a helper function to diff --git a/api/build/create.go b/api/build/create.go index 2801ce0db..dc8c81149 100644 --- a/api/build/create.go +++ b/api/build/create.go @@ -5,7 +5,6 @@ package build import ( "fmt" "net/http" - "strings" "github.com/gin-gonic/gin" "github.com/sirupsen/logrus" @@ -19,7 +18,6 @@ import ( "github.com/go-vela/server/router/middleware/user" "github.com/go-vela/server/scm" "github.com/go-vela/server/util" - "github.com/go-vela/types/constants" "github.com/go-vela/types/library" ) @@ -51,24 +49,32 @@ import ( // - ApiKeyAuth: [] // responses: // '200': -// description: Request processed but build was skipped +// description: Successfully received request but build was skipped // schema: // type: string // '201': -// description: Successfully created the build +// description: Successfully created the build from request // type: json // schema: // "$ref": "#/definitions/Build" // '400': -// description: Unable to create the build +// description: Malformed request payload or improper pipeline configuration +// schema: +// "$ref": "#/definitions/Error" +// '401': +// description: Repository owner does not have proper access // schema: // "$ref": "#/definitions/Error" // '404': -// description: Unable to create the build +// description: Unable to find resources for build +// schema: +// "$ref": "#/definitions/Error" +// '429': +// description: Concurrent build limit reached for repository // schema: // "$ref": "#/definitions/Error" // '500': -// description: Unable to create the build +// description: Unable to receive the request or internal error while processing // schema: // "$ref": "#/definitions/Error" @@ -123,7 +129,7 @@ func CreateBuild(c *gin.Context) { Retries: 1, } - _, item, err := CompileAndPublish( + _, item, code, err := CompileAndPublish( c, config, database.FromContext(c), @@ -133,14 +139,15 @@ func CreateBuild(c *gin.Context) { ) // check if build was skipped - if err != nil && strings.EqualFold(item.Build.GetStatus(), constants.StatusSkipped) { + if err != nil && code == http.StatusOK { c.JSON(http.StatusOK, err.Error()) return } - // error handling done in CompileAndPublish if err != nil { + util.HandleError(c, code, err) + return } diff --git a/api/build/restart.go b/api/build/restart.go index f52cf9c69..51c1bc0ff 100644 --- a/api/build/restart.go +++ b/api/build/restart.go @@ -51,23 +51,32 @@ import ( // - ApiKeyAuth: [] // responses: // '200': -// description: Request processed but build was skipped +// description: Successfully received request but build was skipped // schema: // type: string // '201': -// description: Successfully restarted the build +// description: Successfully created the build from request +// type: json // schema: // "$ref": "#/definitions/Build" // '400': -// description: Unable to restart the build +// description: Malformed request payload or improper pipeline configuration +// schema: +// "$ref": "#/definitions/Error" +// '401': +// description: Repository owner does not have proper access // schema: // "$ref": "#/definitions/Error" // '404': -// description: Unable to restart the build +// description: Unable to find resources for build +// schema: +// "$ref": "#/definitions/Error" +// '429': +// description: Concurrent build limit reached for repository // schema: // "$ref": "#/definitions/Error" // '500': -// description: Unable to restart the build +// description: Unable to receive the request or internal error while processing // schema: // "$ref": "#/definitions/Error" @@ -120,7 +129,7 @@ func RestartBuild(c *gin.Context) { } // generate queue items - _, item, err := CompileAndPublish( + _, item, code, err := CompileAndPublish( c, config, database.FromContext(c), @@ -129,8 +138,9 @@ func RestartBuild(c *gin.Context) { queue.FromContext(c), ) - // error handling done in CompileAndPublish if err != nil { + util.HandleError(c, code, err) + return } diff --git a/api/webhook/post.go b/api/webhook/post.go index f964398d7..8f6ef750e 100644 --- a/api/webhook/post.go +++ b/api/webhook/post.go @@ -47,23 +47,32 @@ var baseErr = "unable to process webhook" // "$ref": "#/definitions/Webhook" // responses: // '200': -// description: Successfully received the webhook +// description: Successfully received the webhook but build was skipped +// schema: +// type: string +// '201': +// description: Successfully created the build from webhook +// type: json // schema: // "$ref": "#/definitions/Build" // '400': -// description: Malformed webhook payload +// description: Malformed webhook payload or improper pipeline configuration +// schema: +// "$ref": "#/definitions/Error" +// '401': +// description: Repository owner does not have proper access // schema: // "$ref": "#/definitions/Error" // '404': // description: Unable to receive the webhook // schema: // "$ref": "#/definitions/Error" -// '401': -// description: Unauthenticated +// '429': +// description: Concurrent build limit reached for repository // schema: // "$ref": "#/definitions/Error" // '500': -// description: Unable to receive the webhook +// description: Unable to receive the webhook or internal error while processing // schema: // "$ref": "#/definitions/Error" @@ -300,7 +309,7 @@ func PostWebhook(c *gin.Context) { } // generate the queue item - p, item, err := build.CompileAndPublish( + p, item, code, err := build.CompileAndPublish( c, config, database.FromContext(c), @@ -310,18 +319,7 @@ func PostWebhook(c *gin.Context) { ) // error handling done in CompileAndPublish - if err != nil { - return - } - - // capture the build and repo from the items - b, repo = item.Build, item.Repo - - // set hook build_id to the generated build id - h.SetBuildID(b.GetID()) - - // check if build was skipped - if err != nil && strings.EqualFold(b.GetStatus(), constants.StatusSkipped) { + if err != nil && code == http.StatusOK { h.SetStatus(constants.StatusSkipped) h.SetError(err.Error()) @@ -334,9 +332,17 @@ func PostWebhook(c *gin.Context) { h.SetStatus(constants.StatusFailure) h.SetError(err.Error()) + util.HandleError(c, code, err) + return } + // capture the build and repo from the items + b, repo = item.Build, item.Repo + + // set hook build_id to the generated build id + h.SetBuildID(b.GetID()) + // if event is deployment, update the deployment record to include this build if strings.EqualFold(b.GetEvent(), constants.EventDeploy) { d, err := database.FromContext(c).GetDeploymentForRepo(c, repo, webhook.Deployment.GetNumber()) @@ -382,7 +388,7 @@ func PostWebhook(c *gin.Context) { } } - c.JSON(http.StatusOK, b) + c.JSON(http.StatusCreated, b) // regardless of whether the build is published to queue, we want to attempt to auto-cancel if no errors defer func() { diff --git a/cmd/vela-server/schedule.go b/cmd/vela-server/schedule.go index dda61bdea..d069b05a4 100644 --- a/cmd/vela-server/schedule.go +++ b/cmd/vela-server/schedule.go @@ -179,7 +179,7 @@ func processSchedule(ctx context.Context, s *library.Schedule, compiler compiler Retries: 1, } - _, item, err := build.CompileAndPublish( + _, item, _, err := build.CompileAndPublish( ctx, config, database, @@ -188,7 +188,6 @@ func processSchedule(ctx context.Context, s *library.Schedule, compiler compiler queue, ) - // error handling done in CompileAndPublish if err != nil { return err }