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

Tasks and Che Commands should not use "machineName" #13667

Closed
slemeur opened this issue Jul 2, 2019 · 11 comments
Closed

Tasks and Che Commands should not use "machineName" #13667

slemeur opened this issue Jul 2, 2019 · 11 comments
Assignees
Labels
kind/bug Outline of a bug - must adhere to the bug report template. severity/P2 Has a minor but important impact to the usage or development of the system.
Milestone

Comments

@slemeur
Copy link
Contributor

slemeur commented Jul 2, 2019

Description

"Machine" has been deprecated and should not be used anywhere in Che anymore.

{
            "label": "run the web app 2",
            "source": "che",
            "type": "che",
            "command": "cd ${CHE_PROJECTS_ROOT}/nodejs-web-app/app && nodemon app.js",
            "target": {
                "machineName": "nodejs"
            }
}

Capture d’écran 2019-07-02 à 11 51 20

We still have it for tasks, so it has to be adapted before the Che 7 GA in order to introduce a formalism which will need to be backward compatible after

@slemeur slemeur added kind/bug Outline of a bug - must adhere to the bug report template. severity/blocker Causes system to crash and be non-recoverable or prevents Che developers from working on Che code. team/ide labels Jul 2, 2019
@RomanNikitenko
Copy link
Member

@slemeur
Could you provide the new field name we should use instead of machineName?

If I understand correctly at least for 3 places changes are required:

  • task plugin
  • machine exec
  • code which serves devfile

thank you!

@slemeur
Copy link
Contributor Author

slemeur commented Jul 8, 2019

Could it just be made consistent with the formalism used in devfile?

@skabashnyuk
Copy link
Contributor

Could it just be made consistent with the formalism used in devfile?

That’s hard. Since devfile are not representing running workspace. In this case, machineName is the name of the running container. For workspace runtime, we still use Che6 like objects and concepts.

@l0rd
Copy link
Contributor

l0rd commented Jul 8, 2019

@skabashnyuk in devfile we use the components alias for that purpose. Not sure why it can't apply it here. And workspace runtime using Che6 objects and concepts should not be taken as a example for new, che 7 only, objects (i.e. che-theia tasks).

@l0rd l0rd added severity/P1 Has a major impact to usage or development of the system. and removed severity/blocker Causes system to crash and be non-recoverable or prevents Che developers from working on Che code. target/che7GA labels Jul 8, 2019
@l0rd l0rd added this to the 7.0.0 milestone Jul 8, 2019
@RomanNikitenko
Copy link
Member

RomanNikitenko commented Jul 8, 2019

At the moment task plugin gets info about target container as attribute of command machineName and provides this one for machine exec. So task-plugin, machine exec and che objects use term machineName.

I guess we can:

  • display for user that field as component or containerName for example, but in code still use machineName - only for task-plugin changes are required

OR

  • clean up code and use the new field for user and in our code as well - for task-plugin, machine exec and for che objects changes are required

@l0rd
Copy link
Contributor

l0rd commented Jul 8, 2019

@RomanNikitenko I think we should do that in 2 steps. First step for 7.0.0 and second step for 7.1.0

@l0rd l0rd mentioned this issue Jul 16, 2019
85 tasks
@RomanNikitenko
Copy link
Member

RomanNikitenko commented Jul 18, 2019

OK for me to do in 2 steps.

But what field we should use instead of machineName?
component? containerName?

@l0rd @slemeur @skabashnyuk

@slemeur
Copy link
Contributor Author

slemeur commented Jul 19, 2019

Let's be consistent with the devfile formalism please.
https://redhat-developer.github.io/devfile/devfile#components

@skabashnyuk
Copy link
Contributor

@slemeur as I remember yesterday on a meeting with you and @l0rd we decided to use "containerName" . The motivation was a workspace panel that is referencing to containers.

@RomanNikitenko RomanNikitenko added the status/in-progress This issue has been taken by an engineer and is under active development. label Jul 22, 2019
@l0rd l0rd added severity/P2 Has a minor but important impact to the usage or development of the system. and removed severity/P1 Has a major impact to usage or development of the system. labels Jul 23, 2019
@RomanNikitenko RomanNikitenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Jul 23, 2019
@RomanNikitenko
Copy link
Member

I provided the PR and I use the containerName instead of machineName field for che tasks.

@RomanNikitenko RomanNikitenko removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 31, 2019
@RomanNikitenko
Copy link
Member

The PR for first step was merged to 7.0.0 branch.
For second step I created another issue, which affect at least task-plugin, machine exec and che objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template. severity/P2 Has a minor but important impact to the usage or development of the system.
Projects
None yet
Development

No branches or pull requests

5 participants