Only copy output resource to PVC for storage/git output #414

afrittoli · 2019-01-21T15:33:05Z

At the moment the output_resource module unconditionally adds a
copy step for every output resource in the taskrun.
However not every output resource will generate content to be
copied. Limit the copy to Storage and Git outoputs for now.

This is meant as a hotfix for bug #401 until an implementation
of the image output resource is available via #216. Until
image resource output is implemented, any consumer won't
have guarantee that the image has been actually built and pushed,
and won't know the hash of the created image.

shashwathi

Thank you so much for adding this PR. Can you add test in output resources package that image outputs does not create output related steps?

afrittoli · 2019-01-21T17:21:23Z

Thank you so much for adding this PR. Can you add test in output resources package that image outputs does not create output related steps?

Sure I can do that. I didn't add tests so far because - I think - this is going away once image output resource is fully implemented, but it's better to have the test and remove it :)

shashwathi · 2019-01-21T17:21:32Z

/assign @shashwathi

shashwathi · 2019-01-21T17:23:33Z

this is going away once image output resource is fully implemented, but it's better to have the test and remove it

Yes I agree with that too

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

At the moment the output_resource module unconditionally adds a copy step for every output resource in the taskrun. However not every output resource will generate content to be copied. Limit the copy to Storage and Git outoputs for now. This is meant as a hotfix for bug tektoncd#401 until an implementation of the image output resource is available via tektoncd#216. Until image resource output is implemented, any consumer won't have guarantee that the image has been actually built and pushed, and won't know the hash of the created image.

shashwathi · 2019-01-22T16:58:20Z

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

+
+// allowedOutputResource checks if an output resource type produces
+// an output that should be copied to the PVC
+func allowedOutputResource(resourceType v1alpha1.PipelineResourceType) bool {


allowedOutputResources is not going to change so makes sense to define it in variable section. Something like

var ( // allowedOutputResource checks if an output resource type produces // an output that should be copied to the PVC allowedOutputResources = map[v1alpha1.PipelineResourceType]bool{ v1alpha1.PipelineResourceTypeStorage: true, v1alpha1.PipelineResourceTypeGit: true, } )

and update the if condition to be

if allowedOutputResources[resource.Spec.Type] && pipelineRunpvcName != "" { }

Code is simple and does not need a function. WDYT?

I made a function because you asked for it to begin with :)

I'm not sure that list is not going to change... other resources will have output to be copied, like logs of the build process for the image resource.
I hope eventually that list will go away for good and it will be replaced by a proper interface on the resource to declare whether they have output to be copied or not.

The advantage of having a function is also that it's possible to write a simple unit test for it, but it's true that there's little value in that unit test, so I'm ok with dropping the function if you prefer so.

I made a function because you asked for it to begin with :)

Sorry for the confusion. My intention was to make the if condition simpler and easier to read so I jumped at function as solution. I should have left up to you come up with simpler solution instead of influencing my solution on you. After I saw your solution it felt declaring map in variables section seemed a lot simpler than function

The advantage of having a function is also that it's possible to write a simple unit test for it, but it's true that there's little value in that unit test, so I'm ok with dropping the function if you prefer so.

You can still have the tests. I think it holds value in showing supported output types explicitly. I like the tests. I have suggested below how to update tests to use map instead of function.

OK, addressed everything - hopefully - in the version :)

I hope eventually that list will go away for good and it will be replaced by a proper interface on the resource to declare whether they have output to be copied or not.

yuusssss

shashwathi · 2019-01-22T17:06:10Z

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

