Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deprecation warnings #2725

Merged
merged 10 commits into from
Nov 4, 2023
6 changes: 5 additions & 1 deletion cli/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
11 changes: 9 additions & 2 deletions cli/lint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
7 changes: 7 additions & 0 deletions pipeline/errors/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions pipeline/frontend/yaml/linter/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
161 changes: 124 additions & 37 deletions pipeline/frontend/yaml/linter/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
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"
)
Expand All @@ -37,65 +39,99 @@
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)
}
}

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
}
Expand All @@ -104,59 +140,66 @@
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)

Check warning on line 143 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L143

Added line #L143 was not covered by tests
}
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"

Check warning on line 173 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L173

Added line #L173 was not covered by tests
}
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"
}

Check warning on line 186 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L185-L186

Added lines #L185 - L186 were not covered by tests

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
))
Expand All @@ -165,12 +208,56 @@
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
}

Check warning on line 216 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L215-L216

Added lines #L215 - L216 were not covered by tests

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,
})
}

Check warning on line 229 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L219-L229

Added lines #L219 - L229 were not covered by tests

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,
})
}

Check warning on line 242 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L232-L242

Added lines #L232 - L242 were not covered by tests

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,
})
}

Check warning on line 255 in pipeline/frontend/yaml/linter/linter.go

View check run for this annotation

Codecov / codecov/patch

pipeline/frontend/yaml/linter/linter.go#L245-L255

Added lines #L245 - L255 were not covered by tests

return err
}

func (l *Linter) lintBadHabits(_ *types.Workflow) error {
func (l *Linter) lintBadHabits(_ *WorkflowConfig) error {
// TODO: add bad habit warnings
return nil
}
15 changes: 12 additions & 3 deletions pipeline/frontend/yaml/linter/linter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package linter
package linter_test

import (
"testing"

"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) {
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
Expand Down
6 changes: 5 additions & 1 deletion pipeline/stepBuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading
Loading