From 26929b928f176726d7a72fbb59a158266de4e9d9 Mon Sep 17 00:00:00 2001 From: Anbraten Date: Fri, 3 Nov 2023 21:42:52 +0100 Subject: [PATCH 1/5] Add deprecation warnings --- pipeline/errors/error.go | 4 +++ pipeline/frontend/yaml/linter/linter.go | 37 +++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/pipeline/errors/error.go b/pipeline/errors/error.go index 7223740657..d866a6cdcb 100644 --- a/pipeline/errors/error.go +++ b/pipeline/errors/error.go @@ -27,6 +27,10 @@ type LinterErrorData struct { Field string `json:"field"` } +type DeprecationErrorData struct { + Docs string `json:"docs"` +} + func (e *PipelineError) Error() string { return fmt.Sprintf("[%s] %s", e.Type, e.Message) } diff --git a/pipeline/frontend/yaml/linter/linter.go b/pipeline/frontend/yaml/linter/linter.go index 088d30d6f7..3c8aa55f13 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/pipeline/frontend/yaml/linter/linter.go @@ -19,6 +19,7 @@ import ( "go.uber.org/multierr" + "github.com/woodpecker-ci/woodpecker/pipeline/errors" "github.com/woodpecker-ci/woodpecker/pipeline/frontend/yaml/linter/schema" "github.com/woodpecker-ci/woodpecker/pipeline/frontend/yaml/types" ) @@ -165,8 +166,40 @@ func (l *Linter) lintSchema(rawConfig string) error { return linterErr } -func (l *Linter) lintDeprecations(_ *types.Workflow) error { - // TODO: add deprecation warnings +func (l *Linter) lintDeprecations(workflow *types.Workflow) error { + if workflow.PipelineDontUseIt.ContainerList != nil { + return &errors.PipelineError{ + Type: errors.PipelineErrorTypeDeprecation, + Message: "Please use 'steps:' instead of deprecated 'pipeline:' list", + Data: errors.DeprecationErrorData{ + Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", + }, + IsWarning: true, + } + } + + if workflow.PlatformDontUseIt != "" { + return &errors.PipelineError{ + Type: errors.PipelineErrorTypeDeprecation, + Message: "Please use labels instead of deprecated 'platform' filters", + Data: errors.DeprecationErrorData{ + Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", + }, + IsWarning: true, + } + } + + if !workflow.BranchesDontUseIt.IsEmpty() { + return &errors.PipelineError{ + Type: errors.PipelineErrorTypeDeprecation, + Message: "Please use global when instead of deprecated 'branches' filter", + Data: errors.DeprecationErrorData{ + Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", + }, + IsWarning: true, + } + } + return nil } From 9acd05783ed4083cce315072ab07f8e737ec68c4 Mon Sep 17 00:00:00 2001 From: Anbraten Date: Sat, 4 Nov 2023 08:14:49 +0100 Subject: [PATCH 2/5] return all errors --- pipeline/frontend/yaml/linter/linter.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pipeline/frontend/yaml/linter/linter.go b/pipeline/frontend/yaml/linter/linter.go index 3c8aa55f13..446ce356ce 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/pipeline/frontend/yaml/linter/linter.go @@ -166,41 +166,41 @@ func (l *Linter) lintSchema(rawConfig string) error { return linterErr } -func (l *Linter) lintDeprecations(workflow *types.Workflow) error { +func (l *Linter) lintDeprecations(workflow *types.Workflow) (err error) { if workflow.PipelineDontUseIt.ContainerList != nil { - return &errors.PipelineError{ + err = multierr.Append(err, &errors.PipelineError{ Type: errors.PipelineErrorTypeDeprecation, Message: "Please use 'steps:' instead of deprecated 'pipeline:' list", Data: errors.DeprecationErrorData{ Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", }, IsWarning: true, - } + }) } if workflow.PlatformDontUseIt != "" { - return &errors.PipelineError{ + err = multierr.Append(err, &errors.PipelineError{ Type: errors.PipelineErrorTypeDeprecation, Message: "Please use labels instead of deprecated 'platform' filters", Data: errors.DeprecationErrorData{ Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", }, IsWarning: true, - } + }) } if !workflow.BranchesDontUseIt.IsEmpty() { - return &errors.PipelineError{ + err = multierr.Append(err, &errors.PipelineError{ Type: errors.PipelineErrorTypeDeprecation, Message: "Please use global when instead of deprecated 'branches' filter", Data: errors.DeprecationErrorData{ Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", }, IsWarning: true, - } + }) } - return nil + return err } func (l *Linter) lintBadHabits(_ *types.Workflow) error { From a657cdd9546cf38af1d6bc7b3909dfa68320c33f Mon Sep 17 00:00:00 2001 From: Anbraten Date: Sat, 4 Nov 2023 12:11:50 +0100 Subject: [PATCH 3/5] improve tests --- cli/exec/exec.go | 6 +- cli/lint/lint.go | 11 +- pipeline/errors/error.go | 1 + pipeline/frontend/yaml/linter/error.go | 4 +- pipeline/frontend/yaml/linter/linter.go | 124 +++++++++++++------ pipeline/frontend/yaml/linter/linter_test.go | 15 ++- pipeline/stepBuilder.go | 6 +- 7 files changed, 120 insertions(+), 47 deletions(-) diff --git a/cli/exec/exec.go b/cli/exec/exec.go index 7df8a38590..3896899e17 100644 --- a/cli/exec/exec.go +++ b/cli/exec/exec.go @@ -167,7 +167,11 @@ func execWithAxis(c *cli.Context, file, repoPath string, axis matrix.Axis) error } // lint the yaml file - if lerr := linter.New(linter.WithTrusted(true)).Lint(confstr, conf); lerr != nil { + if lerr := linter.New(linter.WithTrusted(true)).Lint([]*linter.WorkflowConfig{{ + File: path.Base(file), + RawConfig: confstr, + Workflow: conf, + }}); lerr != nil { return lerr } diff --git a/cli/lint/lint.go b/cli/lint/lint.go index 643d855a39..dcfa9cb754 100644 --- a/cli/lint/lint.go +++ b/cli/lint/lint.go @@ -93,9 +93,16 @@ func lintFile(_ *cli.Context, file string) error { return err } - err = linter.New(linter.WithTrusted(true)).Lint(string(buf), c) + config := &linter.WorkflowConfig{ + File: path.Base(file), + RawConfig: rawConfig, + Workflow: c, + } + + // TODO: lint multiple files at once to allow checks for sth like "depends_on" to work + err = linter.New(linter.WithTrusted(true)).Lint([]*linter.WorkflowConfig{config}) if err != nil { - fmt.Printf("🔥 %s has errors:\n", output.String(path.Base(file)).Underline()) + fmt.Printf("🔥 %s has errors:\n", output.String().Underline()) hasErrors := true for _, err := range pipeline_errors.GetPipelineErrors(err) { diff --git a/pipeline/errors/error.go b/pipeline/errors/error.go index d866a6cdcb..7e55ffc02f 100644 --- a/pipeline/errors/error.go +++ b/pipeline/errors/error.go @@ -24,6 +24,7 @@ type PipelineError struct { } type LinterErrorData struct { + File string `json:"file"` Field string `json:"field"` } diff --git a/pipeline/frontend/yaml/linter/error.go b/pipeline/frontend/yaml/linter/error.go index 94ae0eb311..c6b23b4fa1 100644 --- a/pipeline/frontend/yaml/linter/error.go +++ b/pipeline/frontend/yaml/linter/error.go @@ -18,11 +18,11 @@ import ( "github.com/woodpecker-ci/woodpecker/pipeline/errors" ) -func newLinterError(message, field string, isWarning bool) *errors.PipelineError { +func newLinterError(message, file, field string, isWarning bool) *errors.PipelineError { return &errors.PipelineError{ Type: errors.PipelineErrorTypeLinter, Message: message, - Data: &errors.LinterErrorData{Field: field}, + Data: &errors.LinterErrorData{File: file, Field: field}, IsWarning: isWarning, } } diff --git a/pipeline/frontend/yaml/linter/linter.go b/pipeline/frontend/yaml/linter/linter.go index 446ce356ce..dce68468b9 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/pipeline/frontend/yaml/linter/linter.go @@ -17,6 +17,7 @@ package linter import ( "fmt" + "codeberg.org/6543/xyaml" "go.uber.org/multierr" "github.com/woodpecker-ci/woodpecker/pipeline/errors" @@ -38,50 +39,84 @@ func New(opts ...Option) *Linter { return linter } +type WorkflowConfig struct { + // File is the path to the configuration file. + File string + + // RawConfig is the raw configuration. + RawConfig string + + // Config is the parsed configuration. + Workflow *types.Workflow +} + // Lint lints the configuration. -func (l *Linter) Lint(rawConfig string, c *types.Workflow) error { +func (l *Linter) Lint(configs []*WorkflowConfig) error { + var linterErr error + + for _, config := range configs { + if err := l.lintFile(config); err != nil { + linterErr = multierr.Append(linterErr, err) + } + } + + return linterErr +} + +func (l *Linter) lintFile(config *WorkflowConfig) error { var linterErr error - if len(c.Steps.ContainerList) == 0 { - linterErr = multierr.Append(linterErr, newLinterError("Invalid or missing steps section", "steps", false)) + if len(config.Workflow.Steps.ContainerList) == 0 { + linterErr = multierr.Append(linterErr, newLinterError("Invalid or missing steps section", config.File, "steps", false)) } - if err := l.lintContainers(c.Clone.ContainerList); err != nil { + if err := l.lintContainers(config, "clone"); err != nil { linterErr = multierr.Append(linterErr, err) } - if err := l.lintContainers(c.Steps.ContainerList); err != nil { + if err := l.lintContainers(config, "steps"); err != nil { linterErr = multierr.Append(linterErr, err) } - if err := l.lintContainers(c.Services.ContainerList); err != nil { + if err := l.lintContainers(config, "services"); err != nil { linterErr = multierr.Append(linterErr, err) } - if err := l.lintSchema(rawConfig); err != nil { + if err := l.lintSchema(config); err != nil { linterErr = multierr.Append(linterErr, err) } - if err := l.lintDeprecations(c); err != nil { + if err := l.lintDeprecations(config); err != nil { linterErr = multierr.Append(linterErr, err) } - if err := l.lintBadHabits(c); err != nil { + if err := l.lintBadHabits(config); err != nil { linterErr = multierr.Append(linterErr, err) } return linterErr } -func (l *Linter) lintContainers(containers []*types.Container) error { +func (l *Linter) lintContainers(config *WorkflowConfig, area string) error { var linterErr error + var containers []*types.Container + + switch area { + case "clone": + containers = config.Workflow.Clone.ContainerList + case "steps": + containers = config.Workflow.Steps.ContainerList + case "services": + containers = config.Workflow.Services.ContainerList + } + for _, container := range containers { - if err := l.lintImage(container); err != nil { + if err := l.lintImage(config, container, area); err != nil { linterErr = multierr.Append(linterErr, err) } if !l.trusted { - if err := l.lintTrusted(container); err != nil { + if err := l.lintTrusted(config, container, area); err != nil { linterErr = multierr.Append(linterErr, err) } } - if err := l.lintCommands(container); err != nil { + if err := l.lintCommands(config, container, area); err != nil { linterErr = multierr.Append(linterErr, err) } } @@ -89,14 +124,14 @@ func (l *Linter) lintContainers(containers []*types.Container) error { return linterErr } -func (l *Linter) lintImage(c *types.Container) error { +func (l *Linter) lintImage(config *WorkflowConfig, c *types.Container, area string) error { if len(c.Image) == 0 { - return newLinterError("Invalid or missing image", fmt.Sprintf("steps.%s", c.Name), false) + return newLinterError("Invalid or missing image", config.File, fmt.Sprintf("%s.%s", area, c.Name), false) } return nil } -func (l *Linter) lintCommands(c *types.Container) error { +func (l *Linter) lintCommands(config *WorkflowConfig, c *types.Container, field string) error { if len(c.Commands) == 0 { return nil } @@ -105,58 +140,65 @@ func (l *Linter) lintCommands(c *types.Container) error { for key := range c.Settings { keys = append(keys, key) } - return newLinterError(fmt.Sprintf("Cannot configure both commands and custom attributes %v", keys), fmt.Sprintf("steps.%s", c.Name), false) + return newLinterError(fmt.Sprintf("Cannot configure both commands and custom attributes %v", keys), config.File, fmt.Sprintf("%s.%s", field, c.Name), false) } return nil } -func (l *Linter) lintTrusted(c *types.Container) error { - yamlPath := fmt.Sprintf("steps.%s", c.Name) +func (l *Linter) lintTrusted(config *WorkflowConfig, c *types.Container, area string) error { + yamlPath := fmt.Sprintf("%s.%s", area, c.Name) + err := "" if c.Privileged { - return newLinterError("Insufficient privileges to use privileged mode", yamlPath, false) + err = "Insufficient privileges to use privileged mode" } if c.ShmSize != 0 { - return newLinterError("Insufficient privileges to override shm_size", yamlPath, false) + err = "Insufficient privileges to override shm_size" } if len(c.DNS) != 0 { - return newLinterError("Insufficient privileges to use custom dns", yamlPath, false) + err = "Insufficient privileges to use custom dns" } if len(c.DNSSearch) != 0 { - return newLinterError("Insufficient privileges to use dns_search", yamlPath, false) + err = "Insufficient privileges to use dns_search" } if len(c.Devices) != 0 { - return newLinterError("Insufficient privileges to use devices", yamlPath, false) + err = "Insufficient privileges to use devices" } if len(c.ExtraHosts) != 0 { - return newLinterError("Insufficient privileges to use extra_hosts", yamlPath, false) + err = "Insufficient privileges to use extra_hosts" } if len(c.NetworkMode) != 0 { - return newLinterError("Insufficient privileges to use network_mode", yamlPath, false) + err = "Insufficient privileges to use network_mode" } if len(c.IpcMode) != 0 { - return newLinterError("Insufficient privileges to use ipc_mode", yamlPath, false) + err = "Insufficient privileges to use ipc_mode" } if len(c.Sysctls) != 0 { - return newLinterError("Insufficient privileges to use sysctls", yamlPath, false) + err = "Insufficient privileges to use sysctls" } if c.Networks.Networks != nil && len(c.Networks.Networks) != 0 { - return newLinterError("Insufficient privileges to use networks", yamlPath, false) + err = "Insufficient privileges to use networks" } if c.Volumes.Volumes != nil && len(c.Volumes.Volumes) != 0 { - return newLinterError("Insufficient privileges to use volumes", yamlPath, false) + err = "Insufficient privileges to use volumes" } if len(c.Tmpfs) != 0 { - return newLinterError("Insufficient privileges to use tmpfs", yamlPath, false) + err = "Insufficient privileges to use tmpfs" } + + if len(err) != 0 { + return newLinterError(err, config.File, yamlPath, false) + } + return nil } -func (l *Linter) lintSchema(rawConfig string) error { +func (l *Linter) lintSchema(config *WorkflowConfig) error { var linterErr error - schemaErrors, err := schema.LintString(rawConfig) + schemaErrors, err := schema.LintString(config.RawConfig) if err != nil { for _, schemaError := range schemaErrors { linterErr = multierr.Append(linterErr, newLinterError( + config.File, schemaError.Description(), schemaError.Field(), true, // TODO: let pipelines fail if the schema is invalid @@ -166,8 +208,14 @@ func (l *Linter) lintSchema(rawConfig string) error { return linterErr } -func (l *Linter) lintDeprecations(workflow *types.Workflow) (err error) { - if workflow.PipelineDontUseIt.ContainerList != nil { +func (l *Linter) lintDeprecations(config *WorkflowConfig) (err error) { + parsed := new(types.Workflow) + err = xyaml.Unmarshal([]byte(config.RawConfig), parsed) + if err != nil { + return err + } + + if parsed.PipelineDontUseIt.ContainerList != nil { err = multierr.Append(err, &errors.PipelineError{ Type: errors.PipelineErrorTypeDeprecation, Message: "Please use 'steps:' instead of deprecated 'pipeline:' list", @@ -178,7 +226,7 @@ func (l *Linter) lintDeprecations(workflow *types.Workflow) (err error) { }) } - if workflow.PlatformDontUseIt != "" { + if parsed.PlatformDontUseIt != "" { err = multierr.Append(err, &errors.PipelineError{ Type: errors.PipelineErrorTypeDeprecation, Message: "Please use labels instead of deprecated 'platform' filters", @@ -189,7 +237,7 @@ func (l *Linter) lintDeprecations(workflow *types.Workflow) (err error) { }) } - if !workflow.BranchesDontUseIt.IsEmpty() { + if parsed.BranchesDontUseIt != nil { err = multierr.Append(err, &errors.PipelineError{ Type: errors.PipelineErrorTypeDeprecation, Message: "Please use global when instead of deprecated 'branches' filter", @@ -203,7 +251,7 @@ func (l *Linter) lintDeprecations(workflow *types.Workflow) (err error) { return err } -func (l *Linter) lintBadHabits(_ *types.Workflow) error { +func (l *Linter) lintBadHabits(_ *WorkflowConfig) error { // TODO: add bad habit warnings return nil } diff --git a/pipeline/frontend/yaml/linter/linter_test.go b/pipeline/frontend/yaml/linter/linter_test.go index 9c1c5ff400..0bad7fc4db 100644 --- a/pipeline/frontend/yaml/linter/linter_test.go +++ b/pipeline/frontend/yaml/linter/linter_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package linter +package linter_test import ( "testing" @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/woodpecker-ci/woodpecker/pipeline/errors" "github.com/woodpecker-ci/woodpecker/pipeline/frontend/yaml" + "github.com/woodpecker-ci/woodpecker/pipeline/frontend/yaml/linter" ) func TestLint(t *testing.T) { @@ -82,7 +83,11 @@ steps: t.Fatalf("Cannot unmarshal yaml %q. Error: %s", testd.Title, err) } - if err := New(WithTrusted(true)).Lint(testd.Data, conf); err != nil { + if err := linter.New(linter.WithTrusted(true)).Lint([]*linter.WorkflowConfig{{ + File: testd.Title, + RawConfig: testd.Data, + Workflow: conf, + }}); err != nil { t.Errorf("Expected lint returns no errors, got %q", err) } }) @@ -155,7 +160,11 @@ func TestLintErrors(t *testing.T) { t.Fatalf("Cannot unmarshal yaml %q. Error: %s", test.from, err) } - lerr := New().Lint(test.from, conf) + lerr := linter.New().Lint([]*linter.WorkflowConfig{{ + File: test.from, + RawConfig: test.from, + Workflow: conf, + }}) if lerr == nil { t.Errorf("Expected lint error for configuration %q", test.from) } diff --git a/pipeline/stepBuilder.go b/pipeline/stepBuilder.go index 57232f9558..adb8be2da7 100644 --- a/pipeline/stepBuilder.go +++ b/pipeline/stepBuilder.go @@ -144,7 +144,11 @@ func (b *StepBuilder) genItemForWorkflow(workflow *model.Workflow, axis matrix.A // lint pipeline errorsAndWarnings = multierr.Append(errorsAndWarnings, linter.New( linter.WithTrusted(b.Repo.IsTrusted), - ).Lint(substituted, parsed)) + ).Lint([]*linter.WorkflowConfig{{ + Workflow: parsed, + File: workflow.Name, + RawConfig: data, + }})) if pipeline_errors.HasBlockingErrors(errorsAndWarnings) { return nil, errorsAndWarnings } From ea63f597cdc3bf60537bc21b2986a563adbe0ac6 Mon Sep 17 00:00:00 2001 From: Anbraten Date: Sat, 4 Nov 2023 12:15:24 +0100 Subject: [PATCH 4/5] show file name --- web/src/lib/api/types/pipeline.ts | 4 ++-- web/src/views/repo/pipeline/PipelineErrors.vue | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/web/src/lib/api/types/pipeline.ts b/web/src/lib/api/types/pipeline.ts index d98eb8cd62..9301904472 100644 --- a/web/src/lib/api/types/pipeline.ts +++ b/web/src/lib/api/types/pipeline.ts @@ -1,9 +1,9 @@ import { WebhookEvents } from './webhook'; -export type PipelineError = { +export type PipelineError = { type: string; message: string; - data?: unknown; + data?: D; is_warning: boolean; }; diff --git a/web/src/views/repo/pipeline/PipelineErrors.vue b/web/src/views/repo/pipeline/PipelineErrors.vue index 4f0dea7a73..d5a789bce0 100644 --- a/web/src/views/repo/pipeline/PipelineErrors.vue +++ b/web/src/views/repo/pipeline/PipelineErrors.vue @@ -4,7 +4,10 @@ @@ -16,12 +19,21 @@ import { inject, Ref } from 'vue'; import Panel from '~/components/layout/Panel.vue'; -import { Pipeline } from '~/lib/api/types'; +import type { Pipeline, PipelineError } from '~/lib/api/types'; const pipeline = inject>('pipeline'); if (!pipeline) { throw new Error('Unexpected: "pipeline" should be provided at this place'); } + +type LinterError = PipelineError<{ + file?: string; + field: string; +}>; + +function isLinterError(error: PipelineError): error is LinterError { + return error.type === 'linter'; +}