Skip to content
15 changes: 4 additions & 11 deletions lib/analyze-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 5 additions & 16 deletions lib/init-action-post.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 4 additions & 11 deletions lib/upload-lib.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 6 additions & 14 deletions lib/upload-sarif-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ export enum EnvVar {

/**
* Whether to skip uploading SARIF results to GitHub. Intended for testing purposes.
* This setting is implied by `CODEQL_ACTION_TEST_MODE`, but is more specific.
* This setting is implied by but is more specific than `CODEQL_ACTION_TEST_MODE`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to revert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope 😅

*/
SKIP_SARIF_UPLOAD = "CODEQL_ACTION_SKIP_SARIF_UPLOAD",
}
9 changes: 3 additions & 6 deletions src/init-action-post-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
getErrorMessage,
getRequiredEnvParam,
parseMatrixInput,
getSarifUploadSkipReason,
shouldSkipSarifUpload,
wrapError,
} from "./util";
import {
Expand Down Expand Up @@ -80,14 +80,11 @@ async function maybeUploadFailedSarif(
if (
!["always", "failure-only"].includes(
actionsUtil.getUploadValue(shouldUpload),
)
) ||
shouldSkipSarifUpload()
) {
return { upload_failed_run_skipped_because: "SARIF upload is disabled" };
}
const skipReason = getSarifUploadSkipReason();
if (skipReason) {
return { upload_failed_run_skipped_because: skipReason };
}
const category = getCategoryInputOrThrow(workflow, jobName, matrix);
const checkoutPath = getCheckoutPathInputOrThrow(workflow, jobName, matrix);
const databasePath = config.dbLocation;
Expand Down
5 changes: 2 additions & 3 deletions src/upload-lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -357,13 +357,12 @@ async function uploadPayload(
logger.info("Uploading results");

// If in test mode we don't want to upload the results,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is a bit stale now, ditto at the other isInTestMode/shouldSkipSarifUpload switch.

const skipReason = util.getSarifUploadSkipReason();
if (skipReason) {
if (util.shouldSkipSarifUpload()) {
const payloadSaveFile = path.join(
actionsUtil.getTemporaryDirectory(),
"payload.json",
);
logger.info(`${skipReason}. Saving to ${payloadSaveFile}`);
logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This no longer explains why the SARIF upload is disabled. A simple fix might be wording it like this:

Suggested change
logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);
logger.info(`SARIF upload disabled with an environment variable. Saving to ${payloadSaveFile}`);

A better fix would be to include the responsible environment variable, perhaps with:

Suggested change
logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);
logger.info(`SARIF upload disabled by `${ isInTestMode() ? EnvVar.TEST_MODE : EnvVar.SKIP_SARIF_UPLOAD }`. Saving to ${payloadSaveFile}`);

logger.info(`Payload: ${JSON.stringify(payload, null, 2)}`);
fs.writeFileSync(payloadSaveFile, JSON.stringify(payload, null, 2));
return "dummy-sarif-id";
Expand Down
7 changes: 3 additions & 4 deletions src/upload-sarif-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
checkDiskUsage,
getErrorMessage,
initializeEnvironment,
getSarifUploadSkipReason,
shouldSkipSarifUpload,
wrapError,
} from "./util";

Expand Down Expand Up @@ -113,9 +113,8 @@ async function run() {
core.setOutput("sarif-ids", JSON.stringify(uploadResults));

// We don't upload results in test mode, so don't wait for processing
const skipReason = getSarifUploadSkipReason();
if (skipReason) {
core.debug(`${skipReason}. Waiting for processing is disabled.`);
if (shouldSkipSarifUpload()) {
core.debug("SARIF upload disabled. Waiting for processing is disabled.");
} else if (actionsUtil.getRequiredInput("wait-for-processing") === "true") {
if (codeScanningResult !== undefined) {
await upload_lib.waitForProcessing(
Expand Down
12 changes: 3 additions & 9 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -771,16 +771,10 @@ export function isInTestMode(): boolean {
}

/**
* Returns whether we specifically want to skip uploading SARIF files, and if so, why.
* Returns whether we specifically want to skip uploading SARIF files.
*/
export function getSarifUploadSkipReason(): string | null {
if (isInTestMode()) {
return `SARIF upload is disabled via ${EnvVar.TEST_MODE}`;
}
if (process.env[EnvVar.SKIP_SARIF_UPLOAD] === "true") {
return `SARIF upload is disabled via ${EnvVar.SKIP_SARIF_UPLOAD}`;
}
return null;
export function shouldSkipSarifUpload(): boolean {
return isInTestMode() || process.env[EnvVar.SKIP_SARIF_UPLOAD] === "true";
}

/**
Expand Down