From 092ea7b9076e801c7589a2d1b2c48d4b1391b228 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 17 Nov 2024 08:17:09 +0100 Subject: [PATCH] Deprecate and warn of list syntax of environment (#4358) Co-authored-by: Robert Kaussow --- pipeline/frontend/yaml/compiler/compiler.go | 5 +- pipeline/frontend/yaml/compiler/convert.go | 2 +- pipeline/frontend/yaml/linter/linter.go | 52 ++++++++++------ pipeline/frontend/yaml/linter/linter_test.go | 10 ++- .../frontend/yaml/linter/schema/schema.json | 2 + .../yaml/types/base/deprecatedSliceOrMap.go | 61 +++++++++++++++++++ .../types/base/deprecatedSliceOrMap_test.go | 49 +++++++++++++++ pipeline/frontend/yaml/types/container.go | 8 +-- .../frontend/yaml/types/container_test.go | 2 +- pipeline/frontend/yaml/types/secret.go | 4 +- pipeline/frontend/yaml/types/secret_test.go | 53 +++++++++------- 11 files changed, 198 insertions(+), 50 deletions(-) create mode 100644 pipeline/frontend/yaml/types/base/deprecatedSliceOrMap.go create mode 100644 pipeline/frontend/yaml/types/base/deprecatedSliceOrMap_test.go diff --git a/pipeline/frontend/yaml/compiler/compiler.go b/pipeline/frontend/yaml/compiler/compiler.go index 9525a3a12..24240ce1d 100644 --- a/pipeline/frontend/yaml/compiler/compiler.go +++ b/pipeline/frontend/yaml/compiler/compiler.go @@ -21,6 +21,7 @@ import ( backend_types "go.woodpecker-ci.org/woodpecker/v2/pipeline/backend/types" "go.woodpecker-ci.org/woodpecker/v2/pipeline/frontend/metadata" yaml_types "go.woodpecker-ci.org/woodpecker/v2/pipeline/frontend/yaml/types" + "go.woodpecker-ci.org/woodpecker/v2/pipeline/frontend/yaml/types/base" "go.woodpecker-ci.org/woodpecker/v2/pipeline/frontend/yaml/utils" "go.woodpecker-ci.org/woodpecker/v2/shared/constant" ) @@ -178,10 +179,10 @@ func (c *Compiler) Compile(conf *yaml_types.Workflow) (*backend_types.Config, er Name: defaultCloneName, Image: cloneImage, Settings: cloneSettings, - Environment: make(map[string]any), + Environment: base.DeprecatedSliceOrMap{Map: make(map[string]any)}, } for k, v := range c.cloneEnv { - container.Environment[k] = v + container.Environment.Map[k] = v } step, err := c.createProcess(container, backend_types.StepTypeClone) if err != nil { diff --git a/pipeline/frontend/yaml/compiler/convert.go b/pipeline/frontend/yaml/compiler/convert.go index a99e1aef5..e0daf6eeb 100644 --- a/pipeline/frontend/yaml/compiler/convert.go +++ b/pipeline/frontend/yaml/compiler/convert.go @@ -121,7 +121,7 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe } } - if err := settings.ParamsToEnv(container.Environment, environment, "", false, getSecretValue); err != nil { + if err := settings.ParamsToEnv(container.Environment.Map, environment, "", false, getSecretValue); err != nil { return nil, err } diff --git a/pipeline/frontend/yaml/linter/linter.go b/pipeline/frontend/yaml/linter/linter.go index 0413debf3..e081142ac 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/pipeline/frontend/yaml/linter/linter.go @@ -126,6 +126,9 @@ func (l *Linter) lintContainers(config *WorkflowConfig, area string) error { if err := l.lintPrivilegedPlugins(config, container, area); err != nil { linterErr = multierr.Append(linterErr, err) } + if err := l.lintContainerDeprecations(config, container, area); err != nil { + linterErr = multierr.Append(linterErr, err) + } } return linterErr @@ -160,7 +163,7 @@ func (l *Linter) lintSettings(config *WorkflowConfig, c *types.Container, field if len(c.Entrypoint) != 0 { return newLinterError("Cannot configure both entrypoint and settings", config.File, fmt.Sprintf("%s.%s", field, c.Name), false) } - if len(c.Environment) != 0 { + if len(c.Environment.Map) != 0 { return newLinterError("Should not configure both environment and settings", config.File, fmt.Sprintf("%s.%s", field, c.Name), true) } if len(c.Secrets.Secrets) != 0 { @@ -169,6 +172,36 @@ func (l *Linter) lintSettings(config *WorkflowConfig, c *types.Container, field return nil } +func (l *Linter) lintContainerDeprecations(config *WorkflowConfig, c *types.Container, field string) (err error) { + if c.Environment.WasSlice { + err = multierr.Append(err, &errorTypes.PipelineError{ + Type: errorTypes.PipelineErrorTypeDeprecation, + Message: "List syntax for `environment` is deprecated, use map syntax instead", + Data: errors.DeprecationErrorData{ + File: config.File, + Field: fmt.Sprintf("%s.%s.environment", field, c.Name), + Docs: "https://woodpecker-ci.org/docs/usage/environment", + }, + IsWarning: true, + }) + } + + if c.Secrets.LegacyFormat { + err = multierr.Append(err, &errorTypes.PipelineError{ + Type: errorTypes.PipelineErrorTypeDeprecation, + Message: "Alternative names syntax for `secrets` is deprecated, use list syntax or `from_secret` instead", + Data: errors.DeprecationErrorData{ + File: config.File, + Field: fmt.Sprintf("%s.%s.secrets", field, c.Name), + Docs: "https://woodpecker-ci.org/docs/usage/secrets#use-secrets-in-settings-and-environment", + }, + IsWarning: true, + }) + } + + return err +} + func (l *Linter) lintTrusted(config *WorkflowConfig, c *types.Container, area string) error { yamlPath := fmt.Sprintf("%s.%s", area, c.Name) errors := []string{} @@ -322,23 +355,6 @@ func (l *Linter) lintDeprecations(config *WorkflowConfig) (err error) { } } - for _, step := range parsed.Steps.ContainerList { - for i, c := range step.Secrets.Secrets { - if c.Source != c.Target { - err = multierr.Append(err, &errorTypes.PipelineError{ - Type: errorTypes.PipelineErrorTypeDeprecation, - Message: "Secrets alternative names are deprecated, use environment with from_secret", - Data: errors.DeprecationErrorData{ - File: config.File, - Field: fmt.Sprintf("steps.%s.secrets[%d]", step.Name, i), - Docs: "https://woodpecker-ci.org/docs/usage/secrets#use-secrets-in-settings-and-environment", - }, - IsWarning: true, - }) - } - } - } - for i, c := range parsed.When.Constraints { if !c.Environment.IsEmpty() { err = multierr.Append(err, &errorTypes.PipelineError{ diff --git a/pipeline/frontend/yaml/linter/linter_test.go b/pipeline/frontend/yaml/linter/linter_test.go index cf1443b6e..63c5be485 100644 --- a/pipeline/frontend/yaml/linter/linter_test.go +++ b/pipeline/frontend/yaml/linter/linter_test.go @@ -162,13 +162,21 @@ func TestLintErrors(t *testing.T) { want: "Cannot configure both entrypoint and settings", }, { - from: "steps: { build: { image: golang, settings: { test: 'true' }, environment: [ 'TEST=true' ] } }", + from: "steps: { build: { image: golang, settings: { test: 'true' }, environment: { TEST: 'true' } } }", want: "Should not configure both environment and settings", }, { from: "{steps: { build: { image: plugins/docker, settings: { test: 'true' } } }, when: { branch: main, event: push } } }", want: "Cannot use once by default privileged plugin 'plugins/docker', if needed add it too WOODPECKER_PLUGINS_PRIVILEGED", }, + { + from: "steps: { build: { image: golang, environment: [ 'TEST=true' ] } }", + want: "List syntax for `environment` is deprecated, use map syntax instead", + }, + { + from: "steps: { build: { image: golang, secrets: [ { source: mysql_username, target: mysql_username } ] } }", + want: "Alternative names syntax for `secrets` is deprecated, use list syntax or `from_secret` instead", + }, } for _, test := range testdata { diff --git a/pipeline/frontend/yaml/linter/schema/schema.json b/pipeline/frontend/yaml/linter/schema/schema.json index 73e2af3d5..5e62a6a46 100644 --- a/pipeline/frontend/yaml/linter/schema/schema.json +++ b/pipeline/frontend/yaml/linter/schema/schema.json @@ -805,6 +805,7 @@ "oneOf": [ { "type": "array", + "description": "deprecated", "items": { "type": "string" }, @@ -828,6 +829,7 @@ }, { "type": "object", + "description": "deprecated", "required": ["source", "target"], "properties": { "source": { diff --git a/pipeline/frontend/yaml/types/base/deprecatedSliceOrMap.go b/pipeline/frontend/yaml/types/base/deprecatedSliceOrMap.go new file mode 100644 index 000000000..699d5e205 --- /dev/null +++ b/pipeline/frontend/yaml/types/base/deprecatedSliceOrMap.go @@ -0,0 +1,61 @@ +// Copyright 2023 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package base + +import ( + "errors" + "fmt" + "strings" +) + +// DeprecatedSliceOrMap represents a map of strings, string slice are converted into a map. +type DeprecatedSliceOrMap struct { + Map map[string]any + WasSlice bool +} + +// UnmarshalYAML implements the Unmarshaler interface. +func (s *DeprecatedSliceOrMap) UnmarshalYAML(unmarshal func(any) error) error { + *s = DeprecatedSliceOrMap{} + var sliceType []any + if err := unmarshal(&sliceType); err == nil { + parts := map[string]any{} + for _, s := range sliceType { + if str, ok := s.(string); ok { + str := strings.TrimSpace(str) + key, val, _ := strings.Cut(str, "=") + parts[key] = val + } else { + return fmt.Errorf("cannot unmarshal '%v' of type %T into a string value", s, s) + } + } + s.Map = parts + s.WasSlice = true + return nil + } + + var mapType map[string]any + if err := unmarshal(&mapType); err == nil { + s.Map = mapType + return nil + } + + return errors.New("failed to unmarshal DeprecatedSliceOrMap") +} + +// MarshalYAML implements custom Yaml marshaling. +func (s DeprecatedSliceOrMap) MarshalYAML() (any, error) { + return s.Map, nil +} diff --git a/pipeline/frontend/yaml/types/base/deprecatedSliceOrMap_test.go b/pipeline/frontend/yaml/types/base/deprecatedSliceOrMap_test.go new file mode 100644 index 000000000..2a8087b83 --- /dev/null +++ b/pipeline/frontend/yaml/types/base/deprecatedSliceOrMap_test.go @@ -0,0 +1,49 @@ +// Copyright 2024 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package base + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v3" +) + +type StructDeprecatedSliceOrMap struct { + Foos DeprecatedSliceOrMap `yaml:"foos,omitempty"` + Bars []string `yaml:"bars,omitempty"` +} + +func TestDeprecatedSliceOrMapYaml(t *testing.T) { + str := `{foos: [bar=baz, far=faz]}` + + s := StructDeprecatedSliceOrMap{} + assert.NoError(t, yaml.Unmarshal([]byte(str), &s)) + + assert.Equal(t, DeprecatedSliceOrMap{Map: map[string]any{"bar": "baz", "far": "faz"}, WasSlice: true}, s.Foos) + + d, err := yaml.Marshal(&s) + assert.NoError(t, err) + str = `foos: + bar: baz + far: faz +` + assert.EqualValues(t, str, string(d)) + + s2 := StructDeprecatedSliceOrMap{} + assert.NoError(t, yaml.Unmarshal(d, &s2)) + + assert.Equal(t, DeprecatedSliceOrMap{Map: map[string]any{"bar": "baz", "far": "faz"}, WasSlice: false}, s2.Foos) +} diff --git a/pipeline/frontend/yaml/types/container.go b/pipeline/frontend/yaml/types/container.go index edfacd5c2..2ff9317f6 100644 --- a/pipeline/frontend/yaml/types/container.go +++ b/pipeline/frontend/yaml/types/container.go @@ -49,10 +49,10 @@ type ( Ports []string `yaml:"ports,omitempty"` DependsOn base.StringOrSlice `yaml:"depends_on,omitempty"` - // TODO: make []string in 3.x + // NOTE: only []string in 3.x Secrets Secrets `yaml:"secrets,omitempty"` - // TODO: make map[string]any in 3.x - Environment base.SliceOrMap `yaml:"environment,omitempty"` + // NOTE: only map[string]any allowed in 3.x + Environment base.DeprecatedSliceOrMap `yaml:"environment,omitempty"` // Docker and Kubernetes Specific Privileged bool `yaml:"privileged,omitempty"` @@ -124,7 +124,7 @@ func (c *ContainerList) UnmarshalYAML(value *yaml.Node) error { func (c *Container) IsPlugin() bool { return len(c.Commands) == 0 && len(c.Entrypoint) == 0 && - len(c.Environment) == 0 && + len(c.Environment.Map) == 0 && len(c.Secrets.Secrets) == 0 } diff --git a/pipeline/frontend/yaml/types/container_test.go b/pipeline/frontend/yaml/types/container_test.go index 3573fbc13..8866278a7 100644 --- a/pipeline/frontend/yaml/types/container_test.go +++ b/pipeline/frontend/yaml/types/container_test.go @@ -88,7 +88,7 @@ func TestUnmarshalContainer(t *testing.T) { DNS: base.StringOrSlice{"8.8.8.8"}, DNSSearch: base.StringOrSlice{"example.com"}, Entrypoint: []string{"/bin/sh", "-c"}, - Environment: base.SliceOrMap{"RACK_ENV": "development", "SHOW": "true"}, + Environment: base.DeprecatedSliceOrMap{Map: map[string]any{"RACK_ENV": "development", "SHOW": "true"}, WasSlice: true}, ExtraHosts: []string{"somehost:162.242.195.82", "otherhost:50.31.209.229", "ipv6:2001:db8::10"}, Image: "golang:latest", MemLimit: base.MemStringOrInt(1024), diff --git a/pipeline/frontend/yaml/types/secret.go b/pipeline/frontend/yaml/types/secret.go index 9958b41aa..85aacd7e1 100644 --- a/pipeline/frontend/yaml/types/secret.go +++ b/pipeline/frontend/yaml/types/secret.go @@ -19,7 +19,8 @@ import "gopkg.in/yaml.v3" type ( // Secrets defines a collection of secrets. Secrets struct { - Secrets []*Secret + Secrets []*Secret + LegacyFormat bool } // Secret defines a container secret. @@ -44,5 +45,6 @@ func (s *Secrets) UnmarshalYAML(value *yaml.Node) error { } return nil } + s.LegacyFormat = true return yaml.Unmarshal(y, &s.Secrets) } diff --git a/pipeline/frontend/yaml/types/secret_test.go b/pipeline/frontend/yaml/types/secret_test.go index fcfc6d212..b6010cdd2 100644 --- a/pipeline/frontend/yaml/types/secret_test.go +++ b/pipeline/frontend/yaml/types/secret_test.go @@ -24,41 +24,50 @@ import ( func TestUnmarshalSecrets(t *testing.T) { testdata := []struct { from string - want []*Secret + want Secrets }{ { from: "[ mysql_username, mysql_password]", - want: []*Secret{ - { - Source: "mysql_username", - Target: "mysql_username", - }, - { - Source: "mysql_password", - Target: "mysql_password", + want: Secrets{ + Secrets: []*Secret{ + { + Source: "mysql_username", + Target: "mysql_username", + }, + { + Source: "mysql_password", + Target: "mysql_password", + }, }, + LegacyFormat: false, }, }, { from: "[ { source: mysql_prod_username, target: mysql_username } ]", - want: []*Secret{ - { - Source: "mysql_prod_username", - Target: "mysql_username", + want: Secrets{ + Secrets: []*Secret{ + { + Source: "mysql_prod_username", + Target: "mysql_username", + }, }, + LegacyFormat: true, }, }, { from: "[ { source: mysql_prod_username, target: mysql_username }, { source: redis_username, target: redis_username } ]", - want: []*Secret{ - { - Source: "mysql_prod_username", - Target: "mysql_username", - }, - { - Source: "redis_username", - Target: "redis_username", + want: Secrets{ + Secrets: []*Secret{ + { + Source: "mysql_prod_username", + Target: "mysql_username", + }, + { + Source: "redis_username", + Target: "redis_username", + }, }, + LegacyFormat: true, }, }, } @@ -68,6 +77,6 @@ func TestUnmarshalSecrets(t *testing.T) { got := Secrets{} err := yaml.Unmarshal(in, &got) assert.NoError(t, err) - assert.EqualValues(t, test.want, got.Secrets, "problem parsing secrets %q", test.from) + assert.EqualValues(t, test.want, got, "problem parsing secrets %q", test.from) } }