Add check for id property when running variant analysis#3414
Conversation
| // It's not possible to interpret a BQRS file to SARIF without an id property. | ||
| if ( | ||
| queryMetadata?.kind && | ||
| ["problem", "path-problem"].includes(queryMetadata.kind) && |
There was a problem hiding this comment.
Should we also include alert and path-alert in this list?
There was a problem hiding this comment.
Yes, good point. I've changed this to use a function instead and also used this function in other places where I could find path-problem.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
LGTM
This can technically change behaviour in a couple of places, but it's very minor and I think it's fine and it's only making us more correct and consistent overall.
| queryMetadata.kind === "problem" || | ||
| queryMetadata.kind === "path-problem" | ||
| ) { | ||
| if (isSarifResultsQueryKind(queryMetadata.kind)) { |
There was a problem hiding this comment.
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.
| variantAnalysisQueryKind === "problem" || | ||
| variantAnalysisQueryKind === "path-problem" | ||
| ); | ||
| return isSarifResultsQueryKind(variantAnalysisQueryKind); |
There was a problem hiding this comment.
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.
2 participants
This adds a check for variant analyses to ensure that the
@idmetadata is set when running aproblemorpath-problemquery. This will ensure that this error is caught early.Checklist
ready-for-doc-reviewlabel there.