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 error masking in standalone runner #794

Merged
merged 4 commits into from
May 20, 2022

Conversation

luhi-DT
Copy link
Collaborator

@luhi-DT luhi-DT commented May 20, 2022

Description

Currently our error masking within the standalone runner is broken. The runner always logs that an error has been masked even though there is no error. This happened because, in the check for masking the error consumeErrorIfNecessary(), we do not check for the error itself but for the pointer, which is never nil. Additionally the defer function set the wrong variable to nil because defer runs a function after the return statement but before the function is actually returned, thus enabling modify returned results, but only named return results can be accessed normally. This caused that the error was set to nil, but not actual returned error, because go already evaluated the return result.

More here: https://stackoverflow.com/questions/48680222/what-is-the-difference-between-named-return-value-and-normal-return-value

How can this be tested?

Before this PR
Deploy cloudNative or applicationMonitoring on a cluster and deploy sample apps. If you now check the logs of the initContainer, you can see the error log at the end of the run. Additionally if an error happens within the run and the failure policy is set to silent, the sample app would crash, because the initContainer is not finished successfully.

After this PR
Deploy cloudNative or applicationMonitoring on a cluster and deploy sample apps. If you now check the logs of the initContainer, should not see any false error log anymore. Additionally if an error happens within the run and the failure policy is set to silent, the sample app is not crashing anymore and the initContainer finishes with 0 even though there was an error.

Checklist

  • Unit tests have been updated/added
  • PR is labeled accordingly

@luhi-DT luhi-DT added the bug Something isn't working label May 20, 2022
@luhi-DT luhi-DT requested a review from a team as a code owner May 20, 2022 09:58
@luhi-DT luhi-DT changed the title Fix nil check and error masking in standalone runner Fix error masking in standalone runner May 20, 2022
Copy link
Contributor

@0sewa0 0sewa0 left a comment

Choose a reason for hiding this comment

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

🪄

Copy link
Contributor

@waodim waodim left a comment

Choose a reason for hiding this comment

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

seems fine to me

@luhi-DT luhi-DT merged commit 56fb071 into master May 20, 2022
@luhi-DT luhi-DT deleted the bugfix/remove-unecessary-error-log-in-standalone-init branch May 20, 2022 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants