fix: avoid panic when upgrading pipeline in tektonconfig

[l-qing]

fix #2476

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Run make test lint before submitting a PR
• 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.

Release Notes

fix: avoid panic when upgrading pipeline in tektonconfig

[tekton-robot]

Hi @l-qing. 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.

[pratap0007]

/ok-to-test

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/shared/tektonconfig/upgrade/pre_upgrade.go	83.3%	84.2%	0.9

[vdemeester]

Any reason for changing this ? Shouldn't we instead add another test ?

[l-qing]

I just didn't want to add another test case. 😆

But if the logic of SetDefaults changes in the future, for example, changing the default value from false to true, it may cause the unit test to fail.
I will add another case.

[l-qing]

I have resubmitted the code and made adjustments to the unit test.
Please help review it when you have time. Thank you!

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/shared/tektonconfig/upgrade/pre_upgrade.go	48.4%	50.0%	1.6

[jkhelil]

@l-qing I cant reproduce the issue, with main branch, do you know what creates the panic

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/shared/tektonconfig/upgrade/pre_upgrade.go	48.4%	50.0%	1.6

[l-qing]

@l-qing I cant reproduce the issue, with main branch, do you know what creates the panic

Reproduction steps

# 1. apply v0.67.0
$ kubectl apply -f https://storage.googleapis.com/tekton-releases/operator/previous/v0.67.0/release.yaml

# 2. wait pipeline ready

# 3. apply v0.74.0
$ kubectl apply -f https://storage.googleapis.com/tekton-releases/operator/previous/v0.74.0/release.yaml

# 4. Waiting for reproduction.

Although normally it should not upgrade directly from v0.67 to v0.74.
Theoretically, as long as there are no changes to the tektonpipelines.operator.tekton.dev resource after upgrading from v0.67 to v0.70, the default webhook will not be triggered, and there will still be issues when upgrading to v0.74 next time.

[pratap0007]

@l-qing Thanks for sharing the reproduction steps and the error is happening due to this as EnableStepActions field was not introduced in v0.67 , I think we need to handle it and we should not set the default value cc: @savitaashture @jkhelil

[jkhelil]

@l-qing Thanks for sharing the reproduction steps and the error is happening due to this as EnableStepActions field was not introduced in v0.67 , I think we need to handle it and we should not set the default value cc: @savitaashture @jkhelil

+1, Would you update your PR to fix the nil pointer dereference please @l-qing

[l-qing]

@l-qing Thanks for sharing the reproduction steps and the error is happening due to this as EnableStepActions field was not introduced in v0.67 , I think we need to handle it and we should not set the default value cc: @savitaashture @jkhelil

I previously thought that calling SetDefaults here, followed immediately by Update below, would have a similar effect. Ultimately, both would call the SetDefaults method.

However, if the behavior of SetDefaults is modified in the future, it may not trigger an update and could indeed lead to other issues. I'll adjust my PR.

[l-qing]

/retest

[jkhelil]

/lgtm

[jkhelil]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkhelil

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 [jkhelil]

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