Lint for event filter and deprecate exclude (#3222)

Closes https://github.com/woodpecker-ci/woodpecker/discussions/2174

- return bad habit error if no event filter is set
- If this is applied, it's useless to allow `exclude`s on events.
Therefore, deprecate it together with `include`s which should be
replaced by `base.StringOrSlice` later.

cmd/server/docs/docs.go

@@ -4423,9 +4423,11 @@ const docTemplate = `{
                 "linter",
                 "deprecation",
                 "compiler",
-                "generic"
+                "generic",
+                "bad_habit"
             ],
             "x-enum-comments": {
+                "PipelineErrorTypeBadHabit": "some bad-habit error",
                 "PipelineErrorTypeCompiler": "some error with the config semantics",
                 "PipelineErrorTypeDeprecation": "using some deprecated feature",
                 "PipelineErrorTypeGeneric": "some generic error",
@@ -4435,7 +4437,8 @@ const docTemplate = `{
                 "PipelineErrorTypeLinter",
                 "PipelineErrorTypeDeprecation",
                 "PipelineErrorTypeCompiler",
-                "PipelineErrorTypeGeneric"
+                "PipelineErrorTypeGeneric",
+                "PipelineErrorTypeBadHabit"
             ]
         },
         "model.Workflow": {

docs/docs/91-migrations.md

@@ -7,6 +7,7 @@ Some versions need some changes to the server configuration or the pipeline conf
 - Deprecated `steps.[name].group` in favor of `steps.[name].depends_on` (see [workflow syntax](./20-usage/20-workflow-syntax.md#depends_on) to learn how to set dependencies)
 - Removed `WOODPECKER_ROOT_PATH` and `WOODPECKER_ROOT_URL` config variables. Use `WOODPECKER_HOST` with a path instead
 - Pipelines without a config file will now be skipped instead of failing
+- Deprecated `includes` and `excludes` support from **event** filter
 
 ## 2.0.0
 

pipeline/errors/error.go

@@ -14,6 +14,7 @@ const (
 	PipelineErrorTypeDeprecation PipelineErrorType = "deprecation" // using some deprecated feature
 	PipelineErrorTypeCompiler    PipelineErrorType = "compiler"    // some error with the config semantics
 	PipelineErrorTypeGeneric     PipelineErrorType = "generic"     // some generic error
+	PipelineErrorTypeBadHabit    PipelineErrorType = "bad_habit"   // some bad-habit error
 )
 
 type PipelineError struct {

pipeline/frontend/yaml/constraint/constraint.go

@@ -42,7 +42,6 @@ type (
 		Instance    List
 		Platform    List
 		Environment List
-		Event       List
 		Branch      List
 		Cron        List
 		Status      List
@@ -50,6 +49,8 @@ type (
 		Local       yamlBaseTypes.BoolTrue
 		Path        Path
 		Evaluate    string `yaml:"evaluate,omitempty"`
+		// TODO change to StringOrSlice in 3.x
+		Event List
 	}
 
 	// List defines a runtime constraint for exclude & include string slices.

pipeline/frontend/yaml/linter/linter.go

@@ -263,10 +263,86 @@ func (l *Linter) lintDeprecations(config *WorkflowConfig) (err error) {
 		}
 	}
 
+	for i, c := range parsed.When.Constraints {
+		if len(c.Event.Exclude) != 0 {
+			err = multierr.Append(err, &errors.PipelineError{
+				Type:    errors.PipelineErrorTypeDeprecation,
+				Message: "Please only use allow lists for events",
+				Data: errors.DeprecationErrorData{
+					File:  config.File,
+					Field: fmt.Sprintf("when[%d].event", i),
+					Docs:  "https://woodpecker-ci.org/docs/usage/workflow-syntax#event-1",
+				},
+				IsWarning: true,
+			})
+		}
+	}
+
+	for _, step := range parsed.Steps.ContainerList {
+		for i, c := range step.When.Constraints {
+			if len(c.Event.Exclude) != 0 {
+				err = multierr.Append(err, &errors.PipelineError{
+					Type:    errors.PipelineErrorTypeDeprecation,
+					Message: "Please only use allow lists for events",
+					Data: errors.DeprecationErrorData{
+						File:  config.File,
+						Field: fmt.Sprintf("steps.%s.when[%d].event", step.Name, i),
+						Docs:  "https://woodpecker-ci.org/docs/usage/workflow-syntax#event",
+					},
+					IsWarning: true,
+				})
+			}
+		}
+	}
+
 	return err
 }
 
-func (l *Linter) lintBadHabits(_ *WorkflowConfig) error {
+func (l *Linter) lintBadHabits(config *WorkflowConfig) (err error) {
-	// TODO: add bad habit warnings
+	parsed := new(types.Workflow)
-	return nil
+	err = xyaml.Unmarshal([]byte(config.RawConfig), parsed)
+	if err != nil {
+		return err
+	}
+
+	rootEventFilters := len(parsed.When.Constraints) > 0
+	for _, c := range parsed.When.Constraints {
+		if len(c.Event.Include) == 0 {
+			rootEventFilters = false
+			break
+		}
+	}
+	if !rootEventFilters {
+		// root whens do not necessarily have an event filter, check steps
+		for _, step := range parsed.Steps.ContainerList {
+			var field string
+			if len(step.When.Constraints) == 0 {
+				field = fmt.Sprintf("steps.%s", step.Name)
+			} else {
+				stepEventIndex := -1
+				for i, c := range step.When.Constraints {
+					if len(c.Event.Include) == 0 {
+						stepEventIndex = i
+						break
+					}
+				}
+				if stepEventIndex > -1 {
+					field = fmt.Sprintf("steps.%s.when[%d]", step.Name, stepEventIndex)
+				}
+			}
+			if field != "" {
+				err = multierr.Append(err, &errors.PipelineError{
+					Type:    errors.PipelineErrorTypeBadHabit,
+					Message: "Please set an event filter on all when branches",
+					Data: errors.LinterErrorData{
+						File:  config.File,
+						Field: field,
+					},
+					IsWarning: true,
+				})
+			}
+		}
+	}
+
+	return
 }

pipeline/frontend/yaml/linter/linter_test.go

@@ -27,6 +27,9 @@ import (
 func TestLint(t *testing.T) {
 	testdatas := []struct{ Title, Data string }{{
 		Title: "map", Data: `
+when:
+  event: push
+
 steps:
   build:
     image: docker
@@ -46,6 +49,9 @@ services:
 `,
 	}, {
 		Title: "list", Data: `
+when:
+  event: push
+
 steps:
   - name: build
     image: docker
@@ -65,6 +71,9 @@ services:
 `,
 	}, {
 		Title: "merge maps", Data: `
+when:
+  event: push
+
 variables:
   step_template: &base-step
     image: golang:1.19
@@ -172,3 +181,41 @@ func TestLintErrors(t *testing.T) {
 		assert.True(t, found, "Expected error %q, got %q", test.want, lerrors)
 	}
 }
+
+func TestBadHabits(t *testing.T) {
+	testdata := []struct {
+		from string
+		want string
+	}{
+		{
+			from: "steps: { build: { image: golang } }",
+			want: "Please set an event filter on all when branches",
+		},
+		{
+			from: "when: [{branch: xyz}, {event: push}]\nsteps: { build: { image: golang } }",
+			want: "Please set an event filter on all when branches",
+		},
+	}
+
+	for _, test := range testdata {
+		conf, err := yaml.ParseString(test.from)
+		assert.NoError(t, err)
+
+		lerr := linter.New().Lint([]*linter.WorkflowConfig{{
+			File:      test.from,
+			RawConfig: test.from,
+			Workflow:  conf,
+		}})
+		assert.Error(t, lerr, "expected lint error for configuration", test.from)
+
+		lerrors := errors.GetPipelineErrors(lerr)
+		found := false
+		for _, lerr := range lerrors {
+			if lerr.Message == test.want {
+				found = true
+				break
+			}
+		}
+		assert.True(t, found, "Expected error %q, got %q", test.want, lerrors)
+	}
+}

server/pipeline/stepbuilder/stepBuilder_test.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
+	"go.woodpecker-ci.org/woodpecker/v2/pipeline/errors"
 	"go.woodpecker-ci.org/woodpecker/v2/server/forge"
 	"go.woodpecker-ci.org/woodpecker/v2/server/forge/mocks"
 	forge_types "go.woodpecker-ci.org/woodpecker/v2/server/forge/types"
@@ -39,6 +40,7 @@ func TestGlobalEnvsubst(t *testing.T) {
 		Repo: &model.Repo{},
 		Curr: &model.Pipeline{
 			Message: "aaa",
+			Event:   model.EventPush,
 		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
@@ -47,6 +49,8 @@ func TestGlobalEnvsubst(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: ${IMAGE}
@@ -75,6 +79,7 @@ func TestMissingGlobalEnvsubst(t *testing.T) {
 		Repo: &model.Repo{},
 		Curr: &model.Pipeline{
 			Message: "aaa",
+			Event:   model.EventPush,
 		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
@@ -83,6 +88,8 @@ func TestMissingGlobalEnvsubst(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: ${IMAGE}
@@ -116,6 +123,8 @@ bbb`,
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 steps:
   xxx:
     image: scratch
@@ -123,6 +132,8 @@ steps:
       yyy: ${CI_COMMIT_MESSAGE}
 `)},
 			{Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
@@ -145,7 +156,9 @@ func TestMultiPipeline(t *testing.T) {
 	b := StepBuilder{
 		Forge: getMockForge(t),
 		Repo:  &model.Repo{},
-		Curr:  &model.Pipeline{},
+		Curr: &model.Pipeline{
+			Event: model.EventPush,
+		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
 		Secs:  []*model.Secret{},
@@ -153,11 +166,15 @@ func TestMultiPipeline(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 steps:
   xxx:
     image: scratch
 `)},
 			{Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
@@ -180,7 +197,9 @@ func TestDependsOn(t *testing.T) {
 	b := StepBuilder{
 		Forge: getMockForge(t),
 		Repo:  &model.Repo{},
-		Curr:  &model.Pipeline{},
+		Curr: &model.Pipeline{
+			Event: model.EventPush,
+		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
 		Secs:  []*model.Secret{},
@@ -188,16 +207,22 @@ func TestDependsOn(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Name: "lint", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
 `)},
 			{Name: "test", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
 `)},
 			{Data: []byte(`
+when:
+  event: push
 steps:
   deploy:
     image: scratch
@@ -227,7 +252,9 @@ func TestRunsOn(t *testing.T) {
 	b := StepBuilder{
 		Forge: getMockForge(t),
 		Repo:  &model.Repo{},
-		Curr:  &model.Pipeline{},
+		Curr: &model.Pipeline{
+			Event: model.EventPush,
+		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
 		Secs:  []*model.Secret{},
@@ -235,6 +262,8 @@ func TestRunsOn(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 steps:
   deploy:
     image: scratch
@@ -264,7 +293,9 @@ func TestPipelineName(t *testing.T) {
 	b := StepBuilder{
 		Forge: getMockForge(t),
 		Repo:  &model.Repo{Config: ".woodpecker"},
-		Curr:  &model.Pipeline{},
+		Curr: &model.Pipeline{
+			Event: model.EventPush,
+		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
 		Secs:  []*model.Secret{},
@@ -272,11 +303,15 @@ func TestPipelineName(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Name: ".woodpecker/lint.yml", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
 `)},
 			{Name: ".woodpecker/.test.yml", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
@@ -300,7 +335,10 @@ func TestBranchFilter(t *testing.T) {
 	b := StepBuilder{
 		Forge: getMockForge(t),
 		Repo:  &model.Repo{},
-		Curr:  &model.Pipeline{Branch: "dev"},
+		Curr: &model.Pipeline{
+			Branch: "dev",
+			Event:  model.EventPush,
+		},
 		Last:  &model.Pipeline{},
 		Netrc: &model.Netrc{},
 		Secs:  []*model.Secret{},
@@ -308,12 +346,16 @@ func TestBranchFilter(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 steps:
   xxx:
     image: scratch
 branches: main
 `)},
 			{Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
@@ -371,9 +413,7 @@ steps:
 	}
 
 	pipelineItems, err := b.Build()
-	if err != nil {
+	assert.False(t, errors.HasBlockingErrors(err))
-		t.Fatal(err)
-	}
 
 	if len(pipelineItems) != 2 {
 		t.Fatal("Should have generated 2 pipelineItems")
@@ -383,7 +423,10 @@ steps:
 func TestZeroSteps(t *testing.T) {
 	t.Parallel()
 
-	pipeline := &model.Pipeline{Branch: "dev"}
+	pipeline := &model.Pipeline{
+		Branch: "dev",
+		Event:  model.EventPush,
+	}
 
 	b := StepBuilder{
 		Forge: getMockForge(t),
@@ -396,6 +439,8 @@ func TestZeroSteps(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Data: []byte(`
+when:
+  event: push
 skip_clone: true
 steps:
   build:
@@ -418,7 +463,10 @@ steps:
 func TestZeroStepsAsMultiPipelineDeps(t *testing.T) {
 	t.Parallel()
 
-	pipeline := &model.Pipeline{Branch: "dev"}
+	pipeline := &model.Pipeline{
+		Branch: "dev",
+		Event:  model.EventPush,
+	}
 
 	b := StepBuilder{
 		Forge: getMockForge(t),
@@ -431,6 +479,8 @@ func TestZeroStepsAsMultiPipelineDeps(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Name: "zerostep", Data: []byte(`
+when:
+  event: push
 skip_clone: true
 steps:
   build:
@@ -439,11 +489,15 @@ steps:
     image: scratch
 `)},
 			{Name: "justastep", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
 `)},
 			{Name: "shouldbefiltered", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
@@ -467,7 +521,10 @@ depends_on: [ zerostep ]
 func TestZeroStepsAsMultiPipelineTransitiveDeps(t *testing.T) {
 	t.Parallel()
 
-	pipeline := &model.Pipeline{Branch: "dev"}
+	pipeline := &model.Pipeline{
+		Branch: "dev",
+		Event:  model.EventPush,
+	}
 
 	b := StepBuilder{
 		Forge: getMockForge(t),
@@ -480,6 +537,8 @@ func TestZeroStepsAsMultiPipelineTransitiveDeps(t *testing.T) {
 		Host:  "",
 		Yamls: []*forge_types.FileMeta{
 			{Name: "zerostep", Data: []byte(`
+when:
+  event: push
 skip_clone: true
 steps:
   build:
@@ -488,17 +547,23 @@ steps:
     image: scratch
 `)},
 			{Name: "justastep", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
 `)},
 			{Name: "shouldbefiltered", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch
 depends_on: [ zerostep ]
 `)},
 			{Name: "shouldbefilteredtoo", Data: []byte(`
+when:
+  event: push
 steps:
   build:
     image: scratch