Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change the behavior of outputs that are also used as inputs. #1122

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Change the behavior of outputs that are also used as inputs. #1122

merged 2 commits into from
Sep 13, 2019

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented Jul 24, 2019 •
edited by bobcatfish
Loading

Changes

This change makes the handling of Resources within a Task consistent, regardless of
whether the same Resource is used as both an input and an output. Previously these
were special cased, which made it hard to write Tasks consistently.

This is a followup to #1119 and should be submitted
once the next release is cut.

Fixes #1118

Submitter Checklist

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

Release Notes

Tasks that take input and output resources of the same type must now copy or move the resource from the input directory to the output directory manually.

Tekton no longer automatically reads outputs from the input directory when the same resource is supplied in both places.

@tekton-robot tekton-robot requested review from abayer and imjasonh July 24, 2019 16:20
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 24, 2019
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2019
@dlorenc
Copy link
Contributor Author

dlorenc commented Jul 24, 2019

/hold this should be submitted after the next release.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 89.9% 88.9% -1.0

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 89.9% 88.9% -1.0

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 89.9% 88.9% -1.0

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 89.9% 88.9% -1.0

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 89.9% 88.9% -1.0

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 89.9% 88.9% -1.0

@dlorenc
Copy link
Contributor Author

dlorenc commented Jul 30, 2019

/test pull-tekton-pipeline-integration-tests

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.8% 90.7% -1.1

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.8% 90.7% -1.1

@dlorenc
Copy link
Contributor Author

dlorenc commented Jul 31, 2019

/test pull-tekton-pipeline-integration-tests

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.8% 90.7% -1.1

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.8% 90.7% -1.1

afrittoli
afrittoli reviewed Aug 1, 2019
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this, I think it's an important change in the right direction!
I have some concerns though - it's probably ok to address them in a separate PR as long as keep it within the same release.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.8% 90.7% -1.1
pkg/reconciler/v1alpha1/taskrun/taskrun.go Do not exist 71.0%

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.7% 90.5% -1.2

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 91.7% 90.5% -1.2

@dlorenc dlorenc mentioned this pull request Aug 19, 2019
Closed
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/resources/output_resource.go 91.7% 90.5% -1.2

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/resources/output_resource.go 91.7% 90.5% -1.2

@bobcatfish
Copy link
Collaborator

Since we warned about this in 0.6 (#1119) I think we can merge this for 0.7.

I'll take on fixing the integration test.

@bobcatfish bobcatfish self-assigned this Sep 5, 2019
@dlorenc @bobcatfish
Change the behavior of outputs that are also used as inputs.
e1ccf54 
This change makes the handling of Resources within a Task consistent, regardless of
whether the same Resource is used as both an input and an output. Previously these
were special cased, which made it hard to write Tasks consistently.

This commit also makes a few minor changes to the way our bash output gets logged.
I discovered this was missing during debugging, and made it consistent with the gsutil
wrapper.

This is a followup to tektoncd#1119 and should be submitted
once the next release is cut.
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/resources/output_resource.go 91.7% 90.5% -1.2

@bobcatfish
Update fan in / fan out test (no automatic copy) 📋
7adc586 
Now that we don't automatically copy the content of an input to an
output (when the same resource is used as both an input and an output),
this means that:
- Our fan-in / fan-out test will need to explicitly write to the output
  path, instead of writing to the input path and assuming it would get
  copied over (the very behaviour we're changing in tektoncd#1188)
- Data previously written to an output that is used as an input, and
  then an output later on, will be lost unless explicitly copied. In the
  update to the examples this was handled by symlinking the input to the
  output, I decided to instead update the test to no longer expect to
  see a file that was written by the first task in the graph and to not
  copy it explicitly.

Note that there is actually a race between the two tasks fanning out -
if the were writing the same file we would not be able to reliably
predict which would win.

Part of fixing tektoncd#1188
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/resources/output_resource.go 91.7% 90.5% -1.2

@bobcatfish
Copy link
Collaborator

Ready for another look @afrittoli ! And now that we've had the 0.6 release we should be able to merge this :D I also fixed the failing integration test.

afrittoli
afrittoli approved these changes Sep 13, 2019
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm Nice work, thank you!

vdemeester
vdemeester approved these changes Sep 13, 2019
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dlorenc, vdemeester

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 [afrittoli,dlorenc,vdemeester]

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

@tekton-robot tekton-robot merged commit 5e3276d into tektoncd:master Sep 13, 2019
@afrittoli afrittoli mentioned this pull request Sep 17, 2019
Closed
afrittoli added a commit to afrittoli/pipeline that referenced this pull request Sep 17, 2019
@afrittoli
Fix release pipeline to handle tektoncd#1122
64393fa 
Since tektoncd#1122, we do not treat outputs that were inputs as well in any
special way, meaning that the output folder will be empty unless we
copy the input to the output.

Fix the release pipeline to handle that.

Fixes: tektoncd#1325
@afrittoli afrittoli mentioned this pull request Sep 17, 2019
Merged
3 tasks
tekton-robot pushed a commit that referenced this pull request Sep 18, 2019
@afrittoli @tekton-robot
Fix release pipeline to handle #1122
05c4164 
Since #1122, we do not treat outputs that were inputs as well in any
special way, meaning that the output folder will be empty unless we
copy the input to the output.

Fix the release pipeline to handle that.

Fixes: #1325
afrittoli added a commit to afrittoli/pipeline that referenced this pull request Sep 18, 2019
@afrittoli
Fix release pipeline to handle tektoncd#1122
f89d1ac 
Since tektoncd#1122, we do not treat outputs that were inputs as well in any
special way, meaning that the output folder will be empty unless we
copy the input to the output.

Fix the release pipeline to handle that.
tekton-robot pushed a commit that referenced this pull request Sep 19, 2019
@afrittoli @tekton-robot
Fix release pipeline to handle #1122
8696190 
Since #1122, we do not treat outputs that were inputs as well in any
special way, meaning that the output folder will be empty unless we
copy the input to the output.

Fix the release pipeline to handle that.
@bobcatfish bobcatfish mentioned this pull request Dec 19, 2019
Closed
@shimmerjs shimmerjs mentioned this pull request Mar 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@afrittoli afrittoli afrittoli approved these changes

@abayer abayer Awaiting requested review from abayer

@imjasonh imjasonh Awaiting requested review from imjasonh

Assignees

@vdemeester vdemeester

@bobcatfish bobcatfish

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. next-release For PRs that shouldn't be merged until the next release (backward incompatible) size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not automatically copy inputs used also as outputs
6 participants
@dlorenc @tekton-robot @bobcatfish @vdemeester @afrittoli @googlebot