Remove support for ${} syntax 🗑️

In #850 we decided that to make our syntax more similar to k8s style
syntax, we will use $() for variable substitution instead of ${}. In a
later iteration we may also want to make it so that anything that can be
acesssed as a variable is also available as an env var to the running
container, but that is TBD.

In #1172 we added support for the new syntax, $(), and continued support
for ${}, which was released in 0.6. This commit removes support for
${}.

Fixes #1170

Author: bobcatfish

docs/pipelines.md

@@ -91,9 +91,7 @@ parameters can be passed to the `Pipeline` from a `PipelineRun`.
 
 Input parameters in the form of `$(params.foo)` are replaced inside of the
 [`PipelineTask` parameters' values](#pipeline-tasks) (see also
-[variable substitution](tasks.md#variable-substitution)). _As with
-[variable substitution](tasks.md#variable-substitution), the deprecated syntax
-`${params.foo}` will be supported until [#1170](https://github.com/tektoncd/pipeline/issues/1170)._
+[variable substitution](tasks.md#variable-substitution)).
 
 The following `Pipeline` declares an input parameter called 'context', and uses
 it in the `PipelineTask`'s parameter. The `description` and `default` fields for

docs/resources.md

@@ -86,9 +86,6 @@ The `path` key is pre-defined and refers to the local path to a resource on the
 $(inputs.resouces.<name>.path)
 ```
 
-_The deprecated syntax `${}`, e.g. `${inputs.params.<name>}` will be supported
-until [#1170](https://github.com/tektoncd/pipeline/issues/1170)._
-
 ### Controlling where resources are mounted
 
 The optional field `targetPath` can be used to initialize a resource in specific

docs/tasks.md

@@ -400,9 +400,6 @@ $(inputs.params.<name>)
 
 Param values from resources can also be accessed using [variable substitution](./resources.md#variable-substitution)
 
-_The deprecated syntax `${}`, e.g. `${inputs.params.<name>}` will be supported
-until [#1170](https://github.com/tektoncd/pipeline/issues/1170)._
-
 #### Variable Substitution with Parameters of Type `Array`
 
 Referenced parameters of type `array` will expand to insert the array elements in the reference string's spot.

pkg/apis/pipeline/v1alpha1/param_types_test.go

@@ -128,42 +128,6 @@ func TestArrayOrString_ApplyReplacements(t *testing.T) {
 			arrayReplacements:  map[string][]string{"arraykey": {}},
 		},
 		expectedOutput: builder.ArrayOrString("firstvalue", "lastvalue"),
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated string replacements on string",
-		args: args{
-			input:              builder.ArrayOrString("astring${some} asdf ${anotherkey}"),
-			stringReplacements: map[string]string{"some": "value", "anotherkey": "value"},
-			arrayReplacements:  map[string][]string{"arraykey": {"array", "value"}, "sdfdf": {"asdf", "sdfsd"}},
-		},
-		expectedOutput: builder.ArrayOrString("astringvalue asdf value"),
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated single array replacement",
-		args: args{
-			input:              builder.ArrayOrString("firstvalue", "${arraykey}", "lastvalue"),
-			stringReplacements: map[string]string{"some": "value", "anotherkey": "value"},
-			arrayReplacements:  map[string][]string{"arraykey": {"array", "value"}, "sdfdf": {"asdf", "sdfsd"}},
-		},
-		expectedOutput: builder.ArrayOrString("firstvalue", "array", "value", "lastvalue"),
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated multiple array replacement",
-		args: args{
-			input:              builder.ArrayOrString("firstvalue", "${arraykey}", "lastvalue", "${sdfdf}"),
-			stringReplacements: map[string]string{"some": "value", "anotherkey": "value"},
-			arrayReplacements:  map[string][]string{"arraykey": {"array", "value"}, "sdfdf": {"asdf", "sdfsd"}},
-		},
-		expectedOutput: builder.ArrayOrString("firstvalue", "array", "value", "lastvalue", "asdf", "sdfsd"),
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated empty array replacement",
-		args: args{
-			input:              builder.ArrayOrString("firstvalue", "${arraykey}", "lastvalue"),
-			stringReplacements: map[string]string{"some": "value", "anotherkey": "value"},
-			arrayReplacements:  map[string][]string{"arraykey": {}},
-		},
-		expectedOutput: builder.ArrayOrString("firstvalue", "lastvalue"),
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go

@@ -152,35 +152,6 @@ func TestPipelineSpec_Validate(t *testing.T) {
 				tb.PipelineTaskParam("a-param", "$(input.workspace.$(baz))")),
 		)),
 		failureExpected: false,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated valid parameter variables",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeString),
-			tb.PipelineParamSpec("foo-is-baz", v1alpha1.ParamTypeString),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "${baz} and ${foo-is-baz}")),
-		)),
-		failureExpected: false,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated valid array parameter variables",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeArray, tb.ParamSpecDefault("some", "default")),
-			tb.PipelineParamSpec("foo-is-baz", v1alpha1.ParamTypeArray),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "${baz}", "and", "${foo-is-baz}")),
-		)),
-		failureExpected: false,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated pipeline parameter nested in task parameter",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeString),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "${input.workspace.${baz}}")),
-		)),
-		failureExpected: false,
 	}, {
 		name: "duplicate tasks",
 		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
@@ -305,67 +276,6 @@ func TestPipelineSpec_Validate(t *testing.T) {
 				tb.PipelineTaskParam("a-param", "first", "value: $(params.baz)", "last")),
 		)),
 		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated not defined parameter variable",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineTask("foo", "foo-task",
-				tb.PipelineTaskParam("a-param", "${params.does-not-exist}")))),
-		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "not defined parameter variable with defined",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("foo", v1alpha1.ParamTypeString, tb.ParamSpecDefault("something")),
-			tb.PipelineTask("foo", "foo-task",
-				tb.PipelineTaskParam("a-param", "${params.foo} and ${params.does-not-exist}")))),
-		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated invalid parameter type",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", "invalidtype", tb.ParamSpecDefault("some", "default")),
-			tb.PipelineParamSpec("foo-is-baz", v1alpha1.ParamTypeArray),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "${baz}", "and", "${foo-is-baz}")),
-		)),
-		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated array parameter mismatching default type",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeArray, tb.ParamSpecDefault("astring")),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "arrayelement", "${baz}")),
-		)),
-		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated string parameter mismatching default type",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeString, tb.ParamSpecDefault("anarray", "elements")),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "arrayelement", "${baz}")),
-		)),
-		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated array parameter used as string",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeArray, tb.ParamSpecDefault("anarray", "elements")),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "${params.baz}")),
-		)),
-		failureExpected: true,
-	}, {
-		// TODO(#1170): Remove support for ${} syntax
-		name: "deprecated array parameter string template not isolated",
-		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineParamSpec("baz", v1alpha1.ParamTypeArray, tb.ParamSpecDefault("anarray", "elements")),
-			tb.PipelineTask("bar", "bar-task",
-				tb.PipelineTaskParam("a-param", "first", "value: ${params.baz}", "last")),
-		)),
-		failureExpected: true,
 	}, {
 		name: "invalid dependency graph between the tasks",
 		p: tb.Pipeline("foo", "namespace", tb.PipelineSpec(

pkg/apis/pipeline/v1alpha1/step_replacements_test.go

@@ -34,45 +34,45 @@ func TestApplyStepReplacements(t *testing.T) {
 	}
 
 	s := v1alpha1.Step{Container: corev1.Container{
-		Name:       "${replace.me}",
-		Image:      "${replace.me}",
-		Command:    []string{"${array.replace.me}"},
-		Args:       []string{"${array.replace.me}"},
-		WorkingDir: "${replace.me}",
+		Name:       "$(replace.me)",
+		Image:      "$(replace.me)",
+		Command:    []string{"$(array.replace.me)"},
+		Args:       []string{"$(array.replace.me)"},
+		WorkingDir: "$(replace.me)",
 		EnvFrom: []corev1.EnvFromSource{{
 			ConfigMapRef: &corev1.ConfigMapEnvSource{
 				LocalObjectReference: corev1.LocalObjectReference{
-					Name: "${replace.me}",
+					Name: "$(replace.me)",
 				},
 			},
 			SecretRef: &corev1.SecretEnvSource{
 				LocalObjectReference: corev1.LocalObjectReference{
-					Name: "${replace.me}",
+					Name: "$(replace.me)",
 				},
 			},
 		}},
 		Env: []corev1.EnvVar{{
 			Name:  "not_me",
-			Value: "${replace.me}",
+			Value: "$(replace.me)",
 			ValueFrom: &corev1.EnvVarSource{
 				ConfigMapKeyRef: &corev1.ConfigMapKeySelector{
 					LocalObjectReference: corev1.LocalObjectReference{
-						Name: "${replace.me}",
+						Name: "$(replace.me)",
 					},
-					Key: "${replace.me}",
+					Key: "$(replace.me)",
 				},
 				SecretKeyRef: &corev1.SecretKeySelector{
 					LocalObjectReference: corev1.LocalObjectReference{
-						Name: "${replace.me}",
+						Name: "$(replace.me)",
 					},
-					Key: "${replace.me}",
+					Key: "$(replace.me)",
 				},
 			},
 		}},
 		VolumeMounts: []corev1.VolumeMount{{
-			Name:      "${replace.me}",
-			MountPath: "${replace.me}",
-			SubPath:   "${replace.me}",
+			Name:      "$(replace.me)",
+			MountPath: "$(replace.me)",
+			SubPath:   "$(replace.me)",
 		}},
 	}}
 
@@ -126,7 +126,7 @@ func TestApplyStepReplacements(t *testing.T) {
 
 func TestApplyStepReplacements_NotDefined(t *testing.T) {
 	s := v1alpha1.Step{Container: corev1.Container{
-		Name: "${params.not.defined}",
+		Name: "$(params.not.defined)",
 	}}
 	replacements := map[string]string{
 		"replace.me": "replaced!",
@@ -137,7 +137,7 @@ func TestApplyStepReplacements_NotDefined(t *testing.T) {
 	}
 
 	expected := v1alpha1.Step{Container: corev1.Container{
-		Name: "${params.not.defined}",
+		Name: "$(params.not.defined)",
 	}}
 	v1alpha1.ApplyStepReplacements(&s, replacements, arrayReplacements)
 	if d := cmp.Diff(s, expected); d != "" {

pkg/apis/pipeline/v1alpha1/substitution.go

@@ -26,8 +26,7 @@ import (
 
 const parameterSubstitution = "[_a-zA-Z][_a-zA-Z0-9.-]*"
 
-//TODO(#1170): Regex matches both ${} and $(), we will need to remove ${} compatibility.
-const braceMatchingRegex = "(\\$({%s.(?P<deprecated>%s)}))|(\\$(\\(%s.(?P<var>%s)\\)))"
+const braceMatchingRegex = "(\\$(\\(%s.(?P<var>%s)\\)))"
 
 func ValidateVariable(name, value, prefix, contextPrefix, locationName, path string, vars map[string]struct{}) *apis.FieldError {
 	if vs, present := extractVariablesFromString(value, contextPrefix+prefix); present {
@@ -76,10 +75,10 @@ func ValidateVariableIsolated(name, value, prefix, contextPrefix, locationName,
 	return nil
 }
 
-// Extract a the first full string expressions found (e.g "${input.params.foo}"). Return
+// Extract a the first full string expressions found (e.g "$(input.params.foo)"). Return
 // "" and false if nothing is found.
 func extractExpressionFromString(s, prefix string) (string, bool) {
-	pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, prefix, parameterSubstitution)
+	pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution)
 	re := regexp.MustCompile(pattern)
 	match := re.FindStringSubmatch(s)
 	if match == nil {
@@ -89,7 +88,7 @@ func extractExpressionFromString(s, prefix string) (string, bool) {
 }
 
 func extractVariablesFromString(s, prefix string) ([]string, bool) {
-	pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, prefix, parameterSubstitution)
+	pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution)
 	re := regexp.MustCompile(pattern)
 	matches := re.FindAllStringSubmatch(s, -1)
 	if len(matches) == 0 {
@@ -101,14 +100,7 @@ func extractVariablesFromString(s, prefix string) ([]string, bool) {
 		// foo -> foo
 		// foo.bar -> foo
 		// foo.bar.baz -> foo
-		var v string
-		if groups["var"] != "" {
-			v = groups["var"]
-		} else {
-			//TODO(#1170): Regex matches both ${} and $(), we will need to remove ${} compatibility.
-			v = groups["deprecated"]
-		}
-		vars[i] = strings.SplitN(v, ".", 2)[0]
+		vars[i] = strings.SplitN(groups["var"], ".", 2)[0]
 	}
 	return vars, true
 }
@@ -124,9 +116,6 @@ func matchGroups(matches []string, pattern *regexp.Regexp) map[string]string {
 func ApplyReplacements(in string, replacements map[string]string) string {
 	for k, v := range replacements {
 		in = strings.Replace(in, fmt.Sprintf("$(%s)", k), v, -1)
-
-		//TODO(#1170): Delete the line below when we want to remove support for ${} variable interpolation:
-		in = strings.Replace(in, fmt.Sprintf("${%s}", k), v, -1)
 	}
 	return in
 }
@@ -143,12 +132,6 @@ func ApplyArrayReplacements(in string, stringReplacements map[string]string, arr
 		if (strings.Count(in, stringToReplace) == 1) && len(in) == len(stringToReplace) {
 			return v
 		}
-
-		//TODO(#1170): Delete the block below when we want to remove support for ${} variable interpolation:
-		deprecatedStringToReplace := fmt.Sprintf("${%s}", k)
-		if (strings.Count(in, deprecatedStringToReplace) == 1) && len(in) == len(deprecatedStringToReplace) {
-			return v
-		}
 	}
 
 	// Otherwise return a size-1 array containing the input string with standard stringReplacements applied.