-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement CustomExecution extension API #9189
Implement CustomExecution extension API #9189
Conversation
Hello @shyshywhy,
|
bd7555f
to
3fe0ad7
Compare
@azatsarynnyy Ok, I fixed it. |
packages/task/src/node/custom/custom-task-runner-backend-module.ts
Outdated
Show resolved
Hide resolved
I will have time for a full-blown review and verification tomorrow. Please bear with me. Thanks! |
I am verifying the PR with the Steps:
|
I fixed it, and fixed some problems found by my own test. |
Thank you so much, you work blazing fast. I cannot catch up with your pace... I added a few comments here and there, none of them are crucial. Can you please look into them? I am going to fetch your changes and keep on with the verification. I noticed an issue with the missing |
8bdeba3
to
98e9348
Compare
@kittaakos I fixed the above problem and merged it into the previous commit |
It does not really fix this issue. Although
|
I found an error when playing with the Gradle Tasks extension:
|
The PR looks very promising, @shyshywhy. If you need assistance with the bug-fixes, let me know, and I try to chime in into the development. Thank you so much for your help so far 🙏 |
For this issue, i have fixed, but not sure if the repair is appropriate. In the Task, updateDefinitionBasedOnExecution will modify the type of TaskDefinition, resulting in plugin judgment errors.
so, i change it, but it’s not clear if it conflicts with theia design |
Thank you for the update, @shyshywhy. I will approve your changes and leave the PR open for a few more days if someone else wants to chime in and try it in action. I have not used the
Overall, it's a great improvement. Thank you so much for your help 👍 There are a few issues I have noticed. We can fix them later:
|
Signed-off-by: Lewin Tan <[email protected]>
Signed-off-by: Lewin Tan <[email protected]>
80725c2
to
0d03021
Compare
ok,its done. |
@@ -89,7 +90,7 @@ export class TaskServerImpl implements TaskServer, Disposable { | |||
} | |||
|
|||
async run(taskConfiguration: TaskConfiguration, ctx?: string, option?: RunTaskOption): Promise<TaskInfo> { | |||
const runner = this.runnerRegistry.getRunner(taskConfiguration.type); | |||
const runner = this.runnerRegistry.getRunner(taskConfiguration.taskType); |
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.
@shyshywhy
could you share the reason why type
was replaced by taskType
?
The commit seems to have broken a lot of functionality from
If the issues are not resolved before the release I think we should revert as to not release a broken task system. |
@vince-fugnitto So what breakage is left? |
I see the following for the moment (possibly others): |
I fully agree with Vincent. It should be reverted according to our PR policy. As the PR originator hasn't responded for a week. |
I'm not able to take in progress any issue from the list above soon, so I'm +1 for reverting... |
unless someone else is going to fix all the breakages before the release |
On the other hand, we have almost two weeks before the release date...
|
I think we all agree this PR should not have been merged. The question now is what the way forward should be. Personally, I think we (probably me, since I already have a head start) should fix the issues, The PR was not huge and I think we could fix the issues in question within maybe 2 days. At least invest a couple of hours to see what the problem is before throwing out the feature. |
@tsmaeder since we have time before the release I think we can spend time to fix the regressions, and if ever we see that we run out of time, or there are too many regressions for the framework to be released we can think of reverting. |
Sounds like a plan! |
Great! |
The PR #9189 merged 5 commits to support the Tasks customExecution API, The good part is that removing that particular commit does not remove the support for customExecution, if we consider PR #9373 as a good base, we can then concentrate on fixing #8767. |
Fixes: #7185
Fixes: #8767
What it does
Implement CustomExecution extension API, and fixed issue: #7185, #8767
How to test
Use Gradle Tasks extension to test.
Review checklist
Reminder for reviewers