+ expectedAllowed: false,
+ }} {
+ t.Run(c.desc, func(t *testing.T) {
+ actual := allowedOutputResource(c.resourceType)


Test assertion can be combined into single like

if c.expectedAllowed != allowedOutputResources[c.resourceType] { t.Fatalf("Test AllowedOutputResource %s expected %t but got %t", c.desc, c.expectedAllowed, allowedOutputResources[c.resourceType])}

If test error contains the expected result and current result then debugging test failures is much easier.

Good point, thanks

Add a map that defines which output resource produces an output that should be copied to the PVC. Add unit tests for it too.

bobcatfish · 2019-01-22T18:23:07Z

Seems like a reasonable approach to me! I'll leave it to @shashwathi for the final sign off tho :D

shashwathi

/ok-to-test
/lgtm

knative-prow-robot · 2019-01-22T18:55:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, shashwathi

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~~ [shashwathi]

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

shashwathi · 2019-01-22T18:56:05Z

/ok-to-test

shashwathi · 2019-01-22T18:56:37Z

@afrittoli Thank you so much for your patience on addressing all my comments. 👍 I appreciate that.

knative-metrics-robot · 2019-01-22T18:57:30Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go	93.1%	91.5%	-1.6

Fix license

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 21, 2019

knative-prow-robot requested review from aaron-prindle and dlorenc January 21, 2019 15:33

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 21, 2019

afrittoli changed the title ~~Onpy copy output resource to PVC for storage/git output~~ Only copy output resource to PVC for storage/git output Jan 21, 2019

afrittoli mentioned this pull request Jan 21, 2019

Bug in example for build-pipeline: cp -r /workspace/output/builtImage/ fails #401

Closed

shashwathi reviewed Jan 21, 2019

View reviewed changes

knative-prow-robot assigned shashwathi Jan 21, 2019

shashwathi reviewed Jan 21, 2019

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go Outdated Show resolved Hide resolved

afrittoli force-pushed the bug/401 branch from 7412699 to 8ddbc96 Compare January 21, 2019 18:01

shashwathi reviewed Jan 22, 2019

View reviewed changes

Move the allowed output resource to a map

b116f07

Add a map that defines which output resource produces an output that should be copied to the PVC. Add unit tests for it too.

afrittoli force-pushed the bug/401 branch from eed0b96 to b116f07 Compare January 22, 2019 17:19

bobcatfish mentioned this pull request Jan 22, 2019

Design PipelineResource extensibility #238

Closed

shashwathi approved these changes Jan 22, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2019

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 22, 2019

knative-prow-robot merged commit 02d1ab7 into tektoncd:master Jan 22, 2019

a-roberts mentioned this pull request Jan 22, 2019

Document running the pipeline tutorial on one's laptop #410

Closed

a-roberts mentioned this pull request Jan 23, 2019

Help users who want to develop locally #425

Merged

afrittoli mentioned this pull request Jan 24, 2019

Only copy output of resources when possible #428

Merged

mchmarny unassigned shashwathi Mar 7, 2019

piyush-garg pushed a commit to piyush-garg/pipeline that referenced this pull request May 29, 2020

Merge pull request tektoncd#414 from piyush-garg/fix_license

ac4537f

Fix license

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only copy output resource to PVC for storage/git output #414

Only copy output resource to PVC for storage/git output #414

afrittoli commented Jan 21, 2019

shashwathi left a comment

afrittoli commented Jan 21, 2019

shashwathi commented Jan 21, 2019

shashwathi commented Jan 21, 2019

shashwathi Jan 22, 2019

afrittoli Jan 22, 2019

shashwathi Jan 22, 2019

shashwathi Jan 22, 2019

afrittoli Jan 22, 2019

bobcatfish Jan 22, 2019

shashwathi Jan 22, 2019

shashwathi Jan 22, 2019

afrittoli Jan 22, 2019

bobcatfish commented Jan 22, 2019

shashwathi left a comment

knative-prow-robot commented Jan 22, 2019

shashwathi commented Jan 22, 2019

shashwathi commented Jan 22, 2019

knative-metrics-robot commented Jan 22, 2019

Only copy output resource to PVC for storage/git output #414

Only copy output resource to PVC for storage/git output #414

Conversation

afrittoli commented Jan 21, 2019

shashwathi left a comment

Choose a reason for hiding this comment

afrittoli commented Jan 21, 2019

shashwathi commented Jan 21, 2019

shashwathi commented Jan 21, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Jan 22, 2019

shashwathi left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 22, 2019

shashwathi commented Jan 22, 2019

shashwathi commented Jan 22, 2019

knative-metrics-robot commented Jan 22, 2019