Adding nil Checks to TaskRun and PipelineRun Describe Commands #418

danielhelfand · 2019-10-28T21:25:13Z

Closes #380

This pull request adds nil checks to tkn pr desc and tkn tr desc to prevent describe commands from not working when fields aren't present on specs.

For TaskRef, a nil check function has been added to validate.go. For PipelineRef, an empty string check was added that will determine whether to show the PipelineRef. For ResourceRef, nothing has been implemented as an empty string is returned when not present.

This will need to be updated for the release of pipelines with the prs mentioned below that will change PipelineRef and ResourceRef to nil when not present.

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)
Regenerate the manpages and docs with make docs and make man if needed.
Run the code checkers with make check
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Check whether fields are present on taskrun and pipelinerun specs

tekton-robot · 2019-10-28T21:37:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/formatted/tekton.go	Do not exist	0.0%

piyush-garg · 2019-10-29T03:05:25Z

I think this is the case with Taskref in taskrun, Pipelineref in pipelinerun, resourcespec in both taskrun and pipelinerun.

We need to handle all and also in some way like in case of resourceref, if we can show the details that will be good

danielhelfand · 2019-10-29T03:43:05Z

I think this is the case with Taskref in taskrun, Pipelineref in pipelinerun, resourcespec in both taskrun and pipelinerun.

We need to handle all and also in some way like in case of resourceref, if we can show the details that will be good

I can certainly cover the other cases with nil checks in fomatted/tekton.go and also add resource details. Can we perhaps split these into different issues though? One for adding nil checks and one for showing details?

I can repurpose this issue for all nil checks you mentioned above.

chmouel · 2019-10-29T09:02:24Z

pkg/cmd/taskrun/describe.go

