Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: return code in CompileAndPublish rather than handleError #1107

Merged
merged 5 commits into from
Apr 16, 2024
Merged

fix: return code in CompileAndPublish rather than handleError #1107

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec82e59
fix: return code in CompileAndPublish rather than handleError
ecrupper Apr 12, 2024
563cc11
style check
ecrupper Apr 12, 2024
ae4e504
Merge branch 'main' into fix/compile-publish-error-handling
ecrupper Apr 12, 2024
cb86cf8
add return on errors for create and restart
ecrupper Apr 15, 2024
aa0b87e
update responses for create and restart to not mention webhooks
ecrupper Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 22 additions & 55 deletions api/build/compile_publish.go
View file
Open in desktop
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: 16 additions & 11 deletions api/build/create.go
View file
Open in desktop
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 the webhook but build was skipped
ecrupper marked this conversation as resolved.
Show resolved Hide resolved
// schema:
// type: string
// '201':
// description: Successfully created the build
// description: Successfully created the build from webhook
// type: json
// schema:
// "$ref": "#/definitions/Build"
// '400':
// description: Unable to create the build
// 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 create the build
// description: Unable to receive the webhook
// 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 webhook 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,15 +139,14 @@ 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 {
ecrupper marked this conversation as resolved.
Show resolved Hide resolved
c.JSON(http.StatusOK, err.Error())

return
}

// error handling done in CompileAndPublish
if err != nil {
return
util.HandleError(c, code, err)
ecrupper marked this conversation as resolved.
Show resolved Hide resolved
}

c.JSON(http.StatusCreated, item.Build)
Expand Down
24 changes: 16 additions & 8 deletions api/build/restart.go
View file
Open in desktop
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 the webhook but build was skipped
// schema:
// type: string
// '201':
// description: Successfully restarted the build
// description: Successfully created the build from webhook
// type: json
// schema:
// "$ref": "#/definitions/Build"
// '400':
// description: Unable to restart the build
// 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 restart the build
// description: Unable to receive the webhook
// 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 webhook 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,9 +138,8 @@ func RestartBuild(c *gin.Context) {
queue.FromContext(c),
)

// error handling done in CompileAndPublish
if err != nil {
return
util.HandleError(c, code, err)
ecrupper marked this conversation as resolved.
Show resolved Hide resolved
}

c.JSON(http.StatusCreated, item.Build)
Expand Down
Loading
Loading