Use global branch filter only on events containing branch info (#659)

- close #581
- delete unused code
- simplify code
- add check to procBuilder to fail on invalid config

cmd/server/server.go

@@ -256,7 +256,6 @@ func run(c *cli.Context) error {
 func setupEvilGlobals(c *cli.Context, v store.Store, r remote.Remote) {
 	// storage
 	server.Config.Storage.Files = v
-	server.Config.Storage.Config = v
 
 	// remote
 	server.Config.Services.Remote = r

pipeline/frontend/yaml/config.go

@@ -1,10 +1,6 @@
 package yaml
 
 import (
-	"io"
-	"io/ioutil"
-	"os"
-
 	"gopkg.in/yaml.v3"
 
 	"github.com/woodpecker-ci/woodpecker/pipeline/frontend/yaml/types"
@@ -35,15 +31,6 @@ type (
 	}
 )
 
-// Parse parses the configuration from bytes b.
-func Parse(r io.Reader) (*Config, error) {
-	out, err := ioutil.ReadAll(r)
-	if err != nil {
-		return nil, err
-	}
-	return ParseBytes(out)
-}
-
 // ParseBytes parses the configuration from bytes b.
 func ParseBytes(b []byte) (*Config, error) {
 	out := new(Config)
@@ -61,13 +48,3 @@ func ParseString(s string) (*Config, error) {
 		[]byte(s),
 	)
 }
-
-// ParseFile parses the configuration from path p.
-func ParseFile(p string) (*Config, error) {
-	f, err := os.Open(p)
-	if err != nil {
-		return nil, err
-	}
-	defer f.Close()
-	return Parse(f)
-}

server/api/build.go

@@ -296,7 +296,7 @@ func PostApproval(c *gin.Context) {
 	}
 
 	// fetch the build file from the database
-	configs, err := server.Config.Storage.Config.ConfigsForBuild(build.ID)
+	configs, err := _store.ConfigsForBuild(build.ID)
 	if err != nil {
 		log.Error().Msgf("failure to get build config for %s. %s", repo.FullName, err)
 		_ = c.AbortWithError(404, err)
@@ -426,7 +426,7 @@ func PostBuild(c *gin.Context) {
 	}
 
 	// fetch the pipeline config from database
-	configs, err := server.Config.Storage.Config.ConfigsForBuild(build.ID)
+	configs, err := _store.ConfigsForBuild(build.ID)
 	if err != nil {
 		log.Error().Msgf("failure to get build config for %s. %s", repo.FullName, err)
 		_ = c.AbortWithError(404, err)
@@ -465,8 +465,7 @@ func PostBuild(c *gin.Context) {
 		return
 	}
 
-	err = persistBuildConfigs(configs, build.ID)
-	if err != nil {
+	if err := persistBuildConfigs(_store, configs, build.ID); err != nil {
 		msg := fmt.Sprintf("failure to persist build config for %s.", repo.FullName)
 		log.Error().Err(err).Msg(msg)
 		c.String(http.StatusInternalServerError, msg)
@@ -643,13 +642,13 @@ func updateBuildStatus(ctx context.Context, build *model.Build, repo *model.Repo
 	return nil
 }
 
-func persistBuildConfigs(configs []*model.Config, buildID int64) error {
+func persistBuildConfigs(store store.Store, configs []*model.Config, buildID int64) error {
 	for _, conf := range configs {
 		buildConfig := &model.BuildConfig{
 			ConfigID: conf.ID,
 			BuildID:  buildID,
 		}
-		err := server.Config.Storage.Config.BuildConfigCreate(buildConfig)
+		err := store.BuildConfigCreate(buildConfig)
 		if err != nil {
 			return err
 		}

server/api/hook.go

@@ -227,7 +227,7 @@ func PostHook(c *gin.Context) {
 
 	// persist the build config for historical correctness, restarts, etc
 	for _, remoteYamlConfig := range remoteYamlConfigs {
-		_, err := findOrPersistPipelineConfig(repo, build, remoteYamlConfig)
+		_, err := findOrPersistPipelineConfig(_store, build, remoteYamlConfig)
 		if err != nil {
 			msg := fmt.Sprintf("failure to find or persist pipeline config for %s", repo.FullName)
 			log.Error().Err(err).Msg(msg)
@@ -271,16 +271,20 @@ func PostHook(c *gin.Context) {
 // TODO: parse yaml once and not for each filter function
 func branchFiltered(build *model.Build, remoteYamlConfigs []*remote.FileMeta) (bool, error) {
 	log.Trace().Msgf("hook.branchFiltered(): build branch: '%s' build event: '%s' config count: %d", build.Branch, build.Event, len(remoteYamlConfigs))
+
+	if build.Event == model.EventTag || build.Event == model.EventDeploy {
+		return false, nil
+	}
+
 	for _, remoteYamlConfig := range remoteYamlConfigs {
-		parsedPipelineConfig, err := yaml.ParseString(string(remoteYamlConfig.Data))
+		parsedPipelineConfig, err := yaml.ParseBytes(remoteYamlConfig.Data)
 		if err != nil {
 			log.Trace().Msgf("parse config '%s': %s", remoteYamlConfig.Name, err)
 			return false, err
 		}
 		log.Trace().Msgf("config '%s': %#v", remoteYamlConfig.Name, parsedPipelineConfig)
 
-		if !parsedPipelineConfig.Branches.Match(build.Branch) && build.Event != model.EventTag && build.Event != model.EventDeploy {
-		} else {
+		if parsedPipelineConfig.Branches.Match(build.Branch) {
 			return false, nil
 		}
 	}
@@ -310,9 +314,9 @@ func zeroSteps(build *model.Build, remoteYamlConfigs []*remote.FileMeta) bool {
 	return false
 }
 
-func findOrPersistPipelineConfig(repo *model.Repo, build *model.Build, remoteYamlConfig *remote.FileMeta) (*model.Config, error) {
+func findOrPersistPipelineConfig(store store.Store, build *model.Build, remoteYamlConfig *remote.FileMeta) (*model.Config, error) {
 	sha := shasum(remoteYamlConfig.Data)
-	conf, err := server.Config.Storage.Config.ConfigFindIdentical(build.RepoID, sha)
+	conf, err := store.ConfigFindIdentical(build.RepoID, sha)
 	if err != nil {
 		conf = &model.Config{
 			RepoID: build.RepoID,
@@ -320,10 +324,10 @@ func findOrPersistPipelineConfig(repo *model.Repo, build *model.Build, remoteYam
 			Hash:   sha,
 			Name:   shared.SanitizePath(remoteYamlConfig.Name),
 		}
-		err = server.Config.Storage.Config.ConfigCreate(conf)
+		err = store.ConfigCreate(conf)
 		if err != nil {
 			// retry in case we receive two hooks at the same time
-			conf, err = server.Config.Storage.Config.ConfigFindIdentical(build.RepoID, sha)
+			conf, err = store.ConfigFindIdentical(build.RepoID, sha)
 			if err != nil {
 				return nil, err
 			}
@@ -334,8 +338,7 @@ func findOrPersistPipelineConfig(repo *model.Repo, build *model.Build, remoteYam
 		ConfigID: conf.ID,
 		BuildID:  build.ID,
 	}
-	err = server.Config.Storage.Config.BuildConfigCreate(buildConfig)
-	if err != nil {
+	if err := store.BuildConfigCreate(buildConfig); err != nil {
 		return nil, err
 	}
 

server/config.go

@@ -43,9 +43,8 @@ var Config = struct {
 		// Repos  model.RepoStore
 		// Builds model.BuildStore
 		// Logs   model.LogStore
-		Config model.ConfigStore
-		Files  model.FileStore
-		Procs  model.ProcStore
+		Files model.FileStore
+		Procs model.ProcStore
 		// Registries model.RegistryStore
 		// Secrets model.SecretStore
 	}

server/shared/procBuilder.go

@@ -100,14 +100,13 @@ func (b *ProcBuilder) Build() ([]*BuildItem, error) {
 			}
 
 			// lint pipeline
-			lerr := linter.New(
+			if err := linter.New(
 				linter.WithTrusted(b.Repo.IsTrusted),
-			).Lint(parsed)
-			if lerr != nil {
-				return nil, lerr
+			).Lint(parsed); err != nil {
+				return nil, err
 			}
 
-			if !parsed.Branches.Match(b.Curr.Branch) {
+			if !parsed.Branches.Match(b.Curr.Branch) && (b.Curr.Event != model.EventDeploy && b.Curr.Event != model.EventTag) {
 				proc.State = model.StatusSkipped
 			}
 
@@ -138,9 +137,24 @@ func (b *ProcBuilder) Build() ([]*BuildItem, error) {
 
 	items = filterItemsWithMissingDependencies(items)
 
+	// check if at least one proc can start, if list is not empty
+	procListContainsItemsToRun(items)
+	if len(items) > 0 && !procListContainsItemsToRun(items) {
+		return nil, fmt.Errorf("build has no startpoint")
+	}
+
 	return items, nil
 }
 
+func procListContainsItemsToRun(items []*BuildItem) bool {
+	for i := range items {
+		if items[i].Proc.State == model.StatusPending {
+			return true
+		}
+	}
+	return false
+}
+
 func filterItemsWithMissingDependencies(items []*BuildItem) []*BuildItem {
 	itemsToRemove := make([]*BuildItem, 0)