Always upload DB when in debug mode - #860
edoardopirovano merged 1 commit into
Conversation
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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).
Sorry, something went wrong.
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.debugMode represents that the debugMode: true option 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 the codeql-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 example semmle-code has 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.
Sorry, something went wrong.
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.debugMode or core.isDebug() in a helper function and then that helper function can document the difference between the two?
I think that's the best option.
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Further refactoring as described above can happen later.
Sorry, something went wrong.
Moves the uploading of the CodeQL DBs produced that we do if debug: true is set into the finally block. 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