-
Notifications
You must be signed in to change notification settings - Fork 375
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
Changes from 9 commits
a6c4760
25d2596
9f7e0af
060c11b
7ff117d
b7b85bb
19a1da5
7be3a64
ba3ac6f
e6c9383
0f24d46
7c2bec0
a8ab493
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.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,7 @@ import { | |
ConfigurationError, | ||
wrapError, | ||
checkActionVersion, | ||
cloneObject, | ||
} from "./util"; | ||
import { validateWorkflow } from "./workflow"; | ||
|
||
|
@@ -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
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. 💭 Is it possible that any of these field values are undefined or empty before we pass them to 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. Yes, they might be undefined, but the value of those properties cannot be. |
||
} | ||
|
||
/** Fields of the init status report populated when the tools source is `download`. */ | ||
|
@@ -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, | ||
rvermeulen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
} | ||
|
||
// 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, | ||
|
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.
I'm not sure I understand the logic here 🤔 do you mind explaining it here or in a code comment?
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.
Yes, we use destructuring assignment to unpack the properties of a registry we need to create a new
RegistryConfigNoCredentials
object.Initially I had:
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.
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.
Ah, I see! That makes sense 👍 I was missing the context that we were selecting for
url
andpackages
(it looked to me like we were just returning everything inregistriesInput
)