Add variable substitution for resource paths #877

dibyom · 2019-05-17T19:49:14Z

Changes

This PR adds the ability to substitute resource paths on disk using template variables e.g. ${inputs/outputs.resources.myresource.path}. Prior to this, users would have to hardcode the paths
(e.g. /workspace/myresource).

In order to add this feature, we had to do some refactoring in order to reuse PipelineResourceInterface s when creating pods. Each resource processing step (e.g. adding input/output resources,
applying templating) would create its own PipelineResourceInterface. This is a problem since the interface uses a setter(SetDestinationDirectory) that would not always be called. The destination directory is required to be set in order to facilitate variable substitution for resource paths.

The first commit refactors the resource initialization process by creating a binding between the named resource and the interface implementation and passing it around instead of creating new PipelineResourceInterfaceeach time.

Marking this as a WIP until I:

Add tests for output resources
Update the docs and examples

This PR is part of #626

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Task authors can use template variables to refer to using `path`:

${inputs.resources.someresource.path} 
${outputs.resources.someresource.path}

dibyom · 2019-05-17T19:50:42Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

@@ -752,38 +745,6 @@ func TestStorageInputResource(t *testing.T) {
 			},
 		},
 		wantErr: true,
-	}, {
-		desc: "inputs with both resource spec and resource ref",
-		task: &v1alpha1.Task{


These errors cases are no longer present here since the resource instantiation happens upstream in the calling code

bobcatfish

I started reviewing this and then realized it was a WIP - hope you don't mind some early feedback! :D

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

bobcatfish · 2019-05-21T13:38:00Z

pkg/apis/pipeline/v1alpha1/git_resource.go

-	if s.TargetPath != "" {
-		dPath = s.TargetPath
-	} else {
-		dPath = s.Name


hm, is it okay that we no longer default to s.Name? (i think maybe when we were pairing we found that s.Name was actually a bug? i can't quite remember 😅 )

I think we should be defaulting to the resource name (I'll validate!) but the destination directory (or TargetPath as it is called in the git resource) is set by the SetDestinationDirectory instead of in NewGitResource or GetDownloadContainerSpec.

SetDestinationDirectory itself is called in AddInputResources and the value of the path is set in destinationPath which defaults to the name.

This flow is confusing though. I think we should set the destinationDirectory when we create the object instead of the setter in a future PR

bobcatfish · 2019-05-21T13:39:21Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

+			i = inputResourceInterfaces[name]
+			resolved[r.Name] = i
+		} else if r.ResourceSpec != nil {
+			i, _ =v1alpha1.ResourceFromType(&v1alpha1.PipelineResource{


i think some of your auto-formatting settings might be a bit off here cuz I would expect to see a after the = 🤔

Ran gofmt!!

bobcatfish · 2019-05-21T13:40:00Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

@@ -74,22 +73,22 @@ func AddInputResource(
 	for _, input := range taskSpec.Inputs.Resources {
 		boundResource, err := getBoundResource(input.Name, taskRun.Spec.Inputs.Resources)
 		if err != nil {
-			return nil, fmt.Errorf("Failed to get bound resource: %s", err)
+			return nil, fmt.Errorf("failed to get bound resource: %s", err)


Each resource processing step (e.g. adding input/output resources, applying templating) would create its own `PipelineResourceInterface`. This is a problem since the interface uses a setter (`SetDestinationDirectory`) that would not always be called. The destination directory is required to be set in order to facilitate variable substitution for resource paths. This commit refactors the resource initialization process by creating a binding between the named resource and the interface implementation and passing it around instead of creating new `PipelineResourceInterface` each time.

This commits adds the ability to substitute resource paths on disk using template variables e.g. ${inputs/outputs.resources.myresource.path}. Prior to this, users would have to hardcode the paths (e.g. /workspace/myresource).

bobcatfish

Looks great!! I'm excited to have this functionality, thanks @dibyom ! ❤️

/lgtm
/approve

bobcatfish · 2019-05-29T18:26:28Z

docs/tasks.md

@@ -211,7 +211,8 @@ Input resources, like source code (git) or artifacts, are dumped at path
 `/workspace/task_resource_name` within a mounted
 [volume](https://kubernetes.io/docs/concepts/storage/volumes/) and is available
 to all [`steps`](#steps) of your `Task`. The path that the resources are mounted
-at can be overridden with the `targetPath` value.
+at can be overridden with the `targetPath` value. Steps can use the `path`
+ [template](#Templating) key to refer to the local path to the mounted resource.


bobcatfish · 2019-05-29T18:26:45Z

examples/taskruns/taskrun-targetpath-gotest.yaml

@@ -35,7 +35,7 @@ spec:
      - '-v'
      - '-count=1'
      - './...'
-      workingDir: "/workspace/go/src/github.com/tektoncd/pipeline"
+      workingDir: "${inputs.resources.gitspace.path}"


niiiice i like how you updated the examples too!

bobcatfish · 2019-05-29T18:27:24Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go


-	err := resources.AddOutputImageDigestExporter(tr, ts, c.resourceLister.PipelineResources(tr.Namespace).Get)
+// createPod creates a Pod based on the Task's configuration, with pvcName as a volumeMount
+// TODO(dibyom): Refactor resource setup/templating logic to its own function in the resources package


the only thing I'd say here is to only commit a TODO like this if you definitely mean to get back to it ;)

bobcatfish · 2019-05-29T18:28:50Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	outputResources, err := resourceImplBinding(rtr.Outputs)
+	if err != nil {
+		c.Logger.Errorf("Failed to initialize output resources: %v", err)
+		return nil, err


i think we usually lean toward a pattern where we wrap the error with more context and return it - and only in the reconciler loop where the return value of createPod is handled would we log

i'm happy to leave this as-is for now tho and we can improve it later :D (esp. if you are already planning to refactor 😇 )

tekton-robot · 2019-05-29T18:30:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bobcatfish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish · 2019-05-29T18:37:16Z

I0529 18:34:28.024] ERROR: (gcloud.beta.container.clusters.create) Operation [<Operation
I0529 18:34:28.025]  clusterConditions: [<StatusCondition
I0529 18:34:28.025]  code: CodeValueValuesEnum(GCE_STOCKOUT, 1)
I0529 18:34:28.025]  message: u'Try a different location, or try again later: Google Compute Engine does not have enough resources available to fulfill request: us-central1-c.'>]
I0529 18:34:28.025]  detail: u'Try a different location, or try again later: Google Compute Engine does not have enough resources available to fulfill request: us-central1-c.'

>.>
<.<

/test pull-tekton-pipeline-integration-tests

dibyom · 2019-05-29T19:30:10Z

Missed the memo on squashing up the commits 😞 Next time!

bobcatfish · 2019-05-29T20:20:21Z

Missed the memo on squashing up the commits disappointed Next time!

whoops, and i forgot to look! next time :)

@dibyom

@dibyom has been contributing significant work including variable path resource substitution tektoncd#877 and his continued work on one of our oldest and most sought after features: conditional execution (tektoncd#27)! He has also reviewed 44+ PRs in the Pipelines repo: https://tekton.devstats.cd.foundation/d/46/pr-reviews-by-contributor?orgId=1&var-period=d&var-repo_name=tektoncd%2Fpipeline&var-reviewers="dibyom"

@dibyom

@dibyom has been contributing significant work including variable path resource substitution #877 and his continued work on one of our oldest and most sought after features: conditional execution (#27)! He has also reviewed 44+ PRs in the Pipelines repo: https://tekton.devstats.cd.foundation/d/46/pr-reviews-by-contributor?orgId=1&var-period=d&var-repo_name=tektoncd%2Fpipeline&var-reviewers="dibyom"

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2019

tekton-robot requested review from dlorenc and imjasonh May 17, 2019 19:49

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label May 17, 2019

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 17, 2019

dibyom commented May 17, 2019

View reviewed changes

dibyom force-pushed the 626 branch 2 times, most recently from 8e3f4e7 to 521dd59 Compare May 20, 2019 18:10

bobcatfish reviewed May 21, 2019

View reviewed changes

dibyom force-pushed the 626 branch from 521dd59 to 0727513 Compare May 21, 2019 21:15

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2019

dibyom added 5 commits May 21, 2019 17:32

Add variable substitution for resource paths

925e09c

This commits adds the ability to substitute resource paths on disk using template variables e.g. ${inputs/outputs.resources.myresource.path}. Prior to this, users would have to hardcode the paths (e.g. /workspace/myresource).

Fix build test/lint errors

071bf6a

Review: gofmt + remove outdated comment

22a0216

Add a test for output resource path templating

cde3a30

dibyom force-pushed the 626 branch from 0727513 to cde3a30 Compare May 21, 2019 21:33

Update examples and docs to use path templates

7c251b7

dibyom changed the title ~~WIP Add variable substitution for resource paths~~ Add variable substitution for resource paths May 22, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 22, 2019

vdemeester added this to the Pipelines 0.5 🐱 : <cat name tbd @dlorenc> milestone May 28, 2019

bobcatfish reviewed May 29, 2019

View reviewed changes

tekton-robot assigned bobcatfish May 29, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2019

tekton-robot merged commit 4aac2b6 into tektoncd:master May 29, 2019

dibyom deleted the 626 branch May 29, 2019 19:30

bobcatfish mentioned this pull request Jun 10, 2019

Access paths to input/output resources via templating #626

Closed

bobcatfish mentioned this pull request Jul 30, 2019

Add @dibyom as an OWNER! 🙌 #1138

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variable substitution for resource paths #877

Add variable substitution for resource paths #877

dibyom commented May 17, 2019 •

edited

Loading

dibyom May 17, 2019

bobcatfish left a comment

bobcatfish May 21, 2019

dibyom May 21, 2019 •

edited

Loading

bobcatfish May 21, 2019

dibyom May 21, 2019

bobcatfish May 21, 2019

bobcatfish left a comment

bobcatfish May 29, 2019

bobcatfish May 29, 2019

bobcatfish May 29, 2019

bobcatfish May 29, 2019

bobcatfish May 29, 2019

tekton-robot commented May 29, 2019

bobcatfish commented May 29, 2019 •

edited

Loading

dibyom commented May 29, 2019

bobcatfish commented May 29, 2019

Add variable substitution for resource paths #877

Add variable substitution for resource paths #877

Conversation

dibyom commented May 17, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 29, 2019

bobcatfish commented May 29, 2019 • edited Loading

dibyom commented May 29, 2019

bobcatfish commented May 29, 2019

dibyom commented May 17, 2019 •

edited

Loading

dibyom May 21, 2019 •

edited

Loading

bobcatfish commented May 29, 2019 •

edited

Loading