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

Add previewURL support in Devfile format #13945

Closed
7 tasks done
l0rd opened this issue Jul 22, 2019 · 47 comments
Closed
7 tasks done

Add previewURL support in Devfile format #13945

l0rd opened this issue Jul 22, 2019 · 47 comments
Assignees
Labels
kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. severity/P1 Has a major impact to usage or development of the system.

Comments

@l0rd
Copy link
Contributor

l0rd commented Jul 22, 2019

Is your enhancement related to a problem? Please describe.

Extend current devfile spec to be allow users to specify a previewURL for a given command

Describe the solution you'd like

 commands:
   - name: run
     previewUrl:
       port: <a port>
       path: <a subpath>
     actions:
       - type: exec
         component: mysql
         command: mvn clean
         workdir: /projects/spring-petclinic

Implementation

@l0rd l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Jul 22, 2019
@l0rd l0rd modified the milestones: 7.x, 7.1.0 Jul 22, 2019
@l0rd l0rd added team/platform severity/P1 Has a major impact to usage or development of the system. labels Jul 22, 2019
@l0rd l0rd mentioned this issue Jul 23, 2019
2 tasks
@skabashnyuk
Copy link
Contributor

@l0rd Does the scope of this issue limited to add previewUrl only in devfile. Or it also means to add some information in workspace runtime. Like

command
    attributes
        previewUrl:https://host:port/path

@skabashnyuk skabashnyuk modified the milestones: 7.1.0, 7.2.0 Aug 23, 2019
@skabashnyuk skabashnyuk added the status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. label Aug 27, 2019
@skabashnyuk
Copy link
Contributor

@l0rd any comments?

@gorkem
Copy link
Contributor

gorkem commented Aug 28, 2019

How would ports-plugin utilize this information?

@skabashnyuk skabashnyuk removed the status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. label Aug 29, 2019
@skabashnyuk
Copy link
Contributor

The idea is to add a new attribute(look like an old Che6 commands attribute) in commands in runtime previewUrl
завантаження
Then task-plugin can read it https://github.com/eclipse/che-theia/blob/master/plugins/task-plugin/src/task/task-protocol.ts#L15

@skabashnyuk skabashnyuk modified the milestones: 7.2.0, 7.3.0 Sep 5, 2019
@sparkoo sparkoo self-assigned this Sep 17, 2019
@sparkoo
Copy link
Member

sparkoo commented Sep 26, 2019

@l0rd is command executed always on one of existing servers? Or can it start new one?

@sparkoo
Copy link
Member

sparkoo commented Sep 26, 2019

There's an issue how to find correct server with URL.

There are already exposed URL's in object Runtime>Machines>Server>Url. We can select machine by machineName attribute from command object. As @sleshchenko pointed out, it is not ensured that command will have machineName. Next issue is how to select Server. It should be filterable by port.

Another approach would be to create an extra route for previewUrl. I'm going to investigate this option.

@sparkoo
Copy link
Member

sparkoo commented Sep 26, 2019

@l0rd @skabashnyuk are both port and path mandatory? I could imagine we can use /as default path and 80 as default port. But do we want to do that?

@sleshchenko
Copy link
Member

are both port and path mandatory? I could imagine we can use /as default path and 80 as default port. But do we want to do that?

I agree that path may be optional with / as default value.
But port - it's not a port that clients should use to access an application, but port of a container that should be exposed. For java default is 8080, for nodejs like 4040, I think it would be better to require this value to be set.

@skabashnyuk
Copy link
Contributor

skabashnyuk commented Sep 26, 2019

There are already exposed URL's in object Runtime>Machines>Server>Url. We can select machine by machineName attribute from command object. As @sleshchenko pointed out, it is not ensured that command will have machineName. Next issue is how to select Server. It should be filterable by port.

That is all internal logic of che server. That should not be our focus.

I think that port is mandatory. It can be already declared as an endpoint in devfile https://github.com/eclipse/che-devfile-registry/blob/master/devfiles/java-web-spring/devfile.yaml#L29 or editor https://github.com/eclipse/che-plugin-registry/blob/d9998222c284d40ae0d2e1661452c1bcd5e4fddb/v3/plugins/ws-skeleton/eclipseide/4.9.0/meta.yaml#L17 or plugin, or it can be a command that can start a new server.

@sparkoo
Copy link
Member

sparkoo commented Oct 14, 2019

there is Theia issue that affects the previewUrl feature #14876

@skabashnyuk
Copy link
Contributor

As for the beta feature, I think @sparkos' s solution #13945 (comment) - is ok-ish.
I think one of the trickiest questions before we promote this feature to stable would be - the algorithm of how we proceed if user editing both devfile and task.json at the same time in the same place. And this issue #14876 is one of the sign
of the potential problem of having two sources of commands devfile and task.json.

A couple of question to @sparkos' s solution

  • How this id ${previewUrl1} would be calculated. Does it uniquely identify command? CRC?
  • @RomanNikitenko what do you do if you didn't find this id after workspace restart. The reason: the user just deletes it in devfile.

@sparkoo
Copy link
Member

sparkoo commented Oct 14, 2019

* How this id ${previewUrl1} would be calculated.  Does it uniquely identify command? CRC?

