diff --git a/cmd/server/docs/docs.go b/cmd/server/docs/docs.go index 9536f0f34..6c1d65c4e 100644 --- a/cmd/server/docs/docs.go +++ b/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": { diff --git a/docs/docs/91-migrations.md b/docs/docs/91-migrations.md index 328d87a62..8d4cc08bb 100644 --- a/docs/docs/91-migrations.md +++ b/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 diff --git a/pipeline/errors/error.go b/pipeline/errors/error.go index 3de242205..1a0ae167c 100644 --- a/pipeline/errors/error.go +++ b/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 { diff --git a/pipeline/frontend/yaml/constraint/constraint.go b/pipeline/frontend/yaml/constraint/constraint.go index 2f7f8cdc0..1d08ecb09 100644 --- a/pipeline/frontend/yaml/constraint/constraint.go +++ b/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. diff --git a/pipeline/frontend/yaml/linter/linter.go b/pipeline/frontend/yaml/linter/linter.go index 78a5a0711..8c7e9ae70 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/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 { - // TODO: add bad habit warnings - return nil +func (l *Linter) lintBadHabits(config *WorkflowConfig) (err error) { + parsed := new(types.Workflow) + 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 } diff --git a/pipeline/frontend/yaml/linter/linter_test.go b/pipeline/frontend/yaml/linter/linter_test.go index 50bc4092b..0c49e44ad 100644 --- a/pipeline/frontend/yaml/linter/linter_test.go +++ b/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) + } +} diff --git a/server/pipeline/stepbuilder/stepBuilder_test.go b/server/pipeline/stepbuilder/stepBuilder_test.go index 559a7f6a3..98cb79129 100644 --- a/server/pipeline/stepbuilder/stepBuilder_test.go +++ b/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 { - t.Fatal(err) - } + assert.False(t, errors.HasBlockingErrors(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