@@ -32,7 +32,7 @@ import (

 const templ = `Name:	{{ .TaskRun.Name }}
 Namespace:	{{ .TaskRun.Namespace }}
-Task Ref:	{{ .TaskRun.Spec.TaskRef.Name }}
+Task Ref:    {{ taskRefExists .TaskRun.Spec }}


would it be better to just skip showing Task Ref: label if the task ref is not present (instead of showing that it isnt
) ?

Yeah, agreed. We can just test if the result of taskRefExists is an empty string and show nothing if it's blank.

pkg/cmd/taskrun/describe_test.go

tekton-robot · 2019-10-29T12:41:04Z

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

File	Old Coverage	New Coverage	Delta
pkg/formatted/tekton.go	Do not exist	0.0%

tekton-robot · 2019-10-29T16:34:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipelinerun/describe.go	91.1%	93.3%	2.2
pkg/formatted/tekton.go	Do not exist	0.0%

tekton-robot · 2019-10-29T16:48:30Z

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

File	Old Coverage	New Coverage	Delta
pkg/helper/validate/validate.go	85.7%	42.9%	-42.9

tekton-robot · 2019-10-30T03:53:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipelinerun/describe.go	93.3%	91.1%	-2.2
pkg/helper/validate/validate.go	85.7%	81.8%	-3.9

tekton-robot · 2019-10-30T05:08:12Z

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

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipelinerun/describe.go	93.3%	91.1%	-2.2
pkg/helper/validate/validate.go	85.7%	90.0%	4.3

danielhelfand · 2019-10-30T05:13:54Z

Following up here, I do not believe it is necessary to add nil checks for PipelineRef and ResourceRef. TaskRef appears to be a unique case that can be nil, and that appears to be causing the issue.

tekton-robot · 2019-10-30T05:20:21Z

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

File	Old Coverage	New Coverage	Delta
pkg/helper/validate/validate.go	85.7%	90.0%	4.3

vdemeester · 2019-10-30T08:57:04Z

Following up here, I do not believe it is necessary to add nil checks for PipelineRef and ResourceRef. TaskRef appears to be a unique case that can be nil, and that appears to be causing the issue.

You are right but I think this is an oversight of tektoncd/pipeline, or at least it is not consistent between TaskRun and PipelineRun. In any case, we may also want to not display PipelineRef if it's empty (it's not gonna be nil but it might be empty)

danielhelfand · 2019-10-30T12:22:28Z

Following up here, I do not believe it is necessary to add nil checks for PipelineRef and ResourceRef. TaskRef appears to be a unique case that can be nil, and that appears to be causing the issue.

You are right but I think this is an oversight of tektoncd/pipeline, or at least it is not consistent between TaskRun and PipelineRun. In any case, we may also want to not display PipelineRef if it's empty (it's not gonna be nil but it might be empty)

@vdemeester Yes, agreed on the oversight. It might be worthwhile to report this to pipelines.

The current approach here does not display PipelineRef when it's empty though:

{{- if ne .PipelineRun.Spec.PipelineRef.Name "" }}
Pipeline Ref:	{{ .PipelineRun.Spec.PipelineRef.Name }}
{{- end }}

And there's a test case that's been added to cover this (i.e. TestPipelineRunDescribe_without_pipelineref).

vdemeester · 2019-10-30T14:07:02Z

@danielhelfand tektoncd/pipeline#1497 and tektoncd/pipeline#1495 😛

vdemeester

/lgtm

danielhelfand · 2019-10-30T14:11:38Z

@vdemeester Thanks! I'll create an issue for updating this when we update the version of pipelines.

piyush-garg · 2019-10-31T09:46:05Z

pkg/helper/validate/validate.go

+func TaskRefExists(spec v1alpha1.TaskRunSpec) string {
+
+	if spec.TaskRef == nil {
+		return fieldNotPresent


we can just return "" here

I would personally prefer to keep it to communicate what an empty string represents. It can be used for PipelineRef, ResourceRef, TaskResourceBinding, and PipelineResourceBinding as well when nil checks can be added for those with next pipelines version update.

piyush-garg

/lgtm

chmouel · 2019-10-31T15:23:46Z

/lgtm
/approve
/meow

Thanks 🙇🏽‍♂️😻

tekton-robot · 2019-10-31T15:23:47Z

@chmouel:

In response to this:

/lgtm
/approve
/meow

Thanks 🙇🏽‍♂️😻

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 · 2019-10-31T15:23:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chmouel, piyush-garg

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

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

tekton-robot requested review from hrishin and vdemeester October 28, 2019 21:25

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

googlebot added the cla: yes label Oct 28, 2019

chmouel added this to the 0.5.0 🐱 milestone Oct 29, 2019

chmouel reviewed Oct 29, 2019

View reviewed changes

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 29, 2019

danielhelfand changed the title ~~Check if TaskRef Exists for TaskRun Describe~~ [WIP]: Adding nil Checks to TaskRun and PipelineRun Describe Commands Oct 29, 2019

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2019

check if taskref and pipelineref exist for describe commands

bd4e51d

danielhelfand changed the title ~~[WIP]: Adding nil Checks to TaskRun and PipelineRun Describe Commands~~ Adding nil Checks to TaskRun and PipelineRun Describe Commands Oct 30, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2019

vdemeester reviewed Oct 30, 2019

View reviewed changes

tekton-robot assigned vdemeester Oct 30, 2019

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

piyush-garg reviewed Oct 31, 2019

View reviewed changes

danielhelfand mentioned this pull request Oct 31, 2019

Check for ResourceRef and PipelineRef for TaskRun and PipelineRun Describe to be nil #429

Closed

piyush-garg approved these changes Oct 31, 2019

View reviewed changes

tekton-robot assigned piyush-garg Oct 31, 2019

tekton-robot assigned chmouel Oct 31, 2019

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

tekton-robot merged commit d46a084 into tektoncd:master Oct 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding nil Checks to TaskRun and PipelineRun Describe Commands #418

Adding nil Checks to TaskRun and PipelineRun Describe Commands #418

danielhelfand commented Oct 28, 2019 •

edited

Loading

tekton-robot commented Oct 28, 2019

piyush-garg commented Oct 29, 2019

danielhelfand commented Oct 29, 2019 •

edited

Loading

chmouel Oct 29, 2019

danielhelfand Oct 29, 2019

tekton-robot commented Oct 29, 2019

tekton-robot commented Oct 29, 2019

tekton-robot commented Oct 29, 2019

tekton-robot commented Oct 30, 2019

tekton-robot commented Oct 30, 2019

danielhelfand commented Oct 30, 2019

tekton-robot commented Oct 30, 2019

vdemeester commented Oct 30, 2019

danielhelfand commented Oct 30, 2019

vdemeester commented Oct 30, 2019

vdemeester left a comment

danielhelfand commented Oct 30, 2019

piyush-garg Oct 31, 2019

danielhelfand Oct 31, 2019 •

edited

Loading

piyush-garg left a comment

chmouel commented Oct 31, 2019

tekton-robot commented Oct 31, 2019

tekton-robot commented Oct 31, 2019

Adding nil Checks to TaskRun and PipelineRun Describe Commands #418

Adding nil Checks to TaskRun and PipelineRun Describe Commands #418

Conversation

danielhelfand commented Oct 28, 2019 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented Oct 28, 2019

piyush-garg commented Oct 29, 2019

danielhelfand commented Oct 29, 2019 • edited Loading

chmouel Oct 29, 2019

Choose a reason for hiding this comment

danielhelfand Oct 29, 2019

Choose a reason for hiding this comment

tekton-robot commented Oct 29, 2019

tekton-robot commented Oct 29, 2019

tekton-robot commented Oct 29, 2019

tekton-robot commented Oct 30, 2019

tekton-robot commented Oct 30, 2019

danielhelfand commented Oct 30, 2019

tekton-robot commented Oct 30, 2019

vdemeester commented Oct 30, 2019

danielhelfand commented Oct 30, 2019

vdemeester commented Oct 30, 2019

vdemeester left a comment

Choose a reason for hiding this comment

danielhelfand commented Oct 30, 2019

piyush-garg Oct 31, 2019

Choose a reason for hiding this comment

danielhelfand Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

piyush-garg left a comment

Choose a reason for hiding this comment

chmouel commented Oct 31, 2019

tekton-robot commented Oct 31, 2019

tekton-robot commented Oct 31, 2019

danielhelfand commented Oct 28, 2019 •

edited

Loading

danielhelfand commented Oct 29, 2019 •

edited

Loading

danielhelfand Oct 31, 2019 •

edited

Loading