When #14876 is fixed, we don't have to do any sophisticated calculations. At workspace startup, we basically find URL for command (either it exists by defined endpoint or we create service+route), put it into the map under some key and put this key to command's attribute. Theia then receives this map and command's attributes and must properly update tasks.json with the key received from server. For consistency and human readability I would use something like name.replace(" ", "_")+random_noise() as a key (random noise to fix conflicting names, if someone has run command and run_command in the same devfile).

@sparkoo
Copy link
Member

sparkoo commented Oct 14, 2019

I've just realized that having random noise in key would mean that this key will change at each workspace startup, which is not ok for Theia. We should change tasks.json config only when we really have to. So yes, having some command.name + "-" + CRC(command.name), to have same key for the same name everytime should be enough.

@sparkoo
Copy link
Member

sparkoo commented Oct 14, 2019

we even don't need the tasks.json <> devfile sync to be fixed. Preview url key should be the same for the command name, so we're good here.

@skabashnyuk
Copy link
Contributor

we even don't need the tasks.json <> devfile sync to be fixed. Preview url key should be the same for the command name, so we're good here.

Sounds ok.

@sparkoo
Copy link
Member

sparkoo commented Oct 15, 2019

to sum up the proposed solution:

server will create workspace object like this:

...
    "links": {
      "self": "http://che-eclipse-che.192.168.42.10.nip.io/api/workspace/workspace7grc58gn0jozwkmz",
      "ide": "http://che-eclipse-che.192.168.42.10.nip.io/admin/nodejs-mongo-ymkmt",
      "environment/statusChannel": "ws://che-eclipse-che.192.168.42.10.nip.io/api/websocket",
      "environment/outputChannel": "ws://che-eclipse-che.192.168.42.10.nip.io/api/websocket",
      "previewurl/runthewebapp_123": "http://ingress111aaa.192.168.42.10.nip.io/hello",
      "previewurl/createtestuser_456": "http://ingress222bbb.192.168.42.10.nip.io/some/path"
    },
...
      "commands": [
        {
          "commandLine": "npm install && npm run dev",
          "name": "run the web app",
          "attributes": {
            "componentAlias": "nodejs",
            "machineName": "nodejs",
            "workingDir": "${CHE_PROJECTS_ROOT}/nodejs-mongodb-app"
          },
          "type": "exec",
          "previewUrl": "${previewurl/runthewebapp_123}"
        },
        {
          "commandLine": "curl -X POST --data '{\"user\": {\"username\": \"test\", \"email\": \"test@test.com\", \"password\": \"password\"}}' -H \"Content-Type: application/json\" http://localhost:3000/api/users\n",
          "name": "create test user",
          "attributes": {
            "componentAlias": "nodejs",
            "machineName": "nodejs"
          },
          "type": "exec",
          "previewUrl": "${previewurl/createtestuser_456}"
        }
      ],
...

Theia's responsibility is then to resolve variables in previewUrl attributes, with values from links object.

Key previewurl/run_the_web_app_123 is constructed in this format previewurl/<command_name>_<crc>, where previewurl is hard prefix, <command_name> is command's name without spaces and <crc> is some simple crc hash function applied on commands name. Theia do care only about prefix to know that this is previewurl link and it should create a variable from it. The rest is simple string without any extra meaning for Theia.

@sparkoo
Copy link
Member

sparkoo commented Oct 15, 2019

cc: @l0rd @RomanNikitenko @skabashnyuk

@skabashnyuk
Copy link
Contributor

#13945 (comment)

ok for me

@RomanNikitenko
Copy link
Member

@sparkoo

After restarting workspace key previewurl/createtestuser_456 will be the same previewurl/createtestuser_456? I mean crc hash _456 is not changed, right?

@sparkoo
Copy link
Member

sparkoo commented Oct 15, 2019

@sparkoo

After restarting workspace key previewurl/createtestuser_456 will be the same previewurl/createtestuser_456? I mean crc hash _456 is not changed, right?

yes, it will be calculated from command name, so as long as the name won't change, crc will be the same.

@sparkoo
Copy link
Member

sparkoo commented Oct 15, 2019

I've created an issue for Theia support of previewurl by proposed solution #14892

cc: @RomanNikitenko @l0rd

@skabashnyuk skabashnyuk added kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. team/ide and removed kind/enhancement A feature request - must adhere to the feature request template. labels Oct 15, 2019
@skabashnyuk skabashnyuk modified the milestones: 7.3.0, Backlog - Epics Oct 15, 2019
@l0rd
Copy link
Contributor Author

l0rd commented Oct 15, 2019

@sparkoo that's fine for me

@slemeur
Copy link
Contributor

slemeur commented Dec 5, 2019

When are you planning to merge this work to stable?

@sparkoo
Copy link
Member

sparkoo commented Dec 5, 2019

@slemeur we can plan it for next sprint, or even include it in current sprint. It should be straightforward patch.

@sparkoo
Copy link
Member

sparkoo commented Dec 5, 2019

@slemeur I've created an issue for that #15412

@sparkoo
Copy link
Member

sparkoo commented Jan 9, 2020

all issues in the scope of this epic are done. closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

9 participants