-
Notifications
You must be signed in to change notification settings - Fork 226
Add check for id property when running variant analysis #3414
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 all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| const SARIF_RESULTS_QUERY_KINDS = [ | ||
| "problem", | ||
| "alert", | ||
| "path-problem", | ||
| "path-alert", | ||
| ]; | ||
|
|
||
| /** | ||
| * Returns whether this query kind supports producing SARIF results. | ||
| */ | ||
| export function isSarifResultsQueryKind(kind: string | undefined): boolean { | ||
| if (!kind) { | ||
| return false; | ||
| } | ||
|
|
||
| return SARIF_RESULTS_QUERY_KINDS.includes(kind); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import { RepositoriesSort } from "./RepositoriesSort"; | |
| import { RepositoriesFilter } from "./RepositoriesFilter"; | ||
| import { RepositoriesResultFormat } from "./RepositoriesResultFormat"; | ||
| import type { ResultFormat } from "../../variant-analysis/shared/variant-analysis-result-format"; | ||
| import { isSarifResultsQueryKind } from "../../common/query-metadata"; | ||
|
|
||
| type Props = { | ||
| filterSortValue: RepositoriesFilterSortState; | ||
|
|
@@ -47,10 +48,7 @@ const RepositoriesResultFormatColumn = styled(RepositoriesResultFormat)` | |
| function showResultFormatColumn( | ||
| variantAnalysisQueryKind: string | undefined, | ||
| ): boolean { | ||
| return ( | ||
| variantAnalysisQueryKind === "problem" || | ||
| variantAnalysisQueryKind === "path-problem" | ||
| ); | ||
| return isSarifResultsQueryKind(variantAnalysisQueryKind); | ||
|
Contributor
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. It's possible that this could cause a tiny UI inconsistency until https://github.com/github/codeql-variant-analysis-action/blob/4a8a27800b99bca0d1900c940648b70243804e2f/src/codeql.ts#L351 is also updated. This is because we decide to show the column based on the query kind instead of whether there are SARIF results available, because we can't check every repo and we need to show the column before any repos have been analysed. However, I think it's fine to merge this as it's highly unlikely to affect anybody and it's fine so long as we also update the variant analysis action soon. |
||
| } | ||
|
|
||
| export const RepositoriesSearchSortRow = ({ | ||
|
|
||
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.
This shouldn't change behaviour since all queries in the published packs should be using the new query kinds. Also it should be safe anyway and more correct than it was before.