Skip to content

Commit 2492702

Browse files
dibyomdibyom
committed
Explicitly substitute resource conditional variables.
Previously, we'd change the variable names to what the taskrun format and then rely on the taskrun's variable substitution logic. This approach is hacky and now we explicitly replace the resource variable strings with the right values without relying on the TaskRun logic. In the future, we should consider doing the same for parameter variables as well. Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
1 parent 5ff2ed5 commit 2492702

File tree

5 files changed

+134
-23
lines changed

5 files changed

+134
-23
lines changed

‎pkg/apis/pipeline/v1alpha1/resource_paths.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
Copyright 2019 The Tekton Authors
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package v1alpha1
18+
19+
import "path/filepath"
20+
21+
// InputResourcePath returns the path where the given input resource
22+
// will get mounted in a Pod
23+
func InputResourcePath(r ResourceDeclaration) string {
24+
return path("/workspace", r)
25+
}
26+
27+
// OutputResourcePath returns the path to the output resouce in a Pod
28+
func OutputResourcePath(r ResourceDeclaration) string {
29+
return path("/workspace/output", r)
30+
}
31+
32+
func path(root string, r ResourceDeclaration) string {
33+
if r.TargetPath != "" {
34+
return filepath.Join("/workspace", r.TargetPath)
35+
}
36+
return filepath.Join(root, r.Name)
37+
}

