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.

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

Prevent panic in proxy diffing #48216

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Prevent panic in proxy diffing #48216

merged 1 commit into from
Oct 31, 2024

Conversation

rosstimothy
Copy link
Contributor

#47561 incorrectly translated the logic on when to call the diff function for resources. The previous logic only called the differ if the resource had already been seen before, which guaranteed that both the old and new resource provided to the diff function were not nil. However, after the conversion to the generic watcher the diff function was called for new resources, resulting in a nil value being provided for the old resource and caused a panic. The previous behavior has been restored, and the ProxyWatcher test has been updated to set a diff function to prevent regressions.

#47561 incorrectly translated the logic on when to call the diff
function for resources. The previous logic only called the differ
_if_ the resource had already been seen before, which guaranteed
that both the old and new resource provided to the diff function
were not nil. However, after the conversion to the generic watcher
the diff function was called for new resources, resulting in a nil
value being provided for the old resource and caused a panic.
The previous behavior has been restored, and the ProxyWatcher test
has been updated to set a diff function to prevent regressions.
@rosstimothy rosstimothy added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Oct 31, 2024
@rosstimothy rosstimothy marked this pull request as ready for review October 31, 2024 14:25
@github-actions github-actions bot requested review from kimlisa and kiosion October 31, 2024 14:25
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48216.d3pp5qlev8mo18.amplifyapp.com

@rosstimothy rosstimothy added this pull request to the merge queue Oct 31, 2024
Merged via the queue into master with commit 608a4b3 Oct 31, 2024
46 checks passed
@rosstimothy rosstimothy deleted the tross/fix_watcher_diff branch October 31, 2024 15:51
@public-teleport-github-review-bot

@rosstimothy See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants