-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Error handling for failures on registered actions for Custom Checks #10914
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
I have two suggestions for fixing - let's either discuss or fix - then I'm ready to signoff
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely achieving the goal - but let's make the code little more efficient and less repetitive
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs
Outdated
Show resolved
Hide resolved
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs
Outdated
Show resolved
Hide resolved
src/Build/BuildCheck/Infrastructure/BuildCheckCentralContext.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Fixes #10522
Context
Second PR for that issue. This PR covers the cases in a Custom Check where there is an exception while registering actions, and when running said actions.
Since actions are individuals, we do not deregister the whole analyzer when one fails.
It has been decided to expose these errors are warnings instead to not break the build where they show up.
Changes Made
Added some try catch statements so errors do not break builds. Messages have been added to each case for more detail as we do not preserve anything else from the exception.
Testing
Reenabled unit test and added the previously commented out cases.
Notes