-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Content references in actions for editor specific action definitions. #13273
Conversation
Signed-off-by: Lukas Krejci <[email protected]>
ci-build |
Signed-off-by: Lukas Krejci <[email protected]>
@metlos can |
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.
Code looks good to me.
I have a question about
- name: run
actions:
- type: vscode-task
reference: ../ide-config/vs-code/tasks.json
tasks.json
may contain multiple tasks, right?
If yes, how client is supposed to handle it? Run all tasks when this command is invoked? Display different tasks as different commands?
...e-api-devfile/src/main/java/org/eclipse/che/api/devfile/server/convert/CommandConverter.java
Outdated
Show resolved
Hide resolved
@metlos a couple of questions raised my mind reading this PR description:
"While this is most probably subject to change" --> why? |
This is a copy&paste, I've removed it.
Mostly just wishful thinking :) It's just a matter of taste, but IMHO we're overloading the meaning of a command and actions with this too much (as witnessed by @sleshchenko 's comment #13273 (review) here and my objections on #13057). This was discussed already at the issue, but more importantly the biggest unsolved question is how to figure out in what container to run the process of a task. If we're not able to solve that within the current design, I think we will need to move to the file-based approach. |
sorry, I didn't notice the question. Yes, this should be possible, because it uses the same content resolution mechanism as the reference of the kubernetes components. |
The logic is supposed to be that the referenced "actions" (e.g. contents of tasks.json) are interpreted by the editor. So this command actually does not appear as a task to run at all. It is the contents that are imported into the editor. |
…nts of the referenced command to improve the UX. Signed-off-by: Lukas Krejci <[email protected]>
Signed-off-by: Lukas Krejci <[email protected]>
ci-build |
Signed-off-by: Lukas Krejci <[email protected]>
ci-test |
Results of automated E2E tests of Eclipse Che Multiuser on OCP: |
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.
Please consider adding this feature description to README.md
of devfile module. I'm OK with adding it in a separate PR.
LGTM
@@ -41,6 +41,15 @@ | |||
*/ | |||
String PLUGIN_ATTRIBUTE = "plugin"; | |||
|
|||
/** | |||
* An attribute of the command to store the original path to the file that contains the editor |
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.
I think it makes sense to reference class here to make it clear which command is referenced here, devfile or workspace config
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.
This is in the ws config Command
class and is therefore used for the ws config.
Good point, but I actually want to wait with this until we have a fully working solution on the UI side (the problem with the running of tasks in different containers is not solved yet). Depending on how things pan out on the UI side, we might need to re-consider the approach to this in the devfile. |
What does this PR do?
This is a POC implementation of #13057 with the devfile using the following schema:
While this is most probably subject to change, the contents of the referenced editor configuration is stored inside the workspace config as an attribute of a workspace command called
actionReferenceContent
. The workspace config commands (don't confuse those with the devfile commands!) have been modified to either have a commandline or content.Note though that as we move towards representing the workspace using just the devfile, this is subject to change.
For now, to play with this, consider the following example:
Devfile
is converted into the workspace like this:
What issues does this PR fix or reference?
#13057