Update TaskRunStatus.ResourcesResult to be more generic. #1406

dlorenc · 2019-10-09T18:19:22Z

Changes

This commit extends ResourcesResult to work for more than just ImageOutputs,
but moving to a key/value map instead of a fixed key of "digest".

This also updates the image_digest_exporter to write data in both formats
for backwards compatibility, and updates our e2e test to use this mechanism.

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.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

- The Name and Digest fields on TaskRunStatus.ResourcesResult are deprecated and are replaced by the new Key and ResourceRef fields.
- Both sets of fields are present in this release, but a future release will remove the legacy fields.

dlorenc · 2019-10-09T18:21:14Z

Ref: #1392

tekton-robot · 2019-10-09T18:21:45Z

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/image_exporter.go	87.8%	87.5%	-0.3

tekton-robot · 2019-10-09T18:22:14Z

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/image_exporter.go	87.8%	87.5%	-0.3

tekton-robot · 2019-10-09T18:38:45Z

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/image_exporter.go	87.8%	87.5%	-0.3

tekton-robot · 2019-10-09T19:09:15Z

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/image_exporter.go	87.8%	87.5%	-0.3

tekton-robot · 2019-10-09T19:45:15Z

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/image_exporter.go	87.8%	87.5%	-0.3

bobcatfish

Amaaaazing! My only request is for some updates but otherwise this is 💯

We'll also need to update the docs - this section will need to go and/or be updated https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#surfacing-the-image-digest-built-in-a-task and we should probably add:

something in resources.md about how this works
something about how these values are surfaced (maybe also in resources.md? or maybe in the developer docs) (audience = someone writing a new resource) <-- tho since this PR is just changing image digest, maybe this is something for later
something in the TaskRun status docs https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#status

test/kaniko_task_test.go

bobcatfish · 2019-10-10T11:39:43Z

test/pipelinerun_test.go

@@ -70,7 +70,7 @@ func TestPipelineRun(t *testing.T) {

 			for _, res := range getFanInFanOutGitResources(namespace) {
 				if _, err := c.PipelineResourceClient.Create(res); err != nil {
-					t.Fatalf("Failed to create Pipeline Resource `%s`: %s", kanikoResourceName, err)
+					t.Fatalf("Failed to create Pipeline Resource `%s`: %s", kanikoGitResourceName, err)


this seems like an odd change - i wonder if the wrong string was being used here previously?

pkg/apis/pipeline/v1alpha1/resource_types.go

dlorenc · 2019-10-10T21:04:42Z

OK, docs are updated and ready to go.

tekton-robot · 2019-10-10T22:29:34Z

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/image_exporter.go	87.8%	87.5%	-0.3

vdemeester

Looks good to me 👍

tekton-robot · 2019-10-11T08:49:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vdemeester]

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

This commit extends ResourcesResult to work for more than just ImageOutputs, but moving to a key/value map instead of a fixed key of "digest". This also updates the image_digest_exporter to write data in both formats for backwards compatibility, and updates our e2e test to use this mechanism.

tekton-robot · 2019-10-11T14:09:52Z

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/image_exporter.go	87.8%	87.5%	-0.3

dlorenc · 2019-10-11T14:32:26Z

Rebased!

imjasonh · 2019-10-11T15:13:05Z

/lgtm

This change builds upon tektoncd#1406, and logs the exact Git commit used by a Git input to the ResourceResults field. Some other cleanups are included: - Removing custom TerminationMessagePath from the Image resource. The default is fine. - Test cleanup. - A new helper to write termination messages from resource containers.

This change builds upon tektoncd#1406, and logs the exact Git commit used by a Git input to the ResourceResults field. Some other cleanups are included: - Removing custom TerminationMessagePath from the Image resource. The default is fine. - Test cleanup. - A new helper to write termination messages from resource containers. And finally, this adds a new environment variable to the git resource steps, indicating the name of the resource instance they are running as. We should make this more generic and apply it to all resource steps as part of the extensiblity refactor.

This change builds upon #1406, and logs the exact Git commit used by a Git input to the ResourceResults field. Some other cleanups are included: - Removing custom TerminationMessagePath from the Image resource. The default is fine. - Test cleanup. - A new helper to write termination messages from resource containers. And finally, this adds a new environment variable to the git resource steps, indicating the name of the resource instance they are running as. We should make this more generic and apply it to all resource steps as part of the extensiblity refactor.

tekton-robot requested review from abayer and bobcatfish October 9, 2019 18:19

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

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 9, 2019

dlorenc force-pushed the result branch from 38ad3cf to 1a933db Compare October 9, 2019 18:20

dlorenc force-pushed the result branch from 1a933db to 8092af6 Compare October 9, 2019 18:36

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 9, 2019

dlorenc force-pushed the result branch from 8092af6 to 56e2f1a Compare October 9, 2019 19:07

dlorenc force-pushed the result branch from 56e2f1a to c4f1127 Compare October 9, 2019 19:42

bobcatfish reviewed Oct 10, 2019

View reviewed changes

dlorenc force-pushed the result branch from c4f1127 to 02db4f6 Compare October 10, 2019 22:27

vdemeester approved these changes Oct 11, 2019

View reviewed changes

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

dlorenc force-pushed the result branch from 02db4f6 to cb6ad7e Compare October 11, 2019 14:07

tekton-robot assigned imjasonh Oct 11, 2019

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

tekton-robot merged commit e01fe60 into tektoncd:master Oct 11, 2019

dlorenc deleted the result branch October 11, 2019 15:34

dlorenc mentioned this pull request Oct 14, 2019

Refactor Resource result output, and add support for Git resources. #1424

Merged

3 tasks

bobcatfish mentioned this pull request Oct 23, 2019

Proposal: Make PipelineResourceResult generic #1392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TaskRunStatus.ResourcesResult to be more generic. #1406

Update TaskRunStatus.ResourcesResult to be more generic. #1406

dlorenc commented Oct 9, 2019

dlorenc commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

bobcatfish left a comment

bobcatfish Oct 10, 2019

dlorenc commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

vdemeester left a comment

tekton-robot commented Oct 11, 2019

tekton-robot commented Oct 11, 2019

dlorenc commented Oct 11, 2019

imjasonh commented Oct 11, 2019

Update TaskRunStatus.ResourcesResult to be more generic. #1406

Update TaskRunStatus.ResourcesResult to be more generic. #1406

Conversation

dlorenc commented Oct 9, 2019

Changes

Submitter Checklist

Reviewer Notes

Release Notes

dlorenc commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Oct 10, 2019

Choose a reason for hiding this comment

dlorenc commented Oct 10, 2019

tekton-robot commented Oct 10, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 11, 2019

tekton-robot commented Oct 11, 2019

dlorenc commented Oct 11, 2019

imjasonh commented Oct 11, 2019