Clean up variant analyses from query history#2118
Conversation
| ); | ||
| return; | ||
| } | ||
| if (!(await pathExists(queryHistoryDirs.variantAnalysesDirPath))) { |
There was a problem hiding this comment.
I think this is ok to do since https://github.com/github/vscode-codeql/blob/main/extensions/ql-vscode/src/extension.ts#L628 is not checking for canary flags.
robertbrignull
left a comment
There was a problem hiding this comment.
One question, but I think this is fine as is. It took me a while to understand what's going on, and the tests are still a bit daunting. So if you're also not too confident you may want to get a second review. I leave it up to you.
| void extLogger.log( | ||
| `Cannot clean up query history directories. Local queries directory does not exist: ${queryHistoryDirs.localQueriesDirPath}`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Do we want to return here? If the local query history directory doesn't exist we can still clean up the variant analysis directory. I suppose we do expect both dirs to exist, so this is still an unexpected error, but do we want to abort fully or try to continue.
There was a problem hiding this comment.
I thought that since this is an exceptional case then it's okay to abort. I don't have a strong opinion though - happy to change it to be more resilient.
There was a problem hiding this comment.
You're right it should be an exceptional case that doesn't happen in practice. Also it's not like one of local or remote queries is a more central part of the extension, so it's not like a user would want to only use one and therefore want to run without one of the directories existing. And we recreate the directories on extension startup. So if this does happen it hopefully won't be a long term problem and will be fixed by restarting.
That's a lot of words, but in short I think it's fine as it is. Feel free to abort if any of the directories don't exist and keep the code simple.
|
I'll leave this open until tomorrow in case @aeisenberg wants to have a look (no pressure though). |
1f7c890 to
06463a2
Compare
Updated query history clean up logic to also clean up variant analyses.
This required some minor refactoring - see individual commits.
Checklist
N/A:
ready-for-doc-reviewlabel there.