Upload per-database diagnostic SARIFs on green and red runs #1556
Conversation
Move the definition of the variable from util to shared-environment, and rename.
Adds `codeql database export-diagnostics` command and `CODEQL_ACTION_IS_DATABASE_CLUSTER` internal environment variable
There was a problem hiding this comment.
In the green runs path, we call codeql database interpret-results once per database to produce a SARIF file per DB, combine all these SARIF files together, and then submit a single SARIF file. This has different behaviour to uploading each SARIF file separately, since code scanning will overwrite the existing data for that category each time you upload a new SARIF file.
I think for red runs we can do something simpler — config.dbLocation should always be a DB cluster (at least for all CodeQL versions that support diagnostics). Therefore we should be able to run codeql database export-diagnostics once on that DB cluster and submit the resulting SARIF file.
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
e3b8c1f
to
1d4190a
Compare
|
Unrelated CI failures that I'll fix in a separate PR! |
There was a problem hiding this comment.
Looking good so far. An integration test might be tricky to write due to most of the work happening in post-init. We could work around that by creating a separate Action for the test (like we did for the query filters tests), but I'd be happy to merge this with detailed unit tests instead.
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
bb15428
to
1e4efb1
Compare
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
0a196c3
to
e5a000f
Compare
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
e5a000f
to
6cce51b
Compare
angelapwen
changed the title
|
I've addressed the existing comments other than testing. I need to update some of the existing unit tests to reflect the new behavior, as well as write new ones to confirm that the appropriate export command is called as expected ✍️ |
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
090a18c
to
c193f61
Compare
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
d3e5288
to
b025421
Compare
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
3f5acf0
to
18c0c1a
Compare
angelapwen
changed the title
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
016ce45
to
817a0e5
Compare
| const diagnosticToolExecutionNotification = toolExecutionNotifications[toolExecutionNotifications.length - 1]; | ||
| if (diagnosticToolExecutionNotification.descriptor.id !== 'lang/diagnostics/example' || diagnosticToolExecutionNotification.message.text !== 'Plaintext message') { | ||
| core.setFailed('`toolExecutionNotification` related to this diagnostic was not found in SARIF'); | ||
| } |
There was a problem hiding this comment.
I think this will work with the current implementation, but we probably don't want to rely on the ordering of the SARIF file in general. Could we convert this to instead check inclusion?
There was a problem hiding this comment.
Yep! Done. Not sure if there's a cleaner way to check for inclusion (maybe if I used a filter and checked if there were any elements left afterwards..?)
There was a problem hiding this comment.
Filtering might be slightly neater. You could then also check we have exactly one diagnostic with that ID.
There was a problem hiding this comment.
Ah, that's a benefit — done!
Co-authored-by: Henry Mercer <[email protected]>
angelapwen
force-pushed
the
failed-database-diagnostic
branch
from
92c75df
to
0271b54
Compare
On failed runs we now call
codeql database diagnostic-exportto create and upload a SARIF file for per-database diagnostics. If this file was not created, we fall back on the original behavior of uploading the result ofcodeql export diagnostics.On successful runs, we add the
--sarif-include-diagnosticsoption to thecodeql database interpret-resultscommand.Both the addition of the option to the
interpret-resultscall, and thecodeql database diagnostic-exportcall, are gated behind theExportDiagnosticsEnabledfeature flag.We test the green run scenario with an integration test that adds a diagnostic prior to the
analyzestep, with the feature flag environment variable override to enable the feature. We check the SARIF file generated to make sure it includes the manually added diagnostic.[GitHub internal only]: we test the red run scenario by writing a similar workflow as the one in the integration test, but failing the workflow after adding the diagnostic. See run and corresponding tool status page for the surfaced diagnostic.
Merge / deployment checklist