Changes to add results to a task #1888

othomann · 2020-01-16T21:25:15Z

Changes

This is the first set of changes that modifies the task spec to add results. It also modifies the entrypoint to feed the termination log with the expected results.
It also adds a new cli argument to the entrypoint cmd (-results).

Part of #1273

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] Updated developer documentation
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

Add results to the task spec and handle all changes in the entrypoint code.

tekton-robot · 2020-01-16T21:30:58Z

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/entrypoint/entrypointer.go	95.0%	62.5%	-32.5

pkg/apis/pipeline/v1alpha1/resource_types.go

pkg/entrypoint/entrypointer.go

pkg/pod/entrypoint.go

tekton-robot · 2020-01-16T22:09:32Z

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/entrypoint/entrypointer.go	95.0%	62.5%	-32.5

tekton-robot · 2020-01-16T22:12:07Z

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/entrypoint/entrypointer.go	95.0%	62.5%	-32.5

pkg/apis/pipeline/v1alpha1/resource_types.go

pkg/entrypoint/entrypointer.go

pkg/pod/entrypoint.go

pkg/entrypoint/entrypointer.go

pkg/apis/pipeline/v1alpha1/task_types.go

tekton-robot · 2020-01-17T16:48:06Z

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/entrypoint/entrypointer.go	95.0%	64.5%	-30.5

tekton-robot · 2020-01-17T18:18:51Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T18:23:59Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T18:54:02Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T18:59:59Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

eddie4941 · 2020-01-17T19:10:52Z

pkg/entrypoint/entrypointer.go

+		output = append(output, v1alpha1.PipelineResourceResult{
+			Key:        resultFile,
+			Value:      string(fileContents),
+			ResultType: "result",


Can we use the new ResultType here instead of hard coding this string?

ghost · 2020-01-17T18:57:49Z

docs/developers/README.md

@@ -185,7 +185,7 @@ There are known issues with the existing implementation of sidecars:

 - When the `nop` image does provide the sidecar's command, the sidecar will continue to
 run even after `nop` has been swapped into the sidecar container's image
-field. See https://github.com/tektoncd/pipeline/issues/1347 for the issue
+field. See <https://github.com/tektoncd/pipeline/issues/1347> for the issue


Minor nit: since we're changing this file, I suggest updating it to See [the issue tracking this bug](https://github.com/tektoncd/pipeline/issues/1347).

ghost · 2020-01-17T18:59:31Z

pkg/apis/pipeline/v1alpha1/task_types.go

+	// Name the given name
+	Name string `json:"name"`
+
+	// Description the given description


Suggest tweaking the wording and adding an +optional tag so it becomes:

// Description is a human-readable description of the result // +optional

tekton-robot · 2020-01-17T19:28:29Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T19:29:01Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T21:04:58Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T21:05:53Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

tekton-robot · 2020-01-17T21:05:57Z

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/entrypoint/entrypointer.go	95.0%	57.1%	-37.9

othomann · 2020-01-17T21:15:47Z

/test pull-tekton-pipeline-integration-tests

bobcatfish · 2020-01-17T21:31:47Z

I0117 21:16:30.477] 2020/01/17 21:16:30 main.go:734: provider gke, will acquire project type gke-project from boskos
I0117 21:21:30.475] 2020/01/17 21:21:30 main.go:316: Something went wrong: failed to prepare test environment: --provider=gke boskos failed to acquire project: resources not found

yikes! twice in a row 🤔

tekton-robot · 2020-01-20T17:42: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/entrypoint/entrypointer.go	95.0%	54.1%	-40.9

othomann · 2020-01-20T18:09:00Z

@vdemeester What kind of doc update do you want?

ghost · 2020-01-21T19:44:11Z

cmd/entrypoint/main.go

@@ -51,6 +52,12 @@ func main() {
 		Waiter:          &realWaiter{},
 		Runner:          &realRunner{},
 		PostWriter:      &realPostWriter{},
+		Results:         strings.Split(*results, ","),
+	}
+	if len(e.Results) >= 1 && e.Results[0] != "" {


I am curious - what is the purpose of the e.Results[0] != "" check?

This is because strings.Split("", ",") returns an array with an empty string in it. So an empty string means we try to read the folder as a file and it fails.

ghost · 2020-01-21T19:51:15Z

Since this change is mostly internal to Tekton (for now... it will be exposed to users in an upcoming PR based on this work) I think the best place for documentation would be in docs/developers/README.md. We could add a section called "How task results are defined and outputted by a task" or something like that. This could then explain the -results flag for the entrypoint, the directory that is created for results, the files that are written for each result, and finally the way this data is formatted in the termination log of each step.

Does that make sense?

othomann · 2020-01-21T20:24:46Z

@sbwsg should the doc update be part of this PR?

tekton-robot · 2020-01-21T20:25:24Z

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/entrypoint/entrypointer.go	95.0%	54.1%	-40.9

tekton-robot · 2020-01-21T20:25: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/entrypoint/entrypointer.go	95.0%	54.1%	-40.9

ghost · 2020-01-21T20:26:07Z

@othomann yeah I think so.

ghost · 2020-01-21T20:35:01Z

/approve

othomann · 2020-01-21T20:35:02Z

@sbwsg ok done. Commits squashed and tests are running again. Let me know if there is anything else that is needed to get this approved.

tekton-robot · 2020-01-21T20:35:13Z

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/entrypoint/entrypointer.go	95.0%	54.1%	-40.9

tekton-robot · 2020-01-21T20:36:36Z

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/entrypoint/entrypointer.go	95.0%	54.1%	-40.9

vdemeester

/lgtm
/meow

I'll update v1alpha2 struct accordingly 👼

docs/developers/README.md

tekton-robot · 2020-01-22T09:44:00Z

@vdemeester:

In response to this:

/lgtm
/meow

I'll update v1alpha2 struct accordingly 👼

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2020-01-22T09:44:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

vdemeester · 2020-01-22T11:03:55Z

I0122 10:31:14.503] pkg/pod/status.go:259:13: unslice: could simplify podInstance.Status.ContainerStatuses[:] to podInstance.Status.ContainerStatuses (gocritic)
I0122 10:31:14.503] 	sort.Slice(podInstance.Status.ContainerStatuses[:], func(i, j int) bool {
I0122 10:31:14.503] 	           ^
I0122 10:31:14.503] cmd/pullrequest-init/main.go:21: File is not `goimports`-ed (goimports)
I0122 10:31:14.504] 	"fmt"

