Skip to content

Commit

Permalink
Add deprecation warnings (#2725)
Browse files Browse the repository at this point in the history
  • Loading branch information
@anbraten
anbraten authored Nov 4, 2023
1 parent 33f9574 commit a0f2ee9
Show file tree
Hide file tree
Showing 9 changed files with 187 additions and 51 deletions.
6 changes: 5 additions & 1 deletion cli/exec/exec.go
View file
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
View file
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
View file
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
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -37,65 +39,99 @@ 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)
}
}

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 @@ 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
))
Expand All @@ -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
}
15 changes: 12 additions & 3 deletions pipeline/frontend/yaml/linter/linter_test.go
View file
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
View file
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

0 comments on commit a0f2ee9

Please sign in to comment.