-
Notifications
You must be signed in to change notification settings - Fork 227
Add support for 'canceling' status for variant analysis #3405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
681c761
35ae44f
907febe
9e1f44b
f0ea640
6606764
e2d6e6a
c32040f
07edfdf
9dbea25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { RequestError } from "@octokit/request-error"; | |
| import type { | ||
| VariantAnalysis, | ||
| VariantAnalysisScannedRepository, | ||
| VariantAnalysisStatus, | ||
| } from "./shared/variant-analysis"; | ||
| import { | ||
| isFinalVariantAnalysisStatus, | ||
|
|
@@ -36,6 +37,9 @@ export class VariantAnalysisMonitor extends DisposableObject { | |
| private readonly shouldCancelMonitor: ( | ||
| variantAnalysisId: number, | ||
| ) => Promise<boolean>, | ||
| private readonly getVariantAnalysisStatus: ( | ||
| variantAnalysisId: number, | ||
| ) => VariantAnalysisStatus, | ||
| ) { | ||
| super(); | ||
| } | ||
|
|
@@ -121,6 +125,7 @@ export class VariantAnalysisMonitor extends DisposableObject { | |
|
|
||
| variantAnalysis = mapUpdatedVariantAnalysis( | ||
| variantAnalysis, | ||
| this.getVariantAnalysisStatus(variantAnalysis.id), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain why we call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I agree this is necessary for now, but I think it's not ideal. I'd like to look at changing it in a followup tech-debt issue. In the code here, Adding in We could make it more general and fetch the whole variant analysis from the manager. We could also rewrite the monitor so it can track any changes by listening to the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agree it's an area that could do with improvement. IMO it goes hand in hand with #3405 (comment).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a comment. I would like to make some incremental improvements around this and the mapping functions in separate PRs, if that's okay. |
||
| variantAnalysisSummary, | ||
| ); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I had to pass this as a separate arg instead of adding more items to
Pickbecause we want to allow for it to beundefinedfor the case where we map a variant analysis submission to a variant analysis entity (L30).I don't love how this function is shaping up in terms of args (specially with the
Pick<>) but we could tidy that up a bit separately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should rewrite the exposed interface of this file. Right now we have
where
mapVariantAnalysisdepends onmapUpdatedVariantAnalysis. We could rewritemapUpdatedVariantAnalysisto beand then have a third un-exported method that can have
Pickor other ugly things for its args, but it'll be an internal function and not exposed outside the file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's the sort of thing I had in mind (although I'm still not into using Pick and would rather avoid it). Let's play around that in a separate PR.