Skip to content

Commit

Permalink
fix: return code in CompileAndPublish rather than handleError (#1107)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@ecrupper
ecrupper authored Apr 16, 2024
1 parent 04f9308 commit 7263f25
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 93 deletions.
77 changes: 22 additions & 55 deletions api/build/compile_publish.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -127,19 +115,17 @@ 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())

// 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
Expand All @@ -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
}
}

Expand All @@ -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
}
}

Expand Down Expand Up @@ -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()
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -320,6 +297,7 @@ func CompileAndPublish(
&models.Item{
Build: b,
},
http.StatusOK,
errors.New(skip)
}

Expand All @@ -343,9 +321,7 @@ func CompileAndPublish(
continue
}

util.HandleError(c, http.StatusBadRequest, retErr)

return nil, nil, retErr
return nil, nil, http.StatusInternalServerError, retErr
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
27 changes: 17 additions & 10 deletions api/build/create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package build
import (
"fmt"
"net/http"
"strings"

"github.com/gin-gonic/gin"
"github.com/sirupsen/logrus"
Expand All @@ -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"
)

Expand Down Expand Up @@ -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"

Expand Down Expand Up @@ -123,7 +129,7 @@ func CreateBuild(c *gin.Context) {
Retries: 1,
}

_, item, err := CompileAndPublish(
_, item, code, err := CompileAndPublish(
c,
config,
database.FromContext(c),
Expand All @@ -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
}

Expand Down
24 changes: 17 additions & 7 deletions api/build/restart.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -120,7 +129,7 @@ func RestartBuild(c *gin.Context) {
}

// generate queue items
_, item, err := CompileAndPublish(
_, item, code, err := CompileAndPublish(
c,
config,
database.FromContext(c),
Expand All @@ -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
}

Expand Down
Loading

0 comments on commit 7263f25

Please sign in to comment.