Gracefully continue if createStatusReportBase throws
#2225
Conversation
angelapwen
force-pushed
the
angelapwen/audit-status-report-exceptions
branch
from
788a476
to
17bf96c
Compare
Previously, we weren't catching any possible exceptions in `createStatusReportBase` and runs would fail if any of the telemetry sub-items threw exceptions. As telemetry should not block the analysis, we continue here even if the status report throws.
angelapwen
force-pushed
the
angelapwen/audit-status-report-exceptions
branch
from
17bf96c
to
9e8cae9
Compare
| await statusReport.sendStatusReport(trapCacheUploadStatusReport); | ||
| } else { | ||
| await statusReport.sendStatusReport(report); | ||
| if (config && didUploadTrapCaches) { |
There was a problem hiding this comment.
Do we ever reasonably expect config to be falsy, but didUploadTrapCaches to be true? If so, we might want to still upload in that case:
| if (config && didUploadTrapCaches) { | |
| if (didUploadTrapCaches) { |
And the change below. But only if this is not already an error state.
There was a problem hiding this comment.
I don't think so — if config is undefined, we should be erroring out much earlier:
codeql-action/src/analyze-action.ts
Line 205 in f421cda
| ...report, | ||
| trap_cache_upload_duration_ms: Math.round(trapCacheUploadTime || 0), | ||
| trap_cache_upload_size_bytes: Math.round( | ||
| await getTotalCacheSize(config.trapCaches, logger), |
There was a problem hiding this comment.
Only if also adding above.
| await getTotalCacheSize(config.trapCaches, logger), | |
| await getTotalCacheSize(config?.trapCaches || {}, logger), |
| status === "success" || | ||
| status === "failure" || | ||
| status === "aborted" || | ||
| status === "user-error" |
There was a problem hiding this comment.
Should this change to configuration-error? (Not in this PR)
There was a problem hiding this comment.
We could: the configuration-error was codified in the overall job status (rather than status for each action, which is what this is). It would be nice to have them use the same syntax. But we would want to make sure that everyone consuming the data are aware about the transition, because we likely have existing monitors/queries/dashboards using user-error. I can write up an internal issue for this.
There was a problem hiding this comment.
Sure. I'd say it's low priority since it's just changing a name.
Previously, we weren't catching any possible exceptions in
createStatusReportBaseand runs would fail if any of the telemetry sub-items threw exceptions. As telemetry should not block the analysis, we continue here even if the status report throws.Merge / deployment checklist