-
Notifications
You must be signed in to change notification settings - Fork 333
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
Improve the robustness of creating and uploading debug artifacts #2486
Improve the robustness of creating and uploading debug artifacts #2486
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.
Just a jsdoc comment that can be improved. Otherwise, 👍🏼 .
/** | ||
* Try to get the SARIF result path for the given language. | ||
* | ||
* If an error occurs, log it and return an empty list. | ||
*/ |
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 comment and the function name are a little misleading because this function has a side effect that is not mentioned or implied.
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.
Good point, I'll get the PR in and address this as follow up.
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.
Also looks good to me! Makes sense, the original refactor dropped some of the error catching where the original debug artifact upload methods were called 👍
Tolerate failures when creating and uploading debug artifacts, and add a fallback such that if bundling the database with the CLI fails, we will fall back to creating a partial database bundle.
Also use a helper function to simplify the common pattern
wrapError(e).message
.Merge / deployment checklist