Support Compound Tasks #6680

ShimonBenYair · 2019-12-03T13:33:24Z

Signed-off-by: Shimon Ben Yair shimon.ben.yair@sap.com

What it does

With changes in this pull request, Theia offers users to create compound tasks ( to create tasks that depends on other tasks).
The user can run the tasks ( that his tasks depends on) in sequence or in parallel.And after those tasks will end , the main task will start.
In addition the user can create a task that depends on a background task.

How to test

Extract this project on your file system : mytask.zip
run "npm install"
Check compound tasks in sequence order : change the "dependsOn" and "dependsOrder" properties of "task1" to the following ( as in the gif animate) :
"dependsOn": [
"subTask2",
"subTask3"
],
"dependsOrder": "sequence"

and run "task1" ==> it should first run "subTask2" and then "subTask3" and when they are finished, "task1" will start.

Check compound tasks in parallel order : change the "dependsOn" and "dependsOrder" properties of "task1" to the following ( as in the gif animate) :
"dependsOn": [
"subTask2",
"subTask3"
],
"dependsOrder": "parallel"

and run "task1" ==> it should run in parallel "subTask2" and "subTask3" and when they are finished,
"task1" will start.

Check compound tasks with background task : change the "dependsOn" and "dependsOrder" properties of "task1" to the following ( as in the gif animate) :
"dependsOn": [
"testBackgroundTask"
],
"dependsOrder": "sequence"

and run "task1" ==> it should run first the "testBackgroundTask" task and when the ends pattern matches ( it will not wait until the whole task is finished and exited) "task1" will start.

Check compound tasks with taskIdentifier : change the "dependsOn" and "dependsOrder" properties of "task1" to the following ( as in the gif animate) :
"dependsOn": [
{"type":"npm", "script":"runsample"}
],
"dependsOrder": "sequence"

(Please notify that in the package.json file , there is a script named "runsample").

and run "task1" ==> it should run the script "runsample" before "task1" will start.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines
CQ Approved: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21228

RomanNikitenko

@ShimonBenYair
thank you for your contribution!

I'm testing your changes and I can see two notifications when a task is terminated

Could you check this behavior?

packages/variable-resolver/src/browser/variable-resolver-service.ts

akosyakov · 2019-12-04T05:25:18Z

packages/debug/src/browser/debug-session-manager.ts

