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

Fix AppendingErrHandler issue causing errors not to be stored in passed down handlers #516

Merged
merged 8 commits into from
Nov 27, 2023

Conversation

inFocus7
Copy link
Contributor

@inFocus7 inFocus7 commented Nov 22, 2023

Changes

  • Use a pointer receiver for AppendErrorHandler
  • Add unit tests to make sure AppendErrorHandler works as expected (appending errors)
    • without the AppendingErrorHandler being a pointer receiver -> the tests will fail.
  • Add very simple pull request template to add information regarding changes + context.

Context

This was caught while debugging an issue in Portal. A failed upsert did not result in a State: Failed in Portal due to no errors in the AppendingErrHandler.

From more debugging I noticed that the AppendingErrHandler did have the expected error during syncList here, but the error did not propagate upwards to SyncLocalCluster here.

This was due to reference vs value/copy modifications, where in this case the errHandler stored the error in a copy of the object instead of the object itself. Updating the AppendingErrHandler methods to use *AppendingErrHandler fixed the issue.

BOT NOTES:
resolves #517

@inFocus7 inFocus7 added the work in progress This pr is still being worked on label Nov 22, 2023
@solo-changelog-bot
Copy link

Issues linked to changelog:
#517

@inFocus7 inFocus7 removed the work in progress This pr is still being worked on label Nov 27, 2023
Copy link
Contributor

@marcogschmidt marcogschmidt left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit

contrib/pkg/output/errhandlers/err_handlers.go Outdated Show resolved Hide resolved
contrib/pkg/output/errhandlers/err_handlers_test.go Outdated Show resolved Hide resolved
marcogschmidt
marcogschmidt previously approved these changes Nov 27, 2023
@inFocus7
Copy link
Contributor Author

github actions were unstable for a while and https://www.githubstatus.com states it's better now. going to look if I can kick them without having to push and require re-reviews

@soloio-bulldozer soloio-bulldozer bot merged commit 460ec3f into main Nov 27, 2023
2 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the fix-errors-not-being-stored-in-error-handlers branch November 27, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix AppendErrHandler not applying errors to original object
3 participants