From f06760b40d9a10c983623d126667a607f10926e0 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Thu, 29 Aug 2019 18:33:00 -0400 Subject: [PATCH] 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 --- pkg/apis/pipeline/v1alpha1/resource_paths.go | 37 +++++++++ .../pipeline/v1alpha1/resource_paths_test.go | 81 +++++++++++++++++++ .../resources/conditionresolution.go | 13 +-- .../resources/conditionresolution_test.go | 14 ++-- pkg/reconciler/taskrun/resources/apply.go | 12 +-- 5 files changed, 134 insertions(+), 23 deletions(-) create mode 100644 pkg/apis/pipeline/v1alpha1/resource_paths.go create mode 100644 pkg/apis/pipeline/v1alpha1/resource_paths_test.go diff --git a/pkg/apis/pipeline/v1alpha1/resource_paths.go b/pkg/apis/pipeline/v1alpha1/resource_paths.go new file mode 100644 index 00000000000..ddeef1c3567 --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/resource_paths.go @@ -0,0 +1,37 @@ +/* + Copyright 2019 The Tekton 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 v1alpha1 + +import "path/filepath" + +// InputResourcePath returns the path where the given input resource +// will get mounted in a Pod +func InputResourcePath(r ResourceDeclaration) string { + return path("/workspace", r) +} + +// OutputResourcePath returns the path to the output resouce in a Pod +func OutputResourcePath(r ResourceDeclaration) string { + return path("/workspace/output", r) +} + +func path(root string, r ResourceDeclaration) string { + if r.TargetPath != "" { + return filepath.Join("/workspace", r.TargetPath) + } + return filepath.Join(root, r.Name) +} diff --git a/pkg/apis/pipeline/v1alpha1/resource_paths_test.go b/pkg/apis/pipeline/v1alpha1/resource_paths_test.go new file mode 100644 index 00000000000..0fa0a0112a4 --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/resource_paths_test.go @@ -0,0 +1,81 @@ +/* + Copyright 2019 The Tekton 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 v1alpha1_test + +import ( + "testing" + + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" +) + +func TestInputResourcePath(t *testing.T) { + tcs := []struct { + name string + resource v1alpha1.ResourceDeclaration + expected string + }{{ + name: "default_path", + resource: v1alpha1.ResourceDeclaration{ + Name: "foo", + }, + expected: "/workspace/foo", + }, { + name: "with target path", + resource: v1alpha1.ResourceDeclaration{ + Name: "foo", + TargetPath: "bar", + }, + expected: "/workspace/bar", + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + if actual := v1alpha1.InputResourcePath(tc.resource); actual != tc.expected { + t.Errorf("Unexpected input resource path: %s", actual) + } + }) + } +} + +func TestOutputResourcePath(t *testing.T) { + tcs := []struct { + name string + resource v1alpha1.ResourceDeclaration + expected string + }{{ + name: "default_path", + resource: v1alpha1.ResourceDeclaration{ + Name: "foo", + }, + expected: "/workspace/output/foo", + }, { + name: "with target path", + resource: v1alpha1.ResourceDeclaration{ + Name: "foo", + TargetPath: "bar", + }, + expected: "/workspace/bar", + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + if actual := v1alpha1.OutputResourcePath(tc.resource); actual != tc.expected { + t.Errorf("Unexpected output resource path: %s", actual) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/resources/conditionresolution.go b/pkg/reconciler/pipelinerun/resources/conditionresolution.go index 5b977fa1797..eeafee3a8ff 100644 --- a/pkg/reconciler/pipelinerun/resources/conditionresolution.go +++ b/pkg/reconciler/pipelinerun/resources/conditionresolution.go @@ -110,7 +110,7 @@ func (rcc *ResolvedConditionCheck) ConditionToTaskSpec() (*v1alpha1.TaskSpec, er // convert param strings of type ${params.x} to ${inputs.params.x} convertParamTemplates(&t.Steps[0], rcc.Condition.Spec.Params) // convert resource strings of type ${resources.name.key} to ${inputs.resources.name.key} - err := convertResourceTemplateStrings(&t.Steps[0], rcc.ResolvedResources, rcc.Condition.Spec.Resources) + err := ApplyResourceSubstitution(&t.Steps[0], rcc.ResolvedResources, rcc.Condition.Spec.Resources) if err != nil { return nil, xerrors.Errorf("Failed to replace resource template strings %w", err) @@ -130,9 +130,9 @@ func convertParamTemplates(step *v1alpha1.Step, params []v1alpha1.ParamSpec) { v1alpha1.ApplyStepReplacements(step, replacements, map[string][]string{}) } -// Prepends inputs. to all resource template strings so that they can be replaced by -// taskrun variable substitution e.g. ${resources.name.key} to ${inputs.resources.name.key} -func convertResourceTemplateStrings(step *v1alpha1.Step, resolvedResources map[string]*v1alpha1.PipelineResource, conditionResources []v1alpha1.ResourceDeclaration) error { +// ApplyResources applies the substitution from values in resources which are referenced +// in spec as subitems of the replacementStr. +func ApplyResourceSubstitution(step *v1alpha1.Step, resolvedResources map[string]*v1alpha1.PipelineResource, conditionResources []v1alpha1.ResourceDeclaration) error { replacements := make(map[string]string) for _, cr := range conditionResources { if rSpec, ok := resolvedResources[cr.Name]; ok { @@ -140,9 +140,10 @@ func convertResourceTemplateStrings(step *v1alpha1.Step, resolvedResources map[s if err != nil { return xerrors.Errorf("Error trying to create resource: %w", err) } - for replaceKeys := range r.Replacements() { - replacements[fmt.Sprintf("resources.%s.%s", cr.Name, replaceKeys)] = fmt.Sprintf("$(inputs.resources.%s.%s)", cr.Name, replaceKeys) + for k, v := range r.Replacements() { + replacements[fmt.Sprintf("resources.%s.%s", cr.Name, k)] = v } + replacements[fmt.Sprintf("resources.%s.path", cr.Name)] = v1alpha1.InputResourcePath(cr) } } v1alpha1.ApplyStepReplacements(step, replacements, map[string][]string{}) diff --git a/pkg/reconciler/pipelinerun/resources/conditionresolution_test.go b/pkg/reconciler/pipelinerun/resources/conditionresolution_test.go index 3826ac36c8f..997ba148a05 100644 --- a/pkg/reconciler/pipelinerun/resources/conditionresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/conditionresolution_test.go @@ -216,8 +216,8 @@ func TestResolvedConditionCheck_ConditionToTaskSpec(t *testing.T) { }, { name: "with-input-params", cond: tb.Condition("bar", "foo", tb.ConditionSpec( - tb.ConditionSpecCheck("${params.name}", "${params.img}", - tb.WorkingDir("${params.not.replaced}")), + tb.ConditionSpecCheck("$(params.name)", "$(params.img)", + tb.WorkingDir("$(params.not.replaced)")), tb.ConditionParamSpec("name", v1alpha1.ParamTypeString), tb.ConditionParamSpec("img", v1alpha1.ParamTypeString), )), @@ -232,16 +232,16 @@ func TestResolvedConditionCheck_ConditionToTaskSpec(t *testing.T) { }}, }, Steps: []v1alpha1.Step{{Container: corev1.Container{ - Name: "${inputs.params.name}", - Image: "${inputs.params.img}", - WorkingDir: "${params.not.replaced}", + Name: "$(inputs.params.name)", + Image: "$(inputs.params.img)", + WorkingDir: "$(params.not.replaced)", }}}, }, }, { name: "with-resources", cond: tb.Condition("bar", "foo", tb.ConditionSpec( tb.ConditionSpecCheck("name", "ubuntu", - tb.Args("${resources.git-resource.revision}")), + tb.Args("$(resources.git-resource.revision)")), tb.ConditionResource("git-resource", v1alpha1.PipelineResourceTypeGit), )), resolvedResources: map[string]*v1alpha1.PipelineResource{ @@ -261,7 +261,7 @@ func TestResolvedConditionCheck_ConditionToTaskSpec(t *testing.T) { Steps: []v1alpha1.Step{{Container: corev1.Container{ Name: "name", Image: "ubuntu", - Args: []string{"${inputs.resources.git-resource.revision}"}, + Args: []string{"master"}, }}}, }, }} diff --git a/pkg/reconciler/taskrun/resources/apply.go b/pkg/reconciler/taskrun/resources/apply.go index 456763eb3cc..730fb9c519d 100644 --- a/pkg/reconciler/taskrun/resources/apply.go +++ b/pkg/reconciler/taskrun/resources/apply.go @@ -18,7 +18,6 @@ package resources import ( "fmt" - "path/filepath" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" ) @@ -67,25 +66,18 @@ func ApplyResources(spec *v1alpha1.TaskSpec, resolvedResources map[string]v1alph // We always add replacements for 'path' if spec.Inputs != nil { for _, r := range spec.Inputs.Resources { - replacements[fmt.Sprintf("inputs.resources.%s.path", r.Name)] = path("/workspace", r) + replacements[fmt.Sprintf("inputs.resources.%s.path", r.Name)] = v1alpha1.InputResourcePath(r.ResourceDeclaration) } } if spec.Outputs != nil { for _, r := range spec.Outputs.Resources { - replacements[fmt.Sprintf("outputs.resources.%s.path", r.Name)] = path("/workspace/output", r) + replacements[fmt.Sprintf("outputs.resources.%s.path", r.Name)] = v1alpha1.OutputResourcePath(r.ResourceDeclaration) } } return ApplyReplacements(spec, replacements, map[string][]string{}) } -func path(root string, r v1alpha1.TaskResource) string { - if r.TargetPath != "" { - return filepath.Join("/workspace", r.TargetPath) - } - return filepath.Join(root, r.Name) -} - // ApplyReplacements replaces placeholders for declared parameters with the specified replacements. func ApplyReplacements(spec *v1alpha1.TaskSpec, stringReplacements map[string]string, arrayReplacements map[string][]string) *v1alpha1.TaskSpec { spec = spec.DeepCopy()