Introducing InternalTektonResultType as a ResultType #3138

Peaorl · 2020-08-26T14:56:41Z

Changes

This PR introduces a new TektonResultType namely 'InternalTektonResultType'.
Currently TaskRunResultType, PipelineResourceResultType and UnknownResultType exist as a ResultType for a PipelineResourceResult.
All results that conform to one of these ResultTypes are normally exposed as either as a task or resource result.

In light of #3087 the need for a ResultType that is not exposed arises.
In #3087, a Step can emit a result indicating a Step timeout has occurred.
This is a result that should not be exposed hence the need for a new ResultType named InternalTektonResultType.
This PR ensures results of this type are filtered out.

Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed as a TaskRun or PipelineResource result.
Aside from the example in #3087, a present candidate is the result written out by a Step containing a "StartedAt" key.
Up until now this result is filtered out by a specific function.
Marking it as an InternalTektonResultType now allows for this result to automatically be filtered out.

Up until this PR Sidecars can write results.
Since this is not explicitly documented it's unclear whether this is intended behavior.
In this PR it's ensured only Steps can write results.

Additionally this commit brings about refactoring/moving (and sometimes renaming) of functions related to converting Pod statuses to TaskRun statuses from pkg/reconciler/taskrun/taskrun.go to pkg/pod/status/status.go.
This is accompanied with moving unit tests from taskrun_test.go to status_test.go.

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
Release notes block has been filled in or deleted (only if no user facing changes)

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

NONE

tekton-robot · 2020-08-26T14:56:56Z

Hi @Peaorl. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Peaorl · 2020-08-26T14:58:33Z

/kind feature
/kind cleanup

pkg/pod/status.go

afrittoli · 2020-09-01T16:50:41Z

/ok-to-test

tekton-robot · 2020-09-01T16:54:58Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	91.2%	4.2
pkg/reconciler/taskrun/taskrun.go	78.9%	76.4%	-2.6

ghost

Looking good! I left a few comments but I don't think there are any major blockers from my pov.

pkg/pod/status.go

pkg/reconciler/taskrun/taskrun.go

tekton-robot · 2020-09-02T16:29:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	91.8%	4.8
pkg/reconciler/taskrun/taskrun.go	78.9%	76.6%	-2.3

tekton-robot · 2020-09-03T19:50:19Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	92.8%	5.8
pkg/reconciler/taskrun/taskrun.go	78.9%	76.6%	-2.3

tekton-robot · 2020-09-03T20:25:50Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	93.4%	6.4
pkg/reconciler/taskrun/taskrun.go	78.9%	76.6%	-2.3
pkg/termination/parse.go	100.0%	84.2%	-15.8

tekton-robot · 2020-09-03T21:02:14Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	93.4%	6.4
pkg/reconciler/taskrun/taskrun.go	78.9%	76.6%	-2.3
pkg/termination/parse.go	100.0%	84.2%	-15.8

tekton-robot · 2020-09-03T21:22:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	93.4%	6.4
pkg/reconciler/taskrun/taskrun.go	78.9%	76.6%	-2.3
pkg/termination/parse.go	100.0%	84.2%	-15.8

Peaorl · 2020-09-03T22:02:48Z

While migrating unit tests from pkg/reconciler/taskrun/taskrun_test.go to pkg/pod/status/status.go I encountered one test case I don't fully understand.
I believe this test case aims to check whether a taskrun status is set to false if the corresponding task fails.
Nevertheless, this test case does not actually execute a function and the function it presumably would call is not responsible for setting the taskrun status. The described podStatus is also not set to fail.
In this PR I changed the test such that if a task (pod) fails it's checked whether the taskrun status is set accordingly.

Another thing I noticed in the same test is that the description has a termination message with a result that does not follow the struct format of a PipelineResourceResult. For example, the key name does not exist as PipelineResourceResult field.
On master and also in this PR, such "results" get dropped from the taskrun termination message.
Once the termination message is unmarshalled into PipelineResourceResults, JSON objects that don't adhere to the PipelineResourceResult structure are dropped without error by the unmarshal function.

I am wondering, is this behavior intentional? On master, no test case catches this behavior.
This test now records the behavior of dropping incorrect results from the taskrun termination message.
Additionally, ParseMessage now logs incorrect results.
Alternatively, we could decide that incorrect results are kept "as is" for the taskrun termination message.
Note that it's normally impossible for incorrect results to show up in the taskrun termination message if the Tekton code only writes PipelineResourceResults to its termination message.

Lastly, ResourceRef is now a pointer. This ensures that an empty resourceRef field is not marshalled back into the termination message (zero values like a nil pointer are not marshalled) as is the case on master currently.