Interesting, the rebase made the lint fail 😅

vdemeester · 2020-01-22T14:15:34Z

/test pull-tekton-pipeline-build-tests

othomann · 2020-01-22T14:39:44Z

@vdemeester I haven't changed the files pkg/pod/status.go or cmd/pullrequest-init/main.go so I don't see why this is reported against this PR.
Once this is merged, I'll work with Eduardo to get the next PR for updating the task run status.

vdemeester · 2020-01-22T15:27:33Z

@othomann it was some previous PR merged that did that, it got fixed 😉

tekton-robot requested review from bobcatfish and dlorenc January 16, 2020 21:25

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 16, 2020

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 16, 2020

othomann requested a review from a user January 16, 2020 21:26

eddie4941 reviewed Jan 16, 2020

View reviewed changes

pkg/apis/pipeline/v1alpha1/resource_types.go Outdated Show resolved Hide resolved

pkg/entrypoint/entrypointer.go Outdated Show resolved Hide resolved

pkg/pod/entrypoint.go Outdated Show resolved Hide resolved

othomann force-pushed the task_results branch from 8272710 to 2ee1b46 Compare January 16, 2020 22:07

ghost reviewed Jan 17, 2020

View reviewed changes

othomann force-pushed the task_results branch from 24ef369 to 2f81a4b Compare January 17, 2020 18:51

othomann force-pushed the task_results branch from 2f81a4b to d8a0190 Compare January 17, 2020 18:56

eddie4941 reviewed Jan 17, 2020

View reviewed changes

ghost reviewed Jan 17, 2020

View reviewed changes

othomann force-pushed the task_results branch from d5a867a to 2351005 Compare January 17, 2020 19:26

othomann force-pushed the task_results branch 2 times, most recently from 8b35cdf to fe71c51 Compare January 17, 2020 21:03

ghost reviewed Jan 21, 2020

View reviewed changes

othomann force-pushed the task_results branch from d66bb0d to 402cbd7 Compare January 21, 2020 20:23

othomann force-pushed the task_results branch from 7429052 to 8ebcada Compare January 21, 2020 20:31

Changes to add results to a task

b7996ee

othomann force-pushed the task_results branch from 8ebcada to b7996ee Compare January 21, 2020 20:33

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2020

vdemeester approved these changes Jan 22, 2020

View reviewed changes

docs/developers/README.md Show resolved Hide resolved

tekton-robot assigned vdemeester Jan 22, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2020

tekton-robot merged commit 07c6201 into tektoncd:master Jan 22, 2020

vdemeester mentioned this pull request Jan 27, 2020

Port TaskResults and TaskRunResults to v1alpha2 🎋 #1948

Merged

3 tasks

vdemeester mentioned this pull request Mar 3, 2020

Add support for result in Pipeline/Task (and Runs) tektoncd/cli#766

Closed

ghost mentioned this pull request May 5, 2020

Question: Is TaskRunResult auto populated? #2553

Closed

Changes to add results to a task #1888

Changes to add results to a task #1888

Conversation

othomann commented Jan 16, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jan 16, 2020

tekton-robot commented Jan 16, 2020

tekton-robot commented Jan 16, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

eddie4941 Jan 17, 2020

Choose a reason for hiding this comment

ghost Jan 17, 2020

Choose a reason for hiding this comment

ghost Jan 17, 2020

Choose a reason for hiding this comment

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

tekton-robot commented Jan 17, 2020

othomann commented Jan 17, 2020

bobcatfish commented Jan 17, 2020

tekton-robot commented Jan 20, 2020

othomann commented Jan 20, 2020

ghost Jan 21, 2020

Choose a reason for hiding this comment

othomann Jan 21, 2020

Choose a reason for hiding this comment

ghost commented Jan 21, 2020

othomann commented Jan 21, 2020

tekton-robot commented Jan 21, 2020

tekton-robot commented Jan 21, 2020

ghost commented Jan 21, 2020

ghost commented Jan 21, 2020

othomann commented Jan 21, 2020

tekton-robot commented Jan 21, 2020

tekton-robot commented Jan 21, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 22, 2020

tekton-robot commented Jan 22, 2020

vdemeester commented Jan 22, 2020

vdemeester commented Jan 22, 2020

othomann commented Jan 22, 2020

vdemeester commented Jan 22, 2020

othomann commented Jan 16, 2020 •

edited

Loading