-
Notifications
You must be signed in to change notification settings - Fork 409
Treat status reports as non-critical #2121
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
Treat status reports as non-critical #2121
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.
Thanks again for this — our telemetry reporting should indeed be non-critical! I've just added a few comments.
I think this may warrant a CHANGELOG entry 🌞 users who may have seen this failure in the past should be happy to see the change.
6145d06 to
d3f9862
Compare
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 looking great. I have a few copy-editing suggestions and one question about 404/403 errors.
ac030dd to
5942183
Compare
| import { SarifFile, ConfigurationError, wrapError } from "./util"; | ||
|
|
||
| const GENERIC_403_MSG = | ||
| "The repo on which this action is running has not opted-in to CodeQL code scanning."; |
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 message and the one below it needs some work, but as you mentioned, it is outside the scope of this PR.
|
Thanks again for your work on this. |
Change `sendStatusReport` to `void`
df25046 to
5a6da1d
Compare
|
Thanks again! Nice work here. |
Resolve github/codeql#15462 (comment) by converting
sendStatusReportinto a function that always returnsvoidand usescore.warninginstead ofcore.errorSplit from:
Merge / deployment checklist