-
Notifications
You must be signed in to change notification settings - Fork 490
Don't record an overlay status when the job was cancelled #4122
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -316,6 +316,7 @@ export async function tryUploadSarifIfRunFailed( | |
| * @param config The CodeQL Action configuration. | ||
| * @param repositoryNwo The name and owner of the repository. | ||
| * @param features Information about enabled features. | ||
| * @param jobStatus The status of the job, as reported by the Actions runtime environment. | ||
| * @param logger The logger to use. | ||
| * @returns The results of uploading the SARIF file for the failure. | ||
| */ | ||
|
|
@@ -331,9 +332,10 @@ export async function uploadFailureInfo( | |
| config: Config, | ||
| repositoryNwo: RepositoryNwo, | ||
| features: FeatureEnablement, | ||
| jobStatus: string | undefined, | ||
| logger: Logger, | ||
| ): Promise<UploadFailedSarifResult> { | ||
| await recordOverlayStatus(codeql, config, features, logger); | ||
| await recordOverlayStatus(codeql, config, features, jobStatus, logger); | ||
|
|
||
| const uploadFailedSarifResult = await tryUploadSarifIfRunFailed( | ||
| config, | ||
|
|
@@ -412,6 +414,21 @@ export async function uploadFailureInfo( | |
| return uploadFailedSarifResult; | ||
| } | ||
|
|
||
| /** | ||
| * Whether one of the CodeQL Actions reported an error for this job, which means the analysis | ||
| * genuinely failed. | ||
| * | ||
| * Note that the converse does not hold: an Action that is terminated abruptly, or that fails before | ||
| * it can gather telemetry, does not get to report anything. | ||
| */ | ||
| function didCodeQlReportError(): boolean { | ||
| const jobStatus = process.env[EnvVar.JOB_STATUS]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a |
||
| return ( | ||
| jobStatus === JobStatus.FailureStatus || | ||
| jobStatus === JobStatus.ConfigErrorStatus | ||
| ); | ||
|
Comment on lines
+426
to
+429
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we end up with one of these if the workflow job was cancelled? E.g. because it caused a thread abort style exception to be thrown at an inconvenient moment? |
||
| } | ||
|
|
||
| /** | ||
| * If overlay base database creation was attempted but the analysis did not complete | ||
| * successfully, save the failure status to the Actions cache so that subsequent runs | ||
|
|
@@ -421,6 +438,7 @@ async function recordOverlayStatus( | |
| codeql: CodeQL, | ||
| config: Config, | ||
| features: FeatureEnablement, | ||
| jobStatus: string | undefined, | ||
| logger: Logger, | ||
| ) { | ||
| if ( | ||
|
|
@@ -431,6 +449,20 @@ async function recordOverlayStatus( | |
| return; | ||
| } | ||
|
|
||
| // A cancelled run tells us nothing about whether the analysis would have succeeded, so recording | ||
| // a failure would disable overlay analysis needlessly. Note that we still record a failure if one | ||
| // of our own Actions reported an error before the run was cancelled. | ||
| if ( | ||
| jobStatus?.trim().toLowerCase() === "cancelled" && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than checking for the case where we don't want to record the overlay status, would it make sense to make this a check for the cases where we do want to record the overlay status? That would be more defensive and guard against the hypothetical case of a new status value that we don't recognise. I would also expect that we'd have a |
||
| !didCodeQlReportError() | ||
| ) { | ||
| logger.info( | ||
| "Not recording an improved incremental analysis failure for this job because the workflow " + | ||
| "run was cancelled.", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const checkRunIdInput = actionsUtil.getOptionalInput("check-run-id"); | ||
| const checkRunId = | ||
| checkRunIdInput !== undefined ? parseInt(checkRunIdInput, 10) : undefined; | ||
|
|
||
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.
Never nice to have to feed this in through an extra action input, but I don't see a better approach for getting hold of the job status. An alternative option might be to set
CODEQL_ACTION_STEP_(init|analyze|...)environment variables / state that we set to e.g.startingwhen the respective action starts and then tosuccessorfailuredepending on the outcome. That should then allow us to identify which step started, succeeded, or failed (gracefully or not). For the overlay status, we could then check that all available environment variables with aCODEQL_ACTION_STEP_prefix aresuccessand none arestartingorfailure. The downside is that it wouldn't catch if the failure isn't related to what happens in CodeQL Action steps, or we fail to even set thestartingvalue.