ghost · 2020-09-10T12:48:24Z

pkg/apis/pipeline/v1beta1/resource_types.go

@@ -123,10 +123,10 @@ type PipelineResourceResult struct {
 	Key          string `json:"key"`
 	Value        string `json:"value"`
 	ResourceName string `json:"resourceName,omitempty"`
-	// This field should be deprecated and removed in the next API version.
+	// The field ResourceName should be deprecated and removed in the next API version.


Is this right? I have to confess I'm a bit confused by these fields and #2694 doesn't make it totally clear. @dlorenc is it ResourceName or ResourceRef that's supposed to be deprecated?

You're right, it should actually be ResourceRef.

tekton-robot · 2020-09-10T17:09:28Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	93.3%	6.3
pkg/reconciler/taskrun/taskrun.go	78.7%	76.2%	-2.4
pkg/termination/parse.go	100.0%	84.2%	-15.8

ghost

While migrating unit tests from pkg/reconciler/taskrun/taskrun_test.go to pkg/pod/status/status.go I encountered one test case I don't fully understand.
I believe this test case aims to check whether a taskrun status is set to false if the corresponding task fails.
Nevertheless, this test case does not actually execute a function and the function it presumably would call is not responsible for setting the taskrun status. The described podStatus is also not set to fail.
In this PR I changed the test such that if a task (pod) fails it's checked whether the taskrun status is set accordingly.

Good catch. I think the idea of the original test was to confirm that a failed Task does not generate any Results. But as you say this test didn't actually call any functions and didn't test anything 😨

Another thing I noticed in the same test is that the description has a termination message with a result that does not follow the struct format of a PipelineResourceResult. For example, the key name does not exist as PipelineResourceResult field.
On master and also in this PR, such "results" get dropped from the taskrun termination message.
Once the termination message is unmarshalled into PipelineResourceResults, JSON objects that don't adhere to the PipelineResourceResult structure are dropped without error by the unmarshal function.

digest and name were fields used in the alpha API that have since been removed. Have a look through the git history of pkg/apis/pipeline/v1alpha1/resource_types.go, for example, and you'll spot those field names. I think the best course of action is what you've done - removing the old test and codifying the behavior in a new test.

Lastly, ResourceRef is now a pointer. This ensures that an empty resourceRef field is not marshalled back into the termination message (zero values like a nil pointer are not marshalled) as is the case on master currently.

👍 I think this is totally fine.

pkg/pod/status.go

pkg/reconciler/taskrun/taskrun.go

ghost · 2020-09-10T18:51:53Z

I left some small nits but no blockers from my perspective!

/approve

tekton-robot · 2020-09-10T18:52:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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]

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

tekton-robot · 2020-09-14T15:25:31Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	93.3%	6.3
pkg/reconciler/taskrun/taskrun.go	78.7%	76.5%	-2.2
pkg/termination/parse.go	100.0%	84.2%	-15.8

Peaorl · 2020-09-15T10:53:41Z

/remove-label cleanup
/remove-label feature

ghost · 2020-09-15T15:33:47Z

/kind feature

imjasonh

Mostly lgtm, just a few style/readability comments. 👍

pkg/pod/status.go

pkg/pod/status_test.go

In light of tektoncd#3087 the need for a ResultType that is not exposed as a TaskRunResult or PipelineResourceResult arises. In tektoncd#3087, a Step can emit a result indicating a Step timeout has occurred. This is a result that should not be exposed hence the need for a new ResultType called InternalTektonResultType. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in tektoncd#3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultType now allows for this result to automatically be filtered out. Additionally this commit brings about refactoring (and sometimes renaming) of functions related to converting pod statuses to taskrun statuses from pkg/reconciler/taskrun/taskrun.go to pkg/pod/status/status.go. This is accompanied with moving unit test cases from taskrun_test.go to status_test.go.

tekton-robot · 2020-09-22T15:03:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	87.0%	93.3%	6.3
pkg/termination/parse.go	100.0%	84.2%	-15.8

imjasonh

/lgtm

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Aug 26, 2020

tekton-robot requested review from dlorenc and pritidesai August 26, 2020 14:56

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2020

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 26, 2020

Peaorl mentioned this pull request Aug 27, 2020

Step timeout #3087

Merged

4 tasks

ghost reviewed Aug 31, 2020

View reviewed changes

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

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

Peaorl force-pushed the internaltektonresulttype branch from de825d0 to d1efa81 Compare September 1, 2020 12:46

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2020

ghost reviewed Sep 1, 2020

View reviewed changes

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

pkg/pod/status.go Show resolved Hide resolved

pkg/reconciler/taskrun/taskrun.go Outdated Show resolved Hide resolved

