Skip to content

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.

Sign up for GitHub

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

Merged
charisk merged 10 commits into from
Feb 27, 2024
Merged

Add support for 'canceling' status for variant analysis #3405

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
681c761
Add support for 'canceling' status for variant analysis
charisk Feb 26, 2024
35ae44f
Add storybook story for canceling state
charisk Feb 26, 2024
907febe
Update CHANGELOG
charisk Feb 26, 2024
9e1f44b
Move logic to mapUpdatedVariantAnalysis
charisk Feb 27, 2024
f0ea640
Update variant analysis stats area when canceling
charisk Feb 27, 2024
6606764
Add some more tests
charisk Feb 27, 2024
e2d6e6a
Add comment to explain why we get the status
charisk Feb 27, 2024
c32040f
Update extensions/ql-vscode/src/variant-analysis/variant-analysis-mon…
charisk Feb 27, 2024
07edfdf
Merge branch 'main' into charisk/variant-analysis-canceling-status
charisk Feb 27, 2024
9dbea25
Fix build after merging main
charisk Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions extensions/ql-vscode/src/query-history/query-status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export function variantAnalysisStatusToQueryStatus(
return QueryStatus.Failed;
case VariantAnalysisStatus.InProgress:
return QueryStatus.InProgress;
case VariantAnalysisStatus.Canceling:
return QueryStatus.InProgress;
case VariantAnalysisStatus.Canceled:
return QueryStatus.Completed;
default:
Expand Down
5 changes: 5 additions & 0 deletions extensions/ql-vscode/src/query-history/store/query-history-variant-analysis-domain-mapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ function mapVariantAnalysisStatusToDto(
return VariantAnalysisStatusDto.Succeeded;
case VariantAnalysisStatus.Failed:
return VariantAnalysisStatusDto.Failed;
case VariantAnalysisStatus.Canceling:
// The canceling state shouldn't be persisted. We can just
// assume that the analysis is still in progress, since the
// canceling state is very short-lived.
return VariantAnalysisStatusDto.InProgress;
case VariantAnalysisStatus.Canceled:
return VariantAnalysisStatusDto.Canceled;
default:
Expand Down
1 change: 1 addition & 0 deletions extensions/ql-vscode/src/variant-analysis/shared/variant-analysis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export enum VariantAnalysisStatus {
InProgress = "inProgress",
Succeeded = "succeeded",
Failed = "failed",
Canceling = "canceling",
Canceled = "canceled",
}

Expand Down
32 changes: 30 additions & 2 deletions extensions/ql-vscode/src/variant-analysis/variant-analysis-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
isVariantAnalysisComplete,
parseVariantAnalysisQueryLanguage,
VariantAnalysisScannedRepositoryDownloadStatus,
VariantAnalysisStatus,
} from "./shared/variant-analysis";
import { getErrorMessage } from "../common/helpers-pure";
import { VariantAnalysisView } from "./variant-analysis-view";
Expand Down Expand Up @@ -611,10 +612,22 @@ export class VariantAnalysisManager
return;
}

if (!this.variantAnalyses.has(variantAnalysis.id)) {
const originalVariantAnalysis = this.variantAnalyses.get(
variantAnalysis.id,
);

if (!originalVariantAnalysis) {
return;
}

// Maintain the canceling status if we are still canceling.
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure adding this code to onVariantAnalysisUpdated is going to do what we want.

As far as I can tell this method is only called from within cancelVariantAnalysis. Once when we set the status to Canceling and once more if we want to set it back to InProgress.

So it won't help when we get updates from the monitoring job which will try to set the status to InProgress. It will also hinder us because it'll undo the change we're trying to make in the catch block of cancelVariantAnalysis.

I think instead we might want to add this logic to mapUpdatedVariantAnalysis?

What do you think? Have I understood things correctly?

Copy link
Copy Markdown
Contributor Author

@charisk charisk Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. I did a last minute refactor here and didn't think through all the cases I had in mind on Friday.

I'm also having some trouble with testing manually atm - I seem to be testing against older versions of the code somehow. I'm either being forgetful with builds or the watcher/build is a bit off.

I'll move some logic to mapUpdatedVariantAnalysis.

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed originally that we pass onVariantAnalysisUpdated into variantAnalysisMonitor.onVariantAnalysisChange, so it was called in other places. But the point still stands about when we want to set that status back when cancelation fails, so the other changes you made are still the right thing.

if (
originalVariantAnalysis.status === VariantAnalysisStatus.Canceling &&
variantAnalysis.status === VariantAnalysisStatus.InProgress
) {
variantAnalysis.status = VariantAnalysisStatus.Canceling;
}

await this.setVariantAnalysis(variantAnalysis);
this._onVariantAnalysisStatusUpdated.fire(variantAnalysis);
}
Expand Down Expand Up @@ -838,6 +851,11 @@ export class VariantAnalysisManager
throw new Error(`No variant analysis with id: ${variantAnalysisId}`);
}

await this.onVariantAnalysisUpdated({
Comment thread
charisk marked this conversation as resolved.
Outdated
...variantAnalysis,
status: VariantAnalysisStatus.Canceling,
});

if (!variantAnalysis.actionsWorkflowRunId) {
throw new Error(
`No workflow run id for variant analysis with id: ${variantAnalysis.id}`,
Expand All @@ -848,7 +866,17 @@ export class VariantAnalysisManager
this.app.logger,
"Cancelling variant analysis. This may take a while.",
);
await cancelVariantAnalysis(this.app.credentials, variantAnalysis);
try {
await cancelVariantAnalysis(this.app.credentials, variantAnalysis);
} catch (e) {
if (variantAnalysis.status === VariantAnalysisStatus.Canceling) {
await this.onVariantAnalysisUpdated({
Comment thread
charisk marked this conversation as resolved.
Outdated
...variantAnalysis,
status: VariantAnalysisStatus.InProgress,
});
Comment thread
charisk marked this conversation as resolved.
Outdated
}
throw e;
}
}

public async openVariantAnalysisLogs(variantAnalysisId: number) {
Expand Down
5 changes: 5 additions & 0 deletions extensions/ql-vscode/src/view/variant-analysis/VariantAnalysisActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ export const VariantAnalysisActions = ({
Stop query
</Button>
)}
{variantAnalysisStatus === VariantAnalysisStatus.Canceling && (
<Button appearance="secondary" disabled={true}>
Stopping query
</Button>
)}
</Container>
);
};
18 changes: 18 additions & 0 deletions extensions/ql-vscode/src/view/variant-analysis/__tests__/VariantAnalysisActions.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,24 @@ describe(VariantAnalysisActions.name, () => {
expect(onStopQueryClick).toHaveBeenCalledTimes(1);
});

it("renders the stopping query disabled button when canceling", async () => {
Comment thread
robertbrignull marked this conversation as resolved.
render({
variantAnalysisStatus: VariantAnalysisStatus.Canceling,
});

const button = screen.getByText("Stopping query");
expect(button).toBeInTheDocument();
expect(button.getElementsByTagName("input")[0]).toBeDisabled();
});

it("does not render a stop query button when canceling", async () => {
render({
variantAnalysisStatus: VariantAnalysisStatus.Canceling,
});

expect(screen.queryByText("Stop query")).not.toBeInTheDocument();
});

it("renders 3 buttons when in progress with results", async () => {
const { container } = render({
variantAnalysisStatus: VariantAnalysisStatus.InProgress,
Expand Down