8000 Extend init complete status report by rvermeulen · Pull Request #2394 · github/codeql-action · GitHub
[go: up one dir, main page]

Skip to content

Extend init complete status report #2394

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

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions lib/codeql.js

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

2 changes: 1 addition & 1 deletion lib/codeql.js.map

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions lib/config-utils.js

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

2 changes: 1 addition & 1 deletion lib/config-utils.js.map

Large diffs are not rendered by default.

22 changes: 22 additions & 0 deletions lib/init-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 lib/init-action.js.map

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions lib/util.js

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

2 changes: 1 addition & 1 deletion lib/util.js.map

Large diffs are not rendered by default.

6 changes: 1 addition & 5 deletions src/codeql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import * as setupCodeql from "./setup-codeql";
import { ToolsFeature, isSupportedToolsFeature } from "./tools-features";
import { shouldEnableIndirectTracing } from "./tracer-config";
import * as util from "./util";
import { BuildMode, wrapError } from "./util";
import { BuildMode, wrapError, cloneObject } from "./util";

type Options = Array<string | number | boolean>;

Expand Down Expand Up @@ -1344,10 +1344,6 @@ async function generateCodeScanningConfig(
return codeScanningConfigFile;
}

function cloneObject<T>(obj: T): T {
return JSON.parse(JSON.stringify(obj)) as T;
}

// This constant sets the size of each TRAP cache in megabytes.
const TRAP_CACHE_SIZE_MB = 1024;

Expand Down
9 changes: 9 additions & 0 deletions src/config-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,15 @@ function parseRegistries(
}
}

export function parseRegistriesWithoutCredentials(
registriesInput?: string,
): RegistryConfigNoCredentials[] | undefined {
return parseRegistries(registriesInput)?.map((r) => {
const { url, packages } = r;
return { url, packages };
});
Comment on lines +887 to +890
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the logic here 🤔 do you mind explaining it here or in a code comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we use destructuring assignment to unpack the properties of a registry we need to create a new RegistryConfigNoCredentials object.

Initially I had:

const { token: _, ...registryWithoutCredentials } = r;

but this triggered an ESLint unused variable error for _.
In hindsight, selecting the properties we actually care about instead of removing current sensitive properties is a better solution in case we add more sensitive properties in the future or rename the current sensitive property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see! That makes sense 👍 I was missing the context that we were selecting for url and packages (it looked to me like we were just returning everything in registriesInput)

}

function isLocal(configPath: string): boolean {
// If the path starts with ./, look locally
if (configPath.indexOf("./") === 0) {
Expand Down
40 changes: 40 additions & 0 deletions src/init-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
ConfigurationError,
wrapError,
checkActionVersion,
cloneObject,
} from "./util";
import { validateWorkflow } from "./workflow";

Expand All @@ -85,12 +86,19 @@ interface InitWithConfigStatusReport extends InitStatusReport {
paths_ignore: string;
/** Comma-separated list of queries sources, from the 'queries' config field or workflow input. */
queries: string;
/** Stringified JSON object of packs, from the 'packs' config field or workflow input. */
packs: string;
/** Comma-separated list of languages for which we are using TRAP caching. */
trap_cache_languages: string;
/** Size of TRAP caches that we downloaded, in bytes. */
trap_cache_download_size_bytes: number;
/** Time taken to download TRAP caches, in milliseconds. */
trap_cache_download_duration_ms: number;
/** Stringified JSON array of registry configuration objects, from the 'registries' config field
or workflow input. **/
registries: string;
/** Stringified JSON object representing a query-filters, from the 'query-filters' config field. **/
query_filters: string;
Comment on lines +89 to +101
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Is it possible that any of these field values are undefined or empty before we pass them to JSON.stringify? If so, what's the output & is that we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they might be undefined, but the value of those properties cannot be.
That being said, let me explicitly use their initialization values according to their type, that is an empty array for registries and query_filters.

}

/** Fields of the init status report populated when the tools source is `download`. */
Expand Down Expand Up @@ -174,18 +182,50 @@ async function sendCompletedStatusReport(
queries.push(...queriesInput.split(","));
}

let packs: Record<string, string[]> = {};
if (
(config.augmentationProperties.packsInputCombines ||
!config.augmentationProperties.packsInput) &&
config.originalUserInput.packs
) {
// Make a copy, because we might modify `packs`.
const copyPacksFromOriginalUserInput = cloneObject(
config.originalUserInput.packs,
);
// If it is an array, then assume there is only a single language being analyzed.
if (Array.isArray(copyPacksFromOriginalUserInput)) {
packs[config.languages[0]] = copyPacksFromOriginalUserInput;
} else {
packs = copyPacksFromOriginalUserInput;
}
}

if (config.augmentationProperties.packsInput) {
packs[config.languages[0]] ??= [];
packs[config.languages[0]].push(
...config.augmentationProperties.packsInput,
);
}

// Append fields that are dependent on `config`
const initWithConfigStatusReport: InitWithConfigStatusReport = {
...initStatusReport,
disable_default_queries: disableDefaultQueries,
paths,
paths_ignore: pathsIgnore,
queries: queries.join(","),
packs: JSON.stringify(packs),
trap_cache_languages: Object.keys(config.trapCaches).join(","),
trap_cache_download_size_bytes: Math.round(
await getTotalCacheSize(config.trapCaches, logger),
),
trap_cache_download_duration_ms: Math.round(config.trapCacheDownloadTime),
query_filters: JSON.stringify(config.originalUserInput["query-filters"]),
registries: JSON.stringify(
configUtils.parseRegistriesWithoutCredentials(
getOptionalInput("registries"),
),
),
};
await sendStatusReport({
...initWithConfigStatusReport,
Expand Down
4 changes: 4 additions & 0 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1100,3 +1100,7 @@ export enum BuildMode {
/** The database will be created by building the source root using manually specified build steps. */
Manual = "manual",
}

export function cloneObject<T>(obj: T): T {
return JSON.parse(JSON.stringify(obj)) as T;
}
Loading
0