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

Revert task types updates #9373

Closed

Conversation

alvsan09
Copy link
Contributor

What it does

Reverts commits affecting the use of task type

The PR #9189 merged 5 commits to support the Tasks customExecution API,
one of the commits ("Fixed task type is modified") impacted how the task type is used in multiple areas of the task
system, impacting the functionality to configure tasks, running tasks, etc.

There are a couple of other commits trying to fix the broken scenarios which cascaded to other problems.
This PR brings back the functionality before the commit mentioned above.

How to test

  • Plugin provided tasks shall work (shell, process and even customExecution).
  • Global tasks, and manually provisioned task shall work

NOTE: Gradle tasks are not expected to be visible in the task view since that's the scenario which was addressed by the
reverted commit (a proper solution shall be discussed).

Review checklist

Reminder for reviewers

@tsmaeder
Copy link
Contributor

I think I have a better fix coming. Give me a couple of hours.

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

@@ -1911,18 +1911,21 @@ export class Task {
private updateDefinitionBasedOnExecution(): void {
if (this.taskExecution instanceof ProcessExecution) {
Object.assign(this.taskDefinition, {
type: 'process',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should ever update the type field: in Eclipse Che, we register a TaskRunnerfor the task type 'che'. Once the type field is overwritten, we can't get it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This update has been there for two years before the introduction of customExecution,
I am trying to understand how this has not caused problems for 'che' then ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thomas, I think @alvsan09 is right: I provided these changes 2 years ago, please see

Copy link
Contributor

Choose a reason for hiding this comment

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

@alvsan09 It's not a problem if you don't have an execution set in the task (which happens to be the case in Che), but otherwise, you'll never match the task type. So while the problem might not immediately manifest, it's still there.
Generally, I think #9377 offers a cleaner way forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tsmaeder, this PR would be a reset to base functionality in case a more complete solution is not available in time,
I will test and review #9377

@alvsan09
Copy link
Contributor Author

Replaced by: #9377

@alvsan09 alvsan09 closed this Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants