Download variant analysis results#1559
Conversation
elenatanasoiu
force-pushed
the
elenatanasoiu/download-variant-analysis-results
branch
from
f3cdd7e to
3633f71
Compare
| return { | ||
| accessMismatchRepos: createMockSkippedRepoGroup(), | ||
| noCodeqlDbRepos: createMockSkippedRepoGroup(), | ||
| notFoundRepos: createMockSkippedRepoGroup(), |
There was a problem hiding this comment.
Should we ensure that this matches the actual data we receive and make the notFoundRepos mock separate/ The notFoundRepos does not have an id or private, while all the other groups will always have both.
There was a problem hiding this comment.
I wonder why this wasn't caught by our type system. I've added a separate method to create this: 7dc5eeb
There was a problem hiding this comment.
It seems like we don't have a different type for the not found repos in the shared VariantAnalysis, while we do have a separate type in the gh-api VariantAnalysis. It might make sense to create a separate type for it, but that's independent of this PR.
| cancellationToken = { | ||
| isCancellationRequested: false | ||
| } as unknown as CancellationToken; |
There was a problem hiding this comment.
Since this is running in VSCode, could we use a real cancellation token instead using the CancellationTokenSource?
There was a problem hiding this comment.
Sure! It does seem a bit shady to use unknown. Have added the change here.
There was a problem hiding this comment.
Thanks! Does it make sense to not mock the cancellation token at all? Instead of mocking the cancellation token source, can we just call new CancellationTokenSource()?
There was a problem hiding this comment.
Ah I see what you mean! I'll take a look at this in a separate issue.
In a previous PR [1] we introduced factories for generating variant analyses (and their associated objects) that were returned from the API. Let's also introduce factories for generating their VSCode equivalent. We can immediately use them for generating a VariantAnalysis object for the monitor tests. [1]: #1545
This will download the result for a particular repo by making a call to the download URL via octokit.
This method will be called from the VariantAnalysisMonitor once a new repo has been scanned. It will then perform an API request to get the repo task for it, which will contain an `artifact_url`. Finally it will use the API method we introduced in the previous commit to download the result for the repo and then save it on disk.
In the next commit, we'll also import `commands`.
This introduces a new `autoDownloadVariantAnalysisResult` command which will be called by the VariantAnalysisMonitor every time it detects a new repo has been scanned. In turn, this will use the `autoDownloadVariantAnalysisResult` method which we defined in an earlier commit on the VariantAnalysisManager.
This won't have an `id` field. We initially generated this the same way we did for all other skipped repos, but this one is special because it's only providing the fullName field, while the others also provide `id` and `private`.
elenatanasoiu
force-pushed
the
elenatanasoiu/download-variant-analysis-results
branch
from
74f03ed to
77c20cf
Compare
elenatanasoiu
force-pushed
the
elenatanasoiu/download-variant-analysis-results
branch
from
77c20cf to
f56f017
Compare
koesie10
left a comment
koesie10
left a comment
There was a problem hiding this comment.
There's still one outstanding comment about creating a new CancellationTokenSource instead of mocking it, but that can also be done in a follow-up PR.
|
Thanks @koesie10! I've created an issue for that as well. |
elenatanasoiu
deleted the
elenatanasoiu/download-variant-analysis-results
branch
2 participants
🚨 Please review this PR commit-by-commit.
What
Follow-up from this PR where we introduced a monitor to check on the status of the variant analysis.
We'd like to auto-download results from the variant analysis as they become ready.
Why
Based on a previous discussion while implementing the variant analysis monitor, we decided that we'd like to trigger separate commands for actually downloading the results rather than performing it in the same process as the monitor.
The reason for this is that it would delay the polling and isn't really connected to what the monitor is supposed to do.
Future
In a separate PR, we'll also change the monitor and manager in order to emit events to the UI. These will be the various states of a download.
Checklist
ready-for-doc-reviewlabel there.