Use same func for accept gated pipelines and none gated pipelines (#594)

* write back to webhook caller what happend

* skip sound like an error - it is none change that

* improve hook func

* dedup code & fix bugs that only existed on gated builds

* startBuild use std context

* wordings

Co-authored-by: Anbraten <anton@ju60.de>

* nit

* todo done

Co-authored-by: Anbraten <anton@ju60.de>

server/api/build.go

@@ -19,6 +19,9 @@ package api
 
 import (
 	"bytes"
+	"context"
+	"database/sql"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -276,11 +279,10 @@ func DeleteBuild(c *gin.Context) {
 
 func PostApproval(c *gin.Context) {
 	var (
-		_remote = server.Config.Services.Remote
-		_store  = store.FromContext(c)
-		repo    = session.Repo(c)
-		user    = session.User(c)
-		num, _  = strconv.ParseInt(c.Params.ByName("number"), 10, 64)
+		_store = store.FromContext(c)
+		repo   = session.Repo(c)
+		user   = session.User(c)
+		num, _ = strconv.ParseInt(c.Params.ByName("number"), 10, 64)
 	)
 
 	build, err := _store.GetBuildNumber(repo, num)
@@ -289,7 +291,7 @@ func PostApproval(c *gin.Context) {
 		return
 	}
 	if build.Status != model.StatusBlocked {
-		c.String(500, "cannot decline a build with status %s", build.Status)
+		c.String(http.StatusBadRequest, "cannot decline a build with status %s", build.Status)
 		return
 	}
 
@@ -301,30 +303,47 @@ func PostApproval(c *gin.Context) {
 		return
 	}
 
-	netrc, err := _remote.Netrc(user, repo)
-	if err != nil {
-		c.String(500, "failed to generate netrc file. %s", err)
-		return
-	}
-
 	if build, err = shared.UpdateToStatusPending(_store, *build, user.Login); err != nil {
-		c.String(500, "error updating build. %s", err)
+		c.String(http.StatusInternalServerError, "error updating build. %s", err)
 		return
 	}
 
-	c.JSON(200, build)
+	var yamls []*remote.FileMeta
+	for _, y := range configs {
+		yamls = append(yamls, &remote.FileMeta{Data: y.Data, Name: y.Name})
+	}
+
+	build, err = startBuild(c, _store, build, user, repo, yamls)
+	if err != nil {
+		c.String(http.StatusInternalServerError, fmt.Sprintf("startBuild: %v", err))
+	}
+	c.JSON(200, build)
+}
+
+func startBuild(ctx context.Context, store store.Store, build *model.Build, user *model.User, repo *model.Repo, yamls []*remote.FileMeta) (*model.Build, error) {
+	netrc, err := server.Config.Services.Remote.Netrc(user, repo)
+	if err != nil {
+		msg := "Failed to generate netrc file"
+		log.Error().Err(err).Msg(msg)
+		return nil, fmt.Errorf("%s: %v", msg, err)
+	}
+
+	// get the previous build so that we can send status change notifications
+	last, err := store.GetBuildLastBefore(repo, build.Branch, build.ID)
+	if err != nil && !errors.Is(err, sql.ErrNoRows) {
+		log.Error().Err(err).Str("repo", repo.FullName).Msgf("Error getting last build before build number '%d'", build.Number)
+	}
 
-	// get the previous build so that we can send
-	// on status change notifications
-	last, _ := _store.GetBuildLastBefore(repo, build.Branch, build.ID)
 	secs, err := server.Config.Services.Secrets.SecretListBuild(repo, build)
 	if err != nil {
-		log.Debug().Msgf("Error getting secrets for %s#%d. %s", repo.FullName, build.Number, err)
+		log.Error().Err(err).Msgf("Error getting secrets for %s#%d", repo.FullName, build.Number)
 	}
+
 	regs, err := server.Config.Services.Registries.RegistryList(repo)
 	if err != nil {
-		log.Debug().Msgf("Error getting registry credentials for %s#%d. %s", repo.FullName, build.Number, err)
+		log.Error().Err(err).Msgf("Error getting registry credentials for %s#%d", repo.FullName, build.Number)
 	}
+
 	envs := map[string]string{}
 	if server.Config.Services.Environ != nil {
 		globals, _ := server.Config.Services.Environ.EnvironList(repo)
@@ -333,11 +352,6 @@ func PostApproval(c *gin.Context) {
 		}
 	}
 
-	var yamls []*remote.FileMeta
-	for _, y := range configs {
-		yamls = append(yamls, &remote.FileMeta{Data: y.Data, Name: y.Name})
-	}
-
 	b := shared.ProcBuilder{
 		Repo:  repo,
 		Curr:  build,
@@ -345,44 +359,45 @@ func PostApproval(c *gin.Context) {
 		Netrc: netrc,
 		Secs:  secs,
 		Regs:  regs,
+		Envs:  envs,
 		Link:  server.Config.Server.Host,
 		Yamls: yamls,
-		Envs:  envs,
 	}
 	buildItems, err := b.Build()
 	if err != nil {
-		if _, err = shared.UpdateToStatusError(_store, *build, err); err != nil {
-			log.Error().Msgf("Error setting error status of build for %s#%d. %s", repo.FullName, build.Number, err)
+		if _, err := shared.UpdateToStatusError(store, *build, err); err != nil {
+			log.Error().Err(err).Msgf("Error setting error status of build for %s#%d", repo.FullName, build.Number)
 		}
-		return
+		return nil, err
 	}
 	build = shared.SetBuildStepsOnBuild(b.Curr, buildItems)
 
-	err = _store.ProcCreate(build.Procs)
-	if err != nil {
-		log.Error().Msgf("error persisting procs %s/%d: %s", repo.FullName, build.Number, err)
+	if err := store.ProcCreate(build.Procs); err != nil {
+		log.Error().Err(err).Str("repo", repo.FullName).Msgf("error persisting procs for %s#%d", repo.FullName, build.Number)
 	}
 
 	defer func() {
 		for _, item := range buildItems {
-			uri := fmt.Sprintf("%s/%s/%d", server.Config.Server.Host, repo.FullName, build.Number)
+			uri := fmt.Sprintf("%s/%s/build/%d", server.Config.Server.Host, repo.FullName, build.Number)
 			if len(buildItems) > 1 {
-				err = _remote.Status(c, user, repo, build, uri, item.Proc)
+				err = server.Config.Services.Remote.Status(ctx, user, repo, build, uri, item.Proc)
 			} else {
-				err = _remote.Status(c, user, repo, build, uri, nil)
+				err = server.Config.Services.Remote.Status(ctx, user, repo, build, uri, nil)
 			}
 			if err != nil {
-				log.Error().Msgf("error setting commit status for %s/%d: %v", repo.FullName, build.Number, err)
+				log.Error().Err(err).Msgf("error setting commit status for %s/%d", repo.FullName, build.Number)
 			}
 		}
 	}()
 
-	if err := publishToTopic(c, build, repo, model.Enqueued); err != nil {
+	if err := publishToTopic(ctx, build, repo, model.Enqueued); err != nil {
 		log.Error().Err(err).Msg("publishToTopic")
 	}
 	if err := queueBuild(build, repo, buildItems); err != nil {
 		log.Error().Err(err).Msg("queueBuild")
 	}
+
+	return build, nil
 }
 
 func PostDecline(c *gin.Context) {

server/api/hook.go

@@ -76,22 +76,21 @@ func BlockTilQueueHasRunningItem(c *gin.Context) {
 }
 
 func PostHook(c *gin.Context) {
-	_remote := server.Config.Services.Remote
 	_store := store.FromContext(c)
 
-	tmpRepo, build, err := _remote.Hook(c.Request)
+	tmpRepo, build, err := server.Config.Services.Remote.Hook(c.Request)
 	if err != nil {
-		log.Error().Msgf("failure to parse hook. %s", err)
-		_ = c.AbortWithError(400, err)
+		msg := "failure to parse hook"
+		log.Debug().Err(err).Msg(msg)
+		c.String(http.StatusBadRequest, "%s: %v", msg, err)
 		return
 	}
 	if build == nil {
-		c.Writer.WriteHeader(200)
+		c.String(http.StatusOK, "ignoring hook: hook parsing resulted in empty build")
 		return
 	}
 	if tmpRepo == nil {
-		log.Error().Msgf("failure to ascertain repo from hook.")
-		c.Writer.WriteHeader(400)
+		c.String(http.StatusBadRequest, "failure to ascertain repo from hook")
 		return
 	}
 
@@ -99,94 +98,104 @@ func PostHook(c *gin.Context) {
 	// wrapped in square brackets appear in the commit message
 	skipMatch := skipRe.FindString(build.Message)
 	if len(skipMatch) > 0 {
-		log.Info().Msgf("ignoring hook. %s found in %s", skipMatch, build.Commit)
-		c.Writer.WriteHeader(204)
+		msg := fmt.Sprintf("ignoring hook: %s found in %s", skipMatch, build.Commit)
+		log.Debug().Msg(msg)
+		c.String(http.StatusNoContent, msg)
 		return
 	}
 
 	repo, err := _store.GetRepoName(tmpRepo.Owner + "/" + tmpRepo.Name)
 	if err != nil {
-		log.Error().Msgf("failure to find repo %s/%s from hook. %s", tmpRepo.Owner, tmpRepo.Name, err)
-		_ = c.AbortWithError(404, err)
+		msg := fmt.Sprintf("failure to get repo %s/%s from store", tmpRepo.Owner, tmpRepo.Name)
+		log.Error().Err(err).Msg(msg)
+		c.String(http.StatusNotFound, "%s: %v", msg, err)
 		return
 	}
 	if !repo.IsActive {
-		log.Error().Msgf("ignoring hook. %s/%s is inactive.", tmpRepo.Owner, tmpRepo.Name)
-		_ = c.AbortWithError(204, err)
+		c.String(204, "ignoring hook: %s/%s is inactive.", tmpRepo.Owner, tmpRepo.Name)
 		return
 	}
 
 	// get the token and verify the hook is authorized
-	parsed, err := token.ParseRequest(c.Request, func(t *token.Token) (string, error) {
+	parsed, err := token.ParseRequest(c.Request, func(_ *token.Token) (string, error) {
 		return repo.Hash, nil
 	})
 	if err != nil {
-		log.Error().Msgf("failure to parse token from hook for %s. %s", repo.FullName, err)
-		_ = c.AbortWithError(400, err)
+		msg := fmt.Sprintf("failure to parse token from hook for %s", repo.FullName)
+		log.Error().Err(err).Msg(msg)
+		c.String(http.StatusBadRequest, "%s: %v", msg, err)
 		return
 	}
 	if parsed.Text != repo.FullName {
-		log.Error().Msgf("failure to verify token from hook. Expected %s, got %s", repo.FullName, parsed.Text)
-		c.AbortWithStatus(403)
+		msg := fmt.Sprintf("failure to verify token from hook. Expected %s, got %s", repo.FullName, parsed.Text)
+		log.Debug().Msg(msg)
+		c.String(http.StatusForbidden, msg)
 		return
 	}
 
 	if repo.UserID == 0 {
-		log.Warn().Msgf("ignoring hook. repo %s has no owner.", repo.FullName)
-		c.Writer.WriteHeader(204)
+		msg := fmt.Sprintf("ignoring hook. repo %s has no owner.", repo.FullName)
+		log.Warn().Msg(msg)
+		c.String(http.StatusNoContent, msg)
 		return
 	}
 
 	if build.Event == model.EventPull && !repo.AllowPull {
-		log.Info().Msgf("ignoring hook. repo %s is disabled for pull requests.", repo.FullName)
-		_, _ = c.Writer.Write([]byte("pulls are disabled on woodpecker for this repo"))
-		c.Writer.WriteHeader(204)
+		msg := "ignoring hook: pull requests are disabled for this repo in woodpecker"
+		log.Debug().Str("repo", repo.FullName).Msg(msg)
+		c.String(http.StatusNoContent, msg)
 		return
 	}
 
-	user, err := _store.GetUser(repo.UserID)
+	repoUser, err := _store.GetUser(repo.UserID)
 	if err != nil {
-		log.Error().Msgf("failure to find repo owner %s. %s", repo.FullName, err)
-		_ = c.AbortWithError(500, err)
+		log.Error().Err(err).Str("repo", repo.FullName).Msgf("failure to find repo owner via id '%d'", repo.UserID)
+		_ = c.AbortWithError(http.StatusInternalServerError, err)
 		return
 	}
 
 	// if the remote has a refresh token, the current access token
 	// may be stale. Therefore, we should refresh prior to dispatching
 	// the build.
-	if refresher, ok := _remote.(remote.Refresher); ok {
-		ok, err := refresher.Refresh(c, user)
+	if refresher, ok := server.Config.Services.Remote.(remote.Refresher); ok {
+		refreshed, err := refresher.Refresh(c, repoUser)
 		if err != nil {
-			log.Error().Msgf("failed to refresh oauth2 token: %s", err)
-		} else if ok {
-			if err := _store.UpdateUser(user); err != nil {
-				log.Error().Msgf("error while updating user: %s", err)
+			log.Error().Err(err).Msgf("failed to refresh oauth2 token for repoUser: %s", repoUser.Login)
+		} else if refreshed {
+			if err := _store.UpdateUser(repoUser); err != nil {
+				log.Error().Err(err).Msgf("error while updating repoUser: %s", repoUser.Login)
 				// move forward
 			}
 		}
 	}
 
 	// fetch the build file from the remote
-	configFetcher := shared.NewConfigFetcher(_remote, user, repo, build)
+	configFetcher := shared.NewConfigFetcher(server.Config.Services.Remote, repoUser, repo, build)
 	remoteYamlConfigs, err := configFetcher.Fetch(c)
 	if err != nil {
-		log.Error().Msgf("error: %s: cannot find %s in %s: %s", repo.FullName, repo.Config, build.Ref, err)
-		_ = c.AbortWithError(404, err)
+		msg := fmt.Sprintf("cannot find '%s' in '%s', context user: '%s'", repo.Config, build.Ref, repoUser.Login)
+		log.Debug().Err(err).Str("repo", repo.FullName).Msg(msg)
+		c.String(http.StatusNotFound, "%s: %v", msg, err)
 		return
 	}
 
 	filtered, err := branchFiltered(build, remoteYamlConfigs)
 	if err != nil {
-		log.Error().Msgf("failure to parse yaml from hook for %s. %s", repo.FullName, err)
-		_ = c.AbortWithError(400, err)
+		msg := "failure to parse yaml from hook"
+		log.Debug().Err(err).Str("repo", repo.FullName).Msg(msg)
+		c.String(http.StatusBadRequest, "%s: %v", msg, err)
 	}
 	if filtered {
-		c.String(200, "Branch does not match restrictions defined in yaml")
+		msg := "ignoring hook: branch does not match restrictions defined in yaml"
+		log.Debug().Str("repo", repo.FullName).Msg(msg)
+		c.String(200, msg)
 		return
 	}
 
 	if zeroSteps(build, remoteYamlConfigs) {
-		c.String(200, "Step conditions yield zero runnable steps")
+		msg := "ignoring hook: step conditions yield zero runnable steps"
+		log.Debug().Str("repo", repo.FullName).Msg(msg)
+		c.String(200, msg)
 		return
 	}
 
@@ -195,14 +204,15 @@ func PostHook(c *gin.Context) {
 	build.Verified = true
 	build.Status = model.StatusPending
 
-	if repo.IsGated && build.Sender != user.Login {
+	// TODO(336) extend gated feature with an allow/block List
+	if repo.IsGated && build.Sender != repoUser.Login {
 		build.Status = model.StatusBlocked
 	}
 
 	err = _store.CreateBuild(build, build.Procs...)
 	if err != nil {
-		log.Error().Msgf("failure to save commit for %s. %s", repo.FullName, err)
-		_ = c.AbortWithError(500, err)
+		log.Error().Err(err).Msgf("failure to save commit for %s", repo.FullName)
+		_ = c.AbortWithError(http.StatusInternalServerError, err)
 		return
 	}
 
@@ -210,90 +220,22 @@ func PostHook(c *gin.Context) {
 	for _, remoteYamlConfig := range remoteYamlConfigs {
 		_, err := findOrPersistPipelineConfig(repo, build, remoteYamlConfig)
 		if err != nil {
-			log.Error().Msgf("failure to find or persist build config for %s. %s", repo.FullName, err)
-			_ = c.AbortWithError(500, err)
+			log.Error().Err(err).Msgf("failure to find or persist build config for %s", repo.FullName)
+			_ = c.AbortWithError(http.StatusInternalServerError, err)
 			return
 		}
 	}
 
-	c.JSON(200, build)
-
 	if build.Status == model.StatusBlocked {
+		c.JSON(200, build)
 		return
 	}
 
-	netrc, err := _remote.Netrc(user, repo)
+	build, err = startBuild(c, _store, build, repoUser, repo, remoteYamlConfigs)
 	if err != nil {
-		c.String(500, "Failed to generate netrc file. %s", err)
-		return
-	}
-
-	envs := map[string]string{}
-	if server.Config.Services.Environ != nil {
-		globals, _ := server.Config.Services.Environ.EnvironList(repo)
-		for _, global := range globals {
-			envs[global.Name] = global.Value
-		}
-	}
-
-	secs, err := server.Config.Services.Secrets.SecretListBuild(repo, build)
-	if err != nil {
-		log.Debug().Msgf("Error getting secrets for %s#%d. %s", repo.FullName, build.Number, err)
-	}
-
-	regs, err := server.Config.Services.Registries.RegistryList(repo)
-	if err != nil {
-		log.Debug().Msgf("Error getting registry credentials for %s#%d. %s", repo.FullName, build.Number, err)
-	}
-
-	// get the previous build so that we can send status change notifications
-	last, _ := _store.GetBuildLastBefore(repo, build.Branch, build.ID)
-
-	b := shared.ProcBuilder{
-		Repo:  repo,
-		Curr:  build,
-		Last:  last,
-		Netrc: netrc,
-		Secs:  secs,
-		Regs:  regs,
-		Envs:  envs,
-		Link:  server.Config.Server.Host,
-		Yamls: remoteYamlConfigs,
-	}
-	buildItems, err := b.Build()
-	if err != nil {
-		if _, err = shared.UpdateToStatusError(_store, *build, err); err != nil {
-			log.Error().Msgf("Error setting error status of build for %s#%d. %s", repo.FullName, build.Number, err)
-		}
-		return
-	}
-	build = shared.SetBuildStepsOnBuild(b.Curr, buildItems)
-
-	err = _store.ProcCreate(build.Procs)
-	if err != nil {
-		log.Error().Msgf("error persisting procs %s/%d: %s", repo.FullName, build.Number, err)
-	}
-
-	defer func() {
-		for _, item := range buildItems {
-			uri := fmt.Sprintf("%s/%s/build/%d", server.Config.Server.Host, repo.FullName, build.Number)
-			if len(buildItems) > 1 {
-				err = _remote.Status(c, user, repo, build, uri, item.Proc)
-			} else {
-				err = _remote.Status(c, user, repo, build, uri, nil)
-			}
-			if err != nil {
-				log.Error().Msgf("error setting commit status for %s/%d: %v", repo.FullName, build.Number, err)
-			}
-		}
-	}()
-
-	if err := publishToTopic(c, build, repo, model.Enqueued); err != nil {
-		log.Error().Err(err).Msg("publishToTopic")
-	}
-	if err := queueBuild(build, repo, buildItems); err != nil {
-		log.Error().Err(err).Msg("queueBuild")
+		c.String(http.StatusInternalServerError, fmt.Sprintf("startBuild: %v", err))
 	}
+	c.JSON(200, build)
 }
 
 // TODO: parse yaml once and not for each filter function
@@ -371,7 +313,7 @@ func findOrPersistPipelineConfig(repo *model.Repo, build *model.Build, remoteYam
 }
 
 // publishes message to UI clients
-func publishToTopic(c *gin.Context, build *model.Build, repo *model.Repo, event model.EventType) (err error) {
+func publishToTopic(c context.Context, build *model.Build, repo *model.Repo, event model.EventType) (err error) {
 	message := pubsub.Message{
 		Labels: map[string]string{
 			"repo":    repo.FullName,

server/model/secret.go

@@ -17,6 +17,7 @@ package model
 
 import (
 	"errors"
+	"fmt"
 	"path/filepath"
 )
 
@@ -80,7 +81,7 @@ func (s *Secret) Match(event WebhookEvent) bool {
 func (s *Secret) Validate() error {
 	for _, event := range s.Events {
 		if !ValidateWebhookEvent(event) {
-			return errSecretEventInvalid
+			return fmt.Errorf("%s: '%s'", errSecretEventInvalid, event)
 		}
 	}