From a0f2ee9506755ea0af7d64f856bc1e81bc2147da Mon Sep 17 00:00:00 2001 From: Anbraten Date: Sat, 4 Nov 2023 15:30:47 +0100 Subject: [PATCH] Add deprecation warnings (#2725) --- cli/exec/exec.go | 6 +- cli/lint/lint.go | 11 +- pipeline/errors/error.go | 7 + pipeline/frontend/yaml/linter/error.go | 4 +- pipeline/frontend/yaml/linter/linter.go | 161 ++++++++++++++---- pipeline/frontend/yaml/linter/linter_test.go | 15 +- pipeline/stepBuilder.go | 6 +- web/src/lib/api/types/pipeline.ts | 4 +- .../views/repo/pipeline/PipelineErrors.vue | 24 ++- 9 files changed, 187 insertions(+), 51 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..3e8115239f 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(config.File).Underline()) hasErrors := true for _, err := range pipeline_errors.GetPipelineErrors(err) { diff --git a/pipeline/errors/error.go b/pipeline/errors/error.go index 7223740657..991c83f8d8 100644 --- a/pipeline/errors/error.go +++ b/pipeline/errors/error.go @@ -24,9 +24,16 @@ type PipelineError struct { } type LinterErrorData struct { + File string `json:"file"` Field string `json:"field"` } +type DeprecationErrorData struct { + File string `json:"file"` + Field string `json:"field"` + 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/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 088d30d6f7..f98f40545b 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/pipeline/frontend/yaml/linter/linter.go @@ -17,8 +17,10 @@ package linter import ( "fmt" + "codeberg.org/6543/xyaml" "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" ) @@ -37,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 - if len(c.Steps.ContainerList) == 0 { - linterErr = multierr.Append(linterErr, newLinterError("Invalid or missing steps section", "steps", false)) + for _, config := range configs { + if err := l.lintFile(config); err != nil { + linterErr = multierr.Append(linterErr, err) + } } - if err := l.lintContainers(c.Clone.ContainerList); err != nil { + return linterErr +} + +func (l *Linter) lintFile(config *WorkflowConfig) error { + var linterErr error + + 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(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) } } @@ -88,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 } @@ -104,59 +140,66 @@ 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( schemaError.Description(), + config.File, schemaError.Field(), true, // TODO: let pipelines fail if the schema is invalid )) @@ -165,12 +208,56 @@ func (l *Linter) lintSchema(rawConfig string) error { return linterErr } -func (l *Linter) lintDeprecations(_ *types.Workflow) error { - // TODO: add deprecation warnings - return 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", + Data: errors.DeprecationErrorData{ + File: config.File, + Field: "pipeline", + Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", + }, + IsWarning: true, + }) + } + + if parsed.PlatformDontUseIt != "" { + err = multierr.Append(err, &errors.PipelineError{ + Type: errors.PipelineErrorTypeDeprecation, + Message: "Please use labels instead of deprecated 'platform' filters", + Data: errors.DeprecationErrorData{ + File: config.File, + Field: "platform", + Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", + }, + IsWarning: true, + }) + } + + if parsed.BranchesDontUseIt != nil { + err = multierr.Append(err, &errors.PipelineError{ + Type: errors.PipelineErrorTypeDeprecation, + Message: "Please use global when instead of deprecated 'branches' filter", + Data: errors.DeprecationErrorData{ + File: config.File, + Field: "branches", + Docs: "https://woodpecker-ci.org/docs/next/migrations#next-200", + }, + IsWarning: true, + }) + } + + 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 } 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..46a268357b 100644 --- a/web/src/views/repo/pipeline/PipelineErrors.vue +++ b/web/src/views/repo/pipeline/PipelineErrors.vue @@ -4,9 +4,17 @@ @@ -16,12 +24,22 @@ 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'); } + +function isLinterError(error: PipelineError): error is PipelineError<{ file?: string; field: string }> { + return error.type === 'linter'; +} + +function isDeprecationError( + error: PipelineError, +): error is PipelineError<{ file: string; field: string; docs: string }> { + return error.type === 'deprecation'; +}