Do not panic if Timeout is nil

[vdemeester]

Changes

TaskRun and PipelineRun should have a default Timeout set by the
webhook. That said, if the webhook didn't do its work, or the object
where created before those webhook defaults, it could be nil and makes
the controller panicking.

This is a simplest fix for #1075 than #1083 to make the simplest fix for the bug-fix release. Closes #1075.

/cc @bobcatfish

Signed-off-by: Vincent Demeester [email protected]

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 and TaskRun to build and release this image

Release Notes

Fix controller panicking on missing Timeout on TaskRun (existing or new)

[tekton-robot]

[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

[bobcatfish]

woot seems like a reasonable simple fix for this! Thanks @vdemeester 🎉

/lgtm
/meow space

[tekton-robot]

@bobcatfish:

In response to this:

woot seems like a reasonable simple fix for this! Thanks @vdemeester 🎉

/lgtm
/meow space

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.

[bobcatfish]

hm I wonder if we should add a test case @vdemeester

I think we need to revisit this asap anyway so its not critical 🤔 and like you were pointing out in the original PR, we shouldn't get into this state anyway (once we fix the defaulting) 🤔🤔🤔

[vdemeester]

hm I wonder if we should add a test case @vdemeester

I think we need to revisit this asap anyway so its not critical thinking and like you were pointing out in the original PR, we shouldn't get into this state anyway (once we fix the defaulting) thinkingthinkingthinking

Let me take a look real quick

/hold

[vdemeester]

Updated with a test case 👼
/hold cancel

[dlorenc]

/lgtm

[bobcatfish]

hm this is interesting cuz it means if the order of the call to this function were to change, this could still be nil 🤔

[bobcatfish]

Thanks for adding the tests! ❤️❤️❤️