-            const signal = await this.taskService.getTerminateSignal(taskInfo.taskId);
-            if (signal !== undefined) {
-                return this.doPostTaskAction(`Task '${taskName}' terminated by signal ${signal}.`);
+        return Promise.race([this.taskService.getBackgroundTaskEndMatch(taskInfo.taskId), this.taskService.getExitCode(taskInfo.taskId)]).then(async result => {


it looks tricky to decide what is a result depending on the type.

Can we use Promis.all instead to have separate variables?

Also async/await is more preferable than using promise chaining.

it looks tricky to decide what is a result depending on the type.

fixed.done

Can we use Promis.all instead to have separate variables?

Also async/await is more preferable than using promise chaining.

I must use here promise.race since i need to handle the case of depending on background tasks, and background tasks can send a feedback that the task was "ended" before the task was "exited".

akosyakov · 2019-12-04T05:25:42Z

@elaihau please review the task extension

akosyakov · 2019-12-04T05:27:02Z

packages/task/src/browser/task-service.ts

+        }
+    }
+
+    // tslint:disable-next-line: no-any


parameter and return types should not use any, only if it json, please review the rest PR to have explicit types

It might be a json object.
was fixed by renaming the variable and a documentation was added to the method

akosyakov · 2019-12-04T07:34:51Z

packages/task/src/common/task-watcher.ts

@@ -42,6 +43,9 @@ export class TaskWatcher {
            },
            onDidProcessTaskOutput(event: TaskOutputProcessedEvent): void {
                outputProcessedEmitter.fire(event);
+            },
+            onBackgroundTaskActive(event: BackgroundTaskEndEvent): void {


What active means? Usually it means something what is focused in Theia.

coding guidelines for event naming: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#event_names

Alternatively we usually fine with naming like on[Noun]PastTenseVerb, i.e. onTaskCreated or onCreated.

Thanks for the comment.
was fixed and was change to 'onBackgroundTaskEnded'.

The server fires 'BackgroundTaskEndedEvent' when a background task matches its 'beginsPattern' and 'endsPattern' properties in order to notify the client (that the backround task is active).
e.g. if we have a main task that in its "dependsOn" array there is a task that for example runs tomcat ( so it is running in the background and doesn't seems to have exit code ) , then the root task will never start ...
So the solution is to have a problem matcher with a background property and when it matches the 'beginsPattern' and 'endsPattern' properties , we know that the task is active and we can move forward to the main task.

That's why i have added the 'onBackgroundTaskEnded'

packages/task/src/browser/task-schema-updater.ts

packages/task/src/browser/task-service.ts

packages/task/src/browser/taskNode.ts

vince-fugnitto

If you are going to already be updating the PR, I think another improvement would be to update the commit message. I think we can add a description of what the PR does and what it resolves.
Also if you add 'Fixes #5517', then the corresponding issue will automatically be closed when merged.

ShimonBenYair · 2019-12-04T16:12:38Z

@ShimonBenYair
thank you for your contribution!

I'm testing your changes and I can see two notifications when a task is terminated

Could you check this behavior?
@RomanNikitenko
thanks for finding this bug.
was fixed, please fetch and check again.

vince-fugnitto

It looks like the schemas for compound tasks are not supported at the moment (due to mandatory type and command properties).

An example is defined in the VS Code docs: (https://code.visualstudio.com/docs/editor/tasks#_compound-tasks)

{
  "version": "2.0.0",
  "tasks": [
    {
      "label": "Client Build",
      "command": "gulp",
      "args": ["build"],
      "options": {
        "cwd": "${workspaceRoot}/client"
      }
    },
    {
      "label": "Server Build",
      "command": "gulp",
      "args": ["build"],
      "options": {
        "cwd": "${workspaceRoot}/server"
      }
    },
    {
      "label": "Build",
      "dependsOn": ["Client Build", "Server Build"]
    }
  ]
}

ShimonBenYair · 2019-12-04T20:33:25Z

It looks like the schemas for compound tasks are not supported at the moment (due to mandatory type and command properties).

An example is defined in the VS Code docs: (https://code.visualstudio.com/docs/editor/tasks#_compound-tasks)

{
  "version": "2.0.0",
  "tasks": [
    {
      "label": "Client Build",
      "command": "gulp",
      "args": ["build"],
      "options": {
        "cwd": "${workspaceRoot}/client"
      }
    },
    {
      "label": "Server Build",
      "command": "gulp",
      "args": ["build"],
      "options": {
        "cwd": "${workspaceRoot}/server"
      }
    },
    {
      "label": "Build",
      "dependsOn": ["Client Build", "Server Build"]
    }
  ]
}

@vince-fugnitto
I don't understand , do i need to add support for this as part of my contribution ?
Because Theia currently doesn't support tasks with no type

vince-fugnitto · 2019-12-04T22:33:49Z

@vince-fugnitto
I don't understand , do i need to add support for this as part of my contribution ?
Because Theia currently doesn't support tasks with no type

I'm not sure, maybe others can chime in @eclipse-theia/task-extension.
I guess I'm fine with it not being part of the main contribution, it's just that I was testing with custom tasks (like as described in VS Code) and they are not supported. I think if we are actually going to support compound tasks we should think about supporting their schemas as described in the documentation.

vince-fugnitto

I noticed that your custom task1 task fails silently when @theia/vscode-builtin-npm is not included. I think that in this case and in other cases (where errors are thrown), a message should at least be displayed to end users and not fail silently.

Error:

  task ERROR The information provied in the "dependsOn" is not enough for matching the correct task !

packages/task/src/browser/task-service.ts

packages/task/src/browser/taskNode.ts

packages/variable-resolver/src/browser/variable-resolver-service.ts

akosyakov · 2019-12-05T08:21:11Z

It would be nice if @elaihau or @RomanNikitenko can check new control flow, data structures, changes to APIs and so on.

marcdumais-work · 2019-12-05T12:12:36Z

I'm not sure, maybe others can chime in @eclipse-theia/task-extension.
I guess I'm fine with it not being part of the main contribution, it's just that I was testing with custom tasks (like as described in VS Code) and they are not supported. I think if we are actually going to support compound tasks we should think about supporting their schemas as described in the documentation.

@elaihau WDYT?

vince-fugnitto · 2019-12-06T12:56:33Z

I'm not sure, maybe others can chime in @eclipse-theia/task-extension.
I guess I'm fine with it not being part of the main contribution, it's just that I was testing with custom tasks (like as described in VS Code) and they are not supported. I think if we are actually going to support compound tasks we should think about supporting their schemas as described in the documentation.

@elaihau WDYT?

After a bit of investigating, the way forward would be to:

add a default type: (shell), so that configurations may choose to exclude them
add logic so that either command or dependsOn must be defined (same as in VS Code) - this will allow us to support the basic configuration as present in VS Code

Error: the task 'test task' neither specifies a command nor a dependsOn property. The task will be ignored.

ShimonBenYair · 2019-12-07T16:45:32Z

add logic so that either command or dependsOn must be defined (same as in VS Code) - this will allow us to support the basic configuration as present in VS Code

@vince-fugnitto
where do i need to add this logic (in which file ) ?

elaihau

I will do more testing on the functionality later today.

elaihau · 2019-12-11T03:11:05Z

Given that "task 2" depends on "task 1", one thing I found after testing was, vs code proceeded with "task 2" after "task 1" was terminated by "CTRL+C".

This is different from what this pull request does. With this PR, "task 2" is not started if "task 1" is terminated by the user.

I prefer the implementation in this PR. What do you think? @marcdumais-work @akosyakov @vince-fugnitto

ShimonBenYair · 2019-12-11T09:26:12Z

@elaihau
Currently the "preLaunchTask" and "postDebugTask" are declared as string :
preLaunchTask?: string;
( https://github.com/eclipse-theia/theia/blob/master/packages/debug/src/common/debug-configuration.ts#L67 )
and not as :
preLaunchTask?: string | TaskIdentifier;
(like in VScode : https://github.com/microsoft/vscode/blob/cabcace4b5c5816c4715d6dca26edb0e6e826f8b/src/vs/workbench/contrib/debug/common/debug.ts#L511)
I want to change it, and i know what i need to do .
But do i need to make changes also to this file : https://github.com/eclipse-theia/theia/blob/master/packages/plugin-ext/src/hosted/node/scanners/scanner-theia.ts#L495 ?
Can you explain me what this file is responsible for ?

@tolusha is a better person to answer.
Anatolii, could you please take a quick look at this question when you get the chance ? Thank you !

@tolusha

same question, do i need to change also here : https://github.com/eclipse-theia/theia/blob/master/packages/debug/src/node/vscode/vscode-debug-adapter-contribution.ts#L171 ? what is this file responsible for ?

tolusha · 2019-12-11T10:44:35Z

@ShimonBenYair

https://github.com/eclipse-theia/theia/blob/master/packages/plugin-ext/src/hosted/node/scanners/scanner-theia.ts#L495
It Is responsible to update schema for debug configuration that comes from plugins.

https://github.com/eclipse-theia/theia/blob/master/packages/debug/src/node/vscode/vscode-debug-adapter-contribution.ts#L171
Actually the same but it is used for @theia/debug-nodejs and @theia/java-debug

Yes, we have to change those files.

vince-fugnitto · 2019-12-11T11:32:21Z

I prefer the implementation in this PR. What do you think? @marcdumais-work @akosyakov @vince-fugnitto

I think it is better as well. If a task is dependent on another successfully completing, I do not expect the subsequent task to execute if the first has been terminated. However, we should get the opinion of others. I'm not sure that this will break behaviour from plugins contributing such tasks.

ShimonBenYair · 2019-12-14T16:41:20Z

@elaihau

I have fixed everything we talked about.
Can you please approve my pull request so i would be able to merge it ?
Thanks
Shimon

vince-fugnitto

I have a few comments, mainly regarding typos and naming.

vince-fugnitto · 2019-12-15T22:05:23Z

packages/debug/src/browser/debug-session-manager.ts

-        const code = await this.taskService.getExitCode(taskInfo.taskId);
-        if (code === 0) {
+        const getExitCodePromise: Promise<TaskEndedInfo> = this.taskService.getExitCode(taskInfo.taskId).then(result =>
+            ({ taskEndedtype: TasksEndedTypes.TaskExited, value: result }));


[minor] please rename to taskEndedType following the guidelines.

fixed, done

vince-fugnitto · 2019-12-15T22:05:34Z

packages/debug/src/browser/debug-session-manager.ts

+        const getExitCodePromise: Promise<TaskEndedInfo> = this.taskService.getExitCode(taskInfo.taskId).then(result =>
+            ({ taskEndedtype: TasksEndedTypes.TaskExited, value: result }));
+        const isBackgroundTaskEndedPromise: Promise<TaskEndedInfo> = this.taskService.isBackgroundTaskEnded(taskInfo.taskId).then(result =>
+            ({ taskEndedtype: TasksEndedTypes.BackgroundTaskEnded, value: result }));


[minor] please rename to taskEndedType following the guidelines.

fixed, done

vince-fugnitto · 2019-12-15T22:06:02Z

packages/debug/src/browser/debug-session-manager.ts

+        const isBackgroundTaskEndedPromise: Promise<TaskEndedInfo> = this.taskService.isBackgroundTaskEnded(taskInfo.taskId).then(result =>
+            ({ taskEndedtype: TasksEndedTypes.BackgroundTaskEnded, value: result }));
+
+        // After start running the task, we wait for the task proccess to exit and if it is a background task, we also wait for a feedback


[minor] typo - proccess should be process.

fixed, done

vince-fugnitto · 2019-12-15T22:06:25Z

packages/debug/src/browser/debug-session-manager.ts

+        // that a background task is active, as soon as one of the promises fulfills, we can continue and analyze the results.
+        const taskEndedInfo: TaskEndedInfo = await Promise.race([getExitCodePromise, isBackgroundTaskEndedPromise]);
+
+        if (taskEndedInfo.taskEndedtype === TasksEndedTypes.BackgroundTaskEnded && taskEndedInfo.value) {


[minor] please rename to taskEndedType following the guidelines.

fixed, done

vince-fugnitto · 2019-12-15T22:06:57Z

packages/debug/src/browser/debug-session-manager.ts

            return true;
-        } else if (code !== undefined) {
-            return this.doPostTaskAction(`Task '${taskName}' terminated with exit code ${code}.`);
+        } else if (taskEndedInfo.taskEndedtype === TasksEndedTypes.TaskExited && taskEndedInfo.value !== undefined) {


[minor] please rename to taskEndedType following the guidelines.

fixed, done

vince-fugnitto · 2019-12-15T22:08:13Z

packages/task/src/browser/task-service.ts

+}
+
+export interface TaskEndedInfo {
+    taskEndedtype: TasksEndedTypes,


Please rename taskEndedtype to taskEndedType following the guidelines.

fixed, done

vince-fugnitto · 2019-12-15T22:08:59Z

packages/task/src/browser/task-service.ts

+            const isBackgroundTaskEndedPromise: Promise<TaskEndedInfo> = this.isBackgroundTaskEnded(taskInfo.taskId).then(result =>
+                ({ taskEndedtype: TasksEndedTypes.BackgroundTaskEnded, value: result }));
+
+            // After start running the task, we wait for the task proccess to exit and if it is a background task, we also wait for a feedback


[minor] typo - proccess should be process.

fixed, done

vince-fugnitto · 2019-12-15T22:09:33Z

packages/task/src/browser/task-service.ts

+        const notEnoughDataError = 'The information provided in the "dependsOn" is not enough for matching the correct task !';
+        let currentTaskChildConfiguration: TaskConfiguration;
+        if (typeof (taskIdentifier) !== 'string') {
+            // TaskIdentier object does not support tasks of type 'shell' (The same behavior as in VS Code).


[minor] typo - TaskIdentier should be TaskIndentifier.

fixed, done

vince-fugnitto · 2019-12-15T22:10:35Z

packages/task/src/browser/task-service.ts

+        if (typeof (taskIdentifier) !== 'string') {
+            // TaskIdentier object does not support tasks of type 'shell' (The same behavior as in VS Code).
+            // So if we want the 'dependsOn' property to include tasks of type 'shell',
+            // then we must mention their labels (in then'dependsOn' property) and not to create a task identifier object for them.


[minor] typo - should be the 'dependsOn' property

fixed, done

vince-fugnitto · 2019-12-15T22:15:44Z

packages/task/src/common/task-protocol.ts

+    dependsOn?: string | TaskIdentifier | Array<string | TaskIdentifier>;
+
+    /** The order the dependsOn tasks should be executed in. */
+    dependsOrder?: string;


Should we use the enum instead (ex: parallel | sequence) ?

fixed, done

packages/task/src/node/task-server.ts

elaihau · 2019-12-16T00:54:01Z

Also, could you please create a github issue as per @vince-fugnitto suggested here, and let us know if you are going to work on it? Thank you !

vince-fugnitto · 2019-12-16T11:48:03Z

@ShimonBenYair there is a difference when executing the task in VS Code and in Theia.
I see the output of both tasks in VS Code but not in Theia. Is this expected?

VS Code

> Executing task: npm run runsample <


> mytask@1.0.0 runsample /home/evinfug/Documents/mytask
> node HelloWorld.js

Compound!
Tasks!
is great !!!!

Terminal will be reused by tasks, press any key to close it.

> Executing task: echo root task started && sleep 5s && echo root task ended <

root task started
root task ended

Terminal will be reused by tasks, press any key to close it.

Theia: (only one terminal is ever spawned and its output displayed)

root task started
root task ended

ShimonBenYair · 2019-12-16T12:35:49Z

@ShimonBenYair there is a difference when executing the task in VS Code and in Theia.
I see the output of both tasks in VS Code but not in Theia. Is this expected?

VS Code
> Executing task: npm run runsample <


> mytask@1.0.0 runsample /home/evinfug/Documents/mytask
> node HelloWorld.js

Compound!
Tasks!
is great !!!!

Terminal will be reused by tasks, press any key to close it.

> Executing task: echo root task started && sleep 5s && echo root task ended <

root task started
root task ended

Terminal will be reused by tasks, press any key to close it.
Theia: (only one terminal is ever spawned and its output displayed)
root task started
root task ended

as you can see in the "How to test" section in step 6 , two terminals are opened, the first for the "runsample" (the task that task1 is depends on ) and the second terminal for the main task (task1).
please see the gif attached on step 6

vince-fugnitto · 2019-12-16T12:48:43Z

@ShimonBenYair there is a difference when executing the task in VS Code and in Theia.
I see the output of both tasks in VS Code but not in Theia. Is this expected?
VS Code
> Executing task: npm run runsample <


> mytask@1.0.0 runsample /home/evinfug/Documents/mytask
> node HelloWorld.js

Compound!
Tasks!
is great !!!!

Terminal will be reused by tasks, press any key to close it.

> Executing task: echo root task started && sleep 5s && echo root task ended <

root task started
root task ended

Terminal will be reused by tasks, press any key to close it.
Theia: (only one terminal is ever spawned and its output displayed)
root task started
root task ended
as you can see in the "How to test" section in step 6 , two terminals are opened, the first for the "runsample" (the task that task1 is depends on ) and the second terminal for the main task (task1).
please see the gif attached on step 6

I'm asking since only one terminal is ever opened. Please see the gif from the lastest changes:

ShimonBenYair · 2019-12-16T13:08:09Z

@ShimonBenYair there is a difference when executing the task in VS Code and in Theia.
I see the output of both tasks in VS Code but not in Theia. Is this expected?
VS Code
> Executing task: npm run runsample <


> mytask@1.0.0 runsample /home/evinfug/Documents/mytask
> node HelloWorld.js

Compound!
Tasks!
is great !!!!

Terminal will be reused by tasks, press any key to close it.

> Executing task: echo root task started && sleep 5s && echo root task ended <

root task started
root task ended

Terminal will be reused by tasks, press any key to close it.
Theia: (only one terminal is ever spawned and its output displayed)
root task started
root task ended
as you can see in the "How to test" section in step 6 , two terminals are opened, the first for the "runsample" (the task that task1 is depends on ) and the second terminal for the main task (task1).
please see the gif attached on step 6
I'm asking since only one terminal is ever opened. Please see the gif from the lastest changes:

@vince-fugnitto

Thanks a lot for finding this bug.
Thanks a lot !!!
my mistake, was fixed now.
please fetch and check again

ShimonBenYair · 2019-12-16T13:18:53Z

Also, could you please create a github issue as per @vince-fugnitto suggested here, and let us know if you are going to work on it? Thank you !

@elaihau @vince-fugnitto

I have created a github issue for it : #6757

I plan to work on it on January (I will notify in the issue when i start working on it)

packages/task/src/browser/task-schema-updater.ts

vince-fugnitto · 2019-12-16T14:01:34Z

@ShimonBenYair there is a difference when executing the task in VS Code and in Theia.
I see the output of both tasks in VS Code but not in Theia. Is this expected?
VS Code
> Executing task: npm run runsample <


> mytask@1.0.0 runsample /home/evinfug/Documents/mytask
> node HelloWorld.js

Compound!
Tasks!
is great !!!!

Terminal will be reused by tasks, press any key to close it.

> Executing task: echo root task started && sleep 5s && echo root task ended <

root task started
root task ended

Terminal will be reused by tasks, press any key to close it.
Theia: (only one terminal is ever spawned and its output displayed)
root task started
root task ended
as you can see in the "How to test" section in step 6 , two terminals are opened, the first for the "runsample" (the task that task1 is depends on ) and the second terminal for the main task (task1).

@vince-fugnitto

Thanks a lot for finding this bug.
Thanks a lot !!!
my mistake, was fixed now.
please fetch and check again

No problem :) it works a lot better now!

packages/task/src/node/task-server.ts

ShimonBenYair · 2019-12-16T20:45:53Z

@elaihau

I have fixed everything we talked about.
Can you please approve my pull request so i would be able to merge it ?
Thanks
Shimon

elaihau

Looks good to me.
@ShimonBenYair Would it be OK for you to hold this pull request until Dec 20, 2019 (GMT) ?
There will be a build on Dec 19. If your plan is not targeting the Dec 19 release, I guess it would be nice to have this feature merged after the release so that more contributors have opportunities to try it.
What do you think?

vince-fugnitto · 2019-12-17T17:34:46Z

packages/task/src/node/task-server.ts

+                        const backgroundTaskStatus = this.backgroundTaskStatusMap.get(event.taskId)!;
+                        if (!backgroundTaskStatus.isActive) {
+                            // Get the 'activeOnStart' value of the problem matcher 'background' property
+                            const activeOnStart = collector.problemMatchers[0]!.watching!.activeOnStart;


Are we certain that none of these properties will be undefined when isBackground is true?
Also, why do we only care about the first index?

fixed, done.Now it behaves exactly like VS Code.

vince-fugnitto · 2019-12-17T17:58:01Z

packages/task/src/browser/task-service.ts

+        }
+    }
+
+    getTaskConfigurationByTaskIdentifier(taskDefinition: TaskDefinition | undefined, taskIdentifier: TaskIdentifier, tasks: TaskConfiguration[]): TaskConfiguration {


It isn't clear to me what this method actually does (implementation) and I think it'd be worthwhile to properly document it. What are we trying to ultimately achieve? Also, the naming of the method is misleading since we must provide many parameters not only TaskIdentifier.

fixed, done

vince-fugnitto · 2019-12-17T18:17:59Z

packages/task/src/browser/task-service.ts

+    node: TaskNode;
+}
+
+export enum TasksEndedTypes {


Any reason we name it TasksEndedTypes and not TaskEndedTypes similarly to other interfaces?

fixed, done

vince-fugnitto · 2019-12-17T18:30:09Z

packages/task/src/browser/task-service.ts

+        if (typeof (taskIdentifier) !== 'string') {
+            // TaskIdentifier object does not support tasks of type 'shell' (The same behavior as in VS Code).
+            // So if we want the 'dependsOn' property to include tasks of type 'shell',
+            // then we must mention their labels (in the'dependsOn' property) and not to create a task identifier object for them.


typo - I don't think the typo has been addressed yet: (in the'dependsOn' property)

fixed, done

vince-fugnitto · 2019-12-17T19:41:41Z

Looks good to me.
@ShimonBenYair Would it be OK for you to hold this pull request until Dec 20, 2019 (GMT) ?
There will be a build on Dec 19. If your plan is not targeting the Dec 19 release, I guess it would be nice to have this feature merged after the release so that more contributors have opportunities to try it.
What do you think?

@elaihau
It is ok to me to merge it at Dec 20, 2019.
But just wonder , why i don't have the option technically now to merge the code even after you have approved this pull request ? Do i have to wait for more reviewers to approve ? ( the merge button is missing)

Only official committers of the project are able to commit, explicitly approve, etc.
In the future with more and more pull requests and contributions it will be possible to nominate you :)

and also Fixes #6534 Signed-off-by: Shimon Ben Yair <shimon.ben.yair@sap.com>

ShimonBenYair · 2019-12-18T13:26:05Z

Looks good to me.
@ShimonBenYair Would it be OK for you to hold this pull request until Dec 20, 2019 (GMT) ?
There will be a build on Dec 19. If your plan is not targeting the Dec 19 release, I guess it would be nice to have this feature merged after the release so that more contributors have opportunities to try it.
What do you think?

@elaihau

After consulting with my team we will appreciate if it will be part of Theia 14.
Because it is quite urgent for our scenario.

ShimonBenYair · 2020-03-27T05:09:26Z

Also, could you please create a github issue as per @vince-fugnitto suggested here, and let us know if you are going to work on it? Thank you !

@elaihau @vince-fugnitto

I have created a github issue for it : #6757

I plan to work on it on January (I will notify in the issue when i start working on it)

@elaihau @vince-fugnitto

I think that I will not implement it (due to change in the working priorities)

vince-fugnitto requested review from elaihau and tolusha December 3, 2019 13:55

vince-fugnitto added the tasks issues related to the task system label Dec 3, 2019

RomanNikitenko reviewed Dec 3, 2019

View reviewed changes

akosyakov added core issues related to the core of the application debug issues that related to debug functionality variable-resolver issues related to the variable-resolver extension and removed core issues related to the core of the application labels Dec 4, 2019

akosyakov reviewed Dec 4, 2019

View reviewed changes

packages/variable-resolver/src/browser/variable-resolver-service.ts Outdated Show resolved Hide resolved

akosyakov reviewed Dec 4, 2019

View reviewed changes

packages/variable-resolver/src/browser/variable-resolver-service.ts Outdated Show resolved Hide resolved

akosyakov reviewed Dec 4, 2019

View reviewed changes

vince-fugnitto reviewed Dec 4, 2019

View reviewed changes

packages/task/src/browser/task-schema-updater.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Dec 4, 2019

View reviewed changes

packages/task/src/browser/task-service.ts Show resolved Hide resolved

vince-fugnitto reviewed Dec 4, 2019

View reviewed changes

packages/task/src/browser/task-service.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Dec 4, 2019

View reviewed changes

packages/task/src/browser/taskNode.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Dec 4, 2019

View reviewed changes

akosyakov reviewed Dec 5, 2019

View reviewed changes

packages/variable-resolver/src/browser/variable-resolver-service.ts Show resolved Hide resolved

elaihau reviewed Dec 10, 2019

View reviewed changes

ShimonBenYair requested a review from elaihau December 12, 2019 12:30

vince-fugnitto reviewed Dec 15, 2019

View reviewed changes

elaihau reviewed Dec 16, 2019

View reviewed changes

packages/task/src/node/task-server.ts Show resolved Hide resolved

vince-fugnitto reviewed Dec 16, 2019

View reviewed changes

packages/task/src/browser/task-schema-updater.ts Show resolved Hide resolved

elaihau reviewed Dec 16, 2019

View reviewed changes

packages/task/src/node/task-server.ts Show resolved Hide resolved

ShimonBenYair requested review from RomanNikitenko and elaihau December 17, 2019 04:31

elaihau approved these changes Dec 17, 2019

View reviewed changes

vince-fugnitto reviewed Dec 17, 2019

View reviewed changes

Support Compound Tasks, Background Tasks and TaskIdentifier, Fixes #5517

ad80a05

and also Fixes #6534 Signed-off-by: Shimon Ben Yair <shimon.ben.yair@sap.com>

elaihau merged commit f54e970 into eclipse-theia:master Dec 18, 2019

azatsarynnyy mentioned this pull request Dec 20, 2019

Editors Sprint 177 eclipse-che/che#15531

Closed

8 tasks

vince-fugnitto mentioned this pull request Mar 26, 2020

[task] support tasks without 'type' and 'command' if 'dependsOn' property is provided #6757

Open

Support Compound Tasks #6680

Support Compound Tasks #6680

Conversation

ShimonBenYair commented Dec 3, 2019 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

akosyakov Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

ShimonBenYair Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

ShimonBenYair commented Dec 4, 2019 • edited Loading

vince-fugnitto left a comment

Choose a reason for hiding this comment

ShimonBenYair commented Dec 4, 2019 • edited Loading

vince-fugnitto commented Dec 4, 2019

vince-fugnitto left a comment

Choose a reason for hiding this comment

akosyakov commented Dec 5, 2019

marcdumais-work commented Dec 5, 2019

vince-fugnitto commented Dec 6, 2019

ShimonBenYair commented Dec 7, 2019

elaihau left a comment

Choose a reason for hiding this comment

elaihau commented Dec 11, 2019

ShimonBenYair commented Dec 11, 2019

tolusha commented Dec 11, 2019

vince-fugnitto commented Dec 11, 2019 • edited Loading

ShimonBenYair commented Dec 14, 2019

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elaihau commented Dec 16, 2019

vince-fugnitto commented Dec 16, 2019

ShimonBenYair commented Dec 16, 2019

vince-fugnitto commented Dec 16, 2019

ShimonBenYair commented Dec 16, 2019 • edited Loading

ShimonBenYair commented Dec 16, 2019

vince-fugnitto commented Dec 16, 2019

ShimonBenYair commented Dec 16, 2019

elaihau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimonBenYair Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimonBenYair commented Dec 3, 2019 •

edited by vince-fugnitto

Loading

RomanNikitenko left a comment •

edited

Loading

akosyakov Dec 4, 2019 •

edited

Loading

akosyakov Dec 4, 2019 •

edited

Loading

ShimonBenYair Dec 6, 2019 •

edited

Loading

ShimonBenYair commented Dec 4, 2019 •

edited

Loading

ShimonBenYair commented Dec 4, 2019 •

edited

Loading

vince-fugnitto commented Dec 11, 2019 •

edited

Loading

ShimonBenYair commented Dec 16, 2019 •

edited

Loading

ShimonBenYair Dec 18, 2019 •

edited

Loading