Peaorl force-pushed the internaltektonresulttype branch from d1efa81 to d59e41a Compare September 2, 2020 16:25

Peaorl marked this pull request as draft September 2, 2020 18:33

Peaorl force-pushed the internaltektonresulttype branch from d59e41a to 8bc0337 Compare September 3, 2020 19:46

Peaorl force-pushed the internaltektonresulttype branch from 8bc0337 to 755f3ff Compare September 3, 2020 20:20

Peaorl force-pushed the internaltektonresulttype branch from 755f3ff to 1d38854 Compare September 3, 2020 20:58

Peaorl force-pushed the internaltektonresulttype branch from 1d38854 to c6f1b5c Compare September 3, 2020 21:18

Peaorl marked this pull request as ready for review September 3, 2020 22:04

ghost reviewed Sep 10, 2020

View reviewed changes

Peaorl force-pushed the internaltektonresulttype branch from c6f1b5c to ce6b61a Compare September 10, 2020 17:05

ghost reviewed Sep 10, 2020

View reviewed changes

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

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

pkg/reconciler/taskrun/taskrun.go Show resolved Hide resolved

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

Peaorl force-pushed the internaltektonresulttype branch from ce6b61a to cc22620 Compare September 14, 2020 15:20

ghost removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 15, 2020

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 15, 2020

Peaorl mentioned this pull request Sep 15, 2020

Incorrect TaskRun status due to different Steps having the same StartedAt and FinishedAt times #3239

Closed

GregDritschler mentioned this pull request Sep 18, 2020

Sort pod container statuses based on Step order in taskSpec #3256

Merged

3 tasks

imjasonh reviewed Sep 22, 2020

View reviewed changes

pkg/pod/status.go Show resolved Hide resolved

pkg/pod/status_test.go Show resolved Hide resolved

Peaorl force-pushed the internaltektonresulttype branch from cc22620 to ebb25cc Compare September 22, 2020 15:01

Peaorl force-pushed the internaltektonresulttype branch from ebb25cc to 20568a1 Compare September 22, 2020 15:01

imjasonh reviewed Sep 22, 2020

View reviewed changes

tekton-robot assigned imjasonh Sep 22, 2020

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

tekton-robot merged commit a9e7b54 into tektoncd:master Sep 22, 2020

Peaorl deleted the internaltektonresulttype branch November 3, 2020 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing InternalTektonResultType as a ResultType #3138

Introducing InternalTektonResultType as a ResultType #3138

Peaorl commented Aug 26, 2020 •

edited

Loading

tekton-robot commented Aug 26, 2020

Peaorl commented Aug 26, 2020 •

edited

Loading

afrittoli commented Sep 1, 2020

tekton-robot commented Sep 1, 2020

ghost left a comment

tekton-robot commented Sep 2, 2020

tekton-robot commented Sep 3, 2020

tekton-robot commented Sep 3, 2020

tekton-robot commented Sep 3, 2020

tekton-robot commented Sep 3, 2020

Peaorl commented Sep 3, 2020

ghost Sep 10, 2020

Peaorl Sep 10, 2020 •

edited

Loading

tekton-robot commented Sep 10, 2020

ghost left a comment

ghost commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 14, 2020

Peaorl commented Sep 15, 2020 •

edited

Loading

ghost commented Sep 15, 2020

imjasonh left a comment

tekton-robot commented Sep 22, 2020

imjasonh left a comment

Introducing InternalTektonResultType as a ResultType #3138

Introducing InternalTektonResultType as a ResultType #3138

Conversation

Peaorl commented Aug 26, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 26, 2020

Peaorl commented Aug 26, 2020 • edited Loading

afrittoli commented Sep 1, 2020

tekton-robot commented Sep 1, 2020

ghost left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 2, 2020

tekton-robot commented Sep 3, 2020

tekton-robot commented Sep 3, 2020

tekton-robot commented Sep 3, 2020

tekton-robot commented Sep 3, 2020

Peaorl commented Sep 3, 2020

ghost Sep 10, 2020

Choose a reason for hiding this comment

Peaorl Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Sep 10, 2020

ghost left a comment

Choose a reason for hiding this comment

ghost commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 14, 2020

Peaorl commented Sep 15, 2020 • edited Loading

ghost commented Sep 15, 2020

imjasonh left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 22, 2020

imjasonh left a comment

Choose a reason for hiding this comment

Peaorl commented Aug 26, 2020 •

edited

Loading

Peaorl commented Aug 26, 2020 •

edited

Loading

Peaorl Sep 10, 2020 •

edited

Loading

Peaorl commented Sep 15, 2020 •

edited

Loading