Skip to content
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

support watch task reconnection #155120

Merged
merged 13 commits into from
Jul 14, 2022
Merged

support watch task reconnection #155120

merged 13 commits into from
Jul 14, 2022

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Jul 13, 2022

This PR fixes #117408

To do/ might defer:

  • status doesn't update after reload even though errors / problems do

@meganrogge meganrogge requested a review from Tyriar July 13, 2022 23:43
@meganrogge meganrogge self-assigned this Jul 13, 2022
@meganrogge meganrogge added this to the July 2022 milestone Jul 13, 2022
@@ -167,6 +167,8 @@ export interface IPtyHostAttachTarget {
icon: TerminalIcon | undefined;
fixedDimensions: IFixedTerminalDimensions | undefined;
environmentVariableCollections: ISerializableEnvironmentVariableCollections | undefined;
reconnectionOwner?: string;
task?: { label: string; id: string; lastTask?: string; group?: string };
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a reconnection owner id or something and this data gets persisted by contrib/tasks? We don't want to mention tasks anywhere in terminal ideally

Copy link
Member

Choose a reason for hiding this comment

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

This?

reconnectionOwner: {
  ownerName: string;
  ownerId: number;
}

Copy link
Member

Choose a reason for hiding this comment

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

If that's not easily doable, a temporary fix is to give this a generic name with type unknown and validate the type in tasks. I'd be fine for that to live in terminal as it basically moves the tech debt out of terminal and into tasks.

@@ -347,7 +364,7 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer
return this.inTerminal();
}

private _registerCommands(): void {
private async _registerCommands(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Can revert this?

Tyriar
Tyriar previously approved these changes Jul 14, 2022
@meganrogge meganrogge merged commit e0a65a9 into main Jul 14, 2022
@meganrogge meganrogge deleted the merogge/task-2-reconnect branch July 14, 2022 01:16
andreamah pushed a commit that referenced this pull request Jul 14, 2022
jrieken pushed a commit that referenced this pull request Jul 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support reconnect in tasks
2 participants