Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: avoid panic when upgrading pipeline in tektonconfig #2482

Merged
merged 1 commit into from
Jan 8, 2025
Merged

fix: avoid panic when upgrading pipeline in tektonconfig #2482

merged 1 commit into from
Jan 8, 2025

Conversation

l-qing
Copy link
Contributor

@l-qing l-qing commented Jan 3, 2025

fix #2476

Changes

Submitter Checklist

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

See the contribution guide for more details.

Release Notes

fix: avoid panic when upgrading pipeline in tektonconfig

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 3, 2025
@tekton-robot
Copy link
Contributor

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.

@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 3, 2025
@pratap0007
Copy link
Contributor

/ok-to-test

@tekton-robot 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 Jan 6, 2025
@tekton-robot
Copy link
Contributor

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
vdemeester reviewed Jan 6, 2025
View reviewed changes
pkg/reconciler/shared/tektonconfig/upgrade/pre_upgrade_test.go
@@ -82,7 +82,7 @@ func TestUpgradePipelineProperties(t *testing.T) {
Spec: v1alpha1.TektonConfigSpec{
Pipeline: v1alpha1.Pipeline{
PipelineProperties: v1alpha1.PipelineProperties{
EnableStepActions: ptr.Bool(false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@l-qing l-qing force-pushed the fix/panic-when-upgrade branch from 2908a49 to 465da6b Compare January 6, 2025 09:15
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 6, 2025
@tekton-robot
Copy link
Contributor

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
Copy link
Member

jkhelil commented Jan 6, 2025

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

@l-qing l-qing force-pushed the fix/panic-when-upgrade branch from 465da6b to f2d6add Compare January 7, 2025 08:46
@tekton-robot
Copy link
Contributor

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
Copy link
Contributor Author

l-qing commented Jan 7, 2025

@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
Copy link
Contributor

@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
Copy link
Member

jkhelil commented Jan 8, 2025 •
edited
Loading

@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
Copy link
Contributor Author

l-qing commented Jan 8, 2025

@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 l-qing force-pushed the fix/panic-when-upgrade branch from f2d6add to d3bea0a Compare January 8, 2025 06:52
@l-qing
Copy link
Contributor Author

l-qing commented Jan 8, 2025

/retest

@jkhelil
Copy link
Member

jkhelil commented Jan 8, 2025

/lgtm

@jkhelil
Copy link
Member

jkhelil commented Jan 8, 2025

/approve

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2025
@tekton-robot
Copy link
Contributor

[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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2025
@tekton-robot tekton-robot merged commit aea48e1 into tektoncd:main Jan 8, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester left review comments

@jkhelil jkhelil Awaiting requested review from jkhelil

@piyush-garg piyush-garg Awaiting requested review from piyush-garg

Assignees

@jkhelil jkhelil

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
5 participants
@l-qing @tekton-robot @pratap0007 @jkhelil @vdemeester