‎pkg/apis/pipeline/v1alpha1/resource_paths_test.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/*
2+
Copyright 2019 The Tekton Authors
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package v1alpha1_test
18+
19+
import (
20+
"testing"
21+
22+
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
23+
)
24+
25+
func TestInputResourcePath(t *testing.T) {
26+
tcs := []struct {
27+
name string
28+
resource v1alpha1.ResourceDeclaration
29+
expected string
30+
}{{
31+
name: "default_path",
32+
resource: v1alpha1.ResourceDeclaration{
33+
Name: "foo",
34+
},
35+
expected: "/workspace/foo",
36+
}, {
37+
name: "with target path",
38+
resource: v1alpha1.ResourceDeclaration{
39+
Name: "foo",
40+
TargetPath: "bar",
41+
},
42+
expected: "/workspace/bar",
43+
}}
44+
45+
for _, tc := range tcs {
46+
t.Run(tc.name, func(t *testing.T) {
47+
if actual := v1alpha1.InputResourcePath(tc.resource); actual != tc.expected {
48+
t.Errorf("Unexpected input resource path: %s", actual)
49+
}
50+
})
51+
}
52+
}
53+
54+
func TestOutputResourcePath(t *testing.T) {
55+
tcs := []struct {
56+
name string
57+
resource v1alpha1.ResourceDeclaration
58+
expected string
59+
}{{
60+
name: "default_path",
61+
resource: v1alpha1.ResourceDeclaration{
62+
Name: "foo",
63+
},
64+
expected: "/workspace/output/foo",
65+
}, {
66+
name: "with target path",
67+
resource: v1alpha1.ResourceDeclaration{
68+
Name: "foo",
69+
TargetPath: "bar",
70+
},
71+
expected: "/workspace/bar",
72+
}}
73+
74+
for _, tc := range tcs {
75+
t.Run(tc.name, func(t *testing.T) {
76+
if actual := v1alpha1.OutputResourcePath(tc.resource); actual != tc.expected {
77+
t.Errorf("Unexpected output resource path: %s", actual)
78+
}
79+
})
80+
}
81+
}

‎pkg/reconciler/pipelinerun/resources/conditionresolution.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func (rcc *ResolvedConditionCheck) ConditionToTaskSpec() (*v1alpha1.TaskSpec, er
110110
// convert param strings of type ${params.x} to ${inputs.params.x}
111111
convertParamTemplates(&t.Steps[0], rcc.Condition.Spec.Params)
112112
// convert resource strings of type ${resources.name.key} to ${inputs.resources.name.key}
113-
err := convertResourceTemplateStrings(&t.Steps[0], rcc.ResolvedResources, rcc.Condition.Spec.Resources)
113+
err := ApplyResourceSubstitution(&t.Steps[0], rcc.ResolvedResources, rcc.Condition.Spec.Resources)
114114

115115
if err != nil {
116116
return nil, xerrors.Errorf("Failed to replace resource template strings %w", err)
@@ -130,19 +130,20 @@ func convertParamTemplates(step *v1alpha1.Step, params []v1alpha1.ParamSpec) {
130130
v1alpha1.ApplyStepReplacements(step, replacements, map[string][]string{})
131131
}
132132

133-
// Prepends inputs. to all resource template strings so that they can be replaced by
134-
// taskrun variable substitution e.g. ${resources.name.key} to ${inputs.resources.name.key}
135-
func convertResourceTemplateStrings(step *v1alpha1.Step, resolvedResources map[string]*v1alpha1.PipelineResource, conditionResources []v1alpha1.ResourceDeclaration) error {
133+
// ApplyResources applies the substitution from values in resources which are referenced
134+
// in spec as subitems of the replacementStr.
135+
func ApplyResourceSubstitution(step *v1alpha1.Step, resolvedResources map[string]*v1alpha1.PipelineResource, conditionResources []v1alpha1.ResourceDeclaration) error {
136136
replacements := make(map[string]string)
137137
for _, cr := range conditionResources {
138138
if rSpec, ok := resolvedResources[cr.Name]; ok {
139139
r, err := v1alpha1.ResourceFromType(rSpec)
140140
if err != nil {
141141
return xerrors.Errorf("Error trying to create resource: %w", err)
142142
}
143-
for replaceKeys := range r.Replacements() {
144-
replacements[fmt.Sprintf("resources.%s.%s", cr.Name, replaceKeys)] = fmt.Sprintf("$(inputs.resources.%s.%s)", cr.Name, replaceKeys)
143+
for k, v := range r.Replacements() {
144+
replacements[fmt.Sprintf("resources.%s.%s", cr.Name, k)] = v
145145
}
146+
replacements[fmt.Sprintf("resources.%s.path", cr.Name)] = v1alpha1.InputResourcePath(cr)
146147
}
147148
}
148149
v1alpha1.ApplyStepReplacements(step, replacements, map[string][]string{})

‎pkg/reconciler/pipelinerun/resources/conditionresolution_test.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -216,8 +216,8 @@ func TestResolvedConditionCheck_ConditionToTaskSpec(t *testing.T) {
216216
}, {
217217
name: "with-input-params",
218218
cond: tb.Condition("bar", "foo", tb.ConditionSpec(
219-
tb.ConditionSpecCheck("${params.name}", "${params.img}",
220-
tb.WorkingDir("${params.not.replaced}")),
219+
tb.ConditionSpecCheck("$(params.name)", "$(params.img)",
220+
tb.WorkingDir("$(params.not.replaced)")),
221221
tb.ConditionParamSpec("name", v1alpha1.ParamTypeString),
222222
tb.ConditionParamSpec("img", v1alpha1.ParamTypeString),
223223
)),
@@ -232,16 +232,16 @@ func TestResolvedConditionCheck_ConditionToTaskSpec(t *testing.T) {
232232
}},
233233
},
234234
Steps: []v1alpha1.Step{{Container: corev1.Container{
235-
Name: "${inputs.params.name}",
236-
Image: "${inputs.params.img}",
237-
WorkingDir: "${params.not.replaced}",
235+
Name: "$(inputs.params.name)",
236+
Image: "$(inputs.params.img)",
237+
WorkingDir: "$(params.not.replaced)",
238238
}}},
239239
},
240240
}, {
241241
name: "with-resources",
242242
cond: tb.Condition("bar", "foo", tb.ConditionSpec(
243243
tb.ConditionSpecCheck("name", "ubuntu",
244-
tb.Args("${resources.git-resource.revision}")),
244+
tb.Args("$(resources.git-resource.revision)")),
245245
tb.ConditionResource("git-resource", v1alpha1.PipelineResourceTypeGit),
246246
)),
247247
resolvedResources: map[string]*v1alpha1.PipelineResource{
@@ -261,7 +261,7 @@ func TestResolvedConditionCheck_ConditionToTaskSpec(t *testing.T) {
261261
Steps: []v1alpha1.Step{{Container: corev1.Container{
262262
Name: "name",
263263
Image: "ubuntu",
264-
Args: []string{"${inputs.resources.git-resource.revision}"},
264+
Args: []string{"master"},
265265
}}},
266266
},
267267
}}

‎pkg/reconciler/taskrun/resources/apply.go‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package resources
1818

1919
import (
2020
"fmt"
21-
"path/filepath"
2221

2322
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
2423
)
@@ -67,25 +66,18 @@ func ApplyResources(spec *v1alpha1.TaskSpec, resolvedResources map[string]v1alph
6766
// We always add replacements for 'path'
6867
if spec.Inputs != nil {
6968
for _, r := range spec.Inputs.Resources {
70-
replacements[fmt.Sprintf("inputs.resources.%s.path", r.Name)] = path("/workspace", r)
69+
replacements[fmt.Sprintf("inputs.resources.%s.path", r.Name)] = v1alpha1.InputResourcePath(r.ResourceDeclaration)
7170
}
7271
}
7372
if spec.Outputs != nil {
7473
for _, r := range spec.Outputs.Resources {
75-
replacements[fmt.Sprintf("outputs.resources.%s.path", r.Name)] = path("/workspace/output", r)
74+
replacements[fmt.Sprintf("outputs.resources.%s.path", r.Name)] = v1alpha1.OutputResourcePath(r.ResourceDeclaration)
7675
}
7776
}
7877

7978
return ApplyReplacements(spec, replacements, map[string][]string{})
8079
}
8180

82-
func path(root string, r v1alpha1.TaskResource) string {
83-
if r.TargetPath != "" {
84-
return filepath.Join("/workspace", r.TargetPath)
85-
}
86-
return filepath.Join(root, r.Name)
87-
}
88-
8981
// ApplyReplacements replaces placeholders for declared parameters with the specified replacements.
9082
func ApplyReplacements(spec *v1alpha1.TaskSpec, stringReplacements map[string]string, arrayReplacements map[string][]string) *v1alpha1.TaskSpec {
9183
spec = spec.DeepCopy()

0 commit comments

Comments
 (0)