Always upload DB when in debug mode #860
Conversation
src/analyze-action.ts
Outdated
| if (config !== undefined && config.debugMode) { | ||
| // Upload the database bundles as an Actions artifact for debugging | ||
| const toUpload: string[] = []; | ||
| for (const language of config.languages) { | ||
| toUpload.push( | ||
| await bundleDb(config, language, await getCodeQL(config.codeQLCmd)) | ||
| ); | ||
| } | ||
| await uploadDebugArtifacts(toUpload, config.dbLocation); | ||
| } |
There was a problem hiding this comment.
We probably want to wrap this in a try-catch block itself so if uploading fails, we continue to do the rest of the work below.
There was a problem hiding this comment.
I also notice that there is both config.debugMode and core.isDebug(). I'm not 100% sure if they are the same. If they are, then this is confusing. If they aren't, then it's still confusing. :) I wonder if we can clean this up (not necessary for this PR, though).
There was a problem hiding this comment.
Thanks, good point. I've put this it in a try-catch block and with the catch logging the error and then continuing.
Regarding your second comment - indeed those are different things.
config.debugModerepresents that thedebugMode: trueoption was set (i.e. https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/troubleshooting-the-codeql-workflow#creating-codeql-debugging-artifacts) which is an option specific to thecodeql-action.core.isDebug()represents that the more general debugging flag for GitHub Actions was set (https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/enabling-debug-logging). This tells all of GitHub Actions to enable enhanced logging and we also increase our logging verbosity when this is set. I don't think we should enable more expensive operations like the one above which uploads the entire DB based on the more general Actions flag. This is particularly true because the Actions flag is enabled at repo-level so for examplesemmle-codehas it on.
I do think we should enable different things based on these flags, but I agree it is a little confusing. I'm not quite sure where in the code it would be appropriate to do this, though. Perhaps we wrap all calls to config.debugMode or core.isDebug() in a helper function and then that helper function can document the difference between the two?
I think this is best left for a follow-up PR, though.
There was a problem hiding this comment.
I'm not quite sure where in the code it would be appropriate to do this, though. Perhaps we wrap all calls to
config.debugModeorcore.isDebug()in a helper function and then that helper function can document the difference between the two?
I think that's the best option.
edoardopirovano
force-pushed
the
always-upload-db
branch
from
713a1dd to
00d4d60
Compare
Moves the uploading of the CodeQL DBs produced that we do if
debug: trueis set into thefinallyblock. This means we will upload the DB even if the analysis fails, which can aid in debugging certain issues (primarily encountered by the Foundations team) where the analysis is failing due to an error in the database.Merge / deployment checklist