Deprecate and warn of list syntax of environment (#4358)

Co-authored-by: Robert Kaussow <xoxys@rknet.org>

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 {

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
 	}
 

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{

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 {

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": {

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
+}

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

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
 }
 

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

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

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