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

Implementation of local part of devfile feature #11832

Closed
mshaposhnik opened this issue Nov 5, 2018 · 17 comments
Closed

Implementation of local part of devfile feature #11832

mshaposhnik opened this issue Nov 5, 2018 · 17 comments
Assignees
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.

Comments

@mshaposhnik
Copy link
Contributor

mshaposhnik commented Nov 5, 2018

Implement local part of devfile feature (not related to remote repository fetching via git URL etc) which includes:

  • model files (jackson-annotated)
  • REST endpoints (create workspace from yaml, export workspace into yaml),
  • converters of devfile to/from workspace config
  • devfile valitator, validation exceptions;
    etc...
Path("devfile")
public class DefFileService extends Service  {

// generate a workspace by sending a devfile to a rest API
// Initially this method will return empty workspace configuration.
// And will start che-devfile-broker on a background to clone sources and get devfile.
@POST
@Consumes("text/yml")
@Produces(APPLICATION_JSON)
public Response createFromYaml(DevFile defFile){
...
}

Example of yaml functionality

---
specVersion: 0.0.1
name: petclinic-dev-environment
projects:
  - name: petclinic
    source:
      type: git
      location: 'git@github.com:spring-projects/spring-petclinic.git'
tools:
  - name: theia-ide
    definition:
      chePlugin:
        name: theia:0.0.3
  - name: jdt.ls
    definition:
      chePlugin:
        name: theia-jdtls:0.0.3
commands:
  - name: build
    toolsCommands:
      - tool: mvn-stack
        action:
          exec:
            command: mvn package
            workdir: /projects/spring-petclinic 
  - name: run
    toolsCommands:
      - tool: mysql
        action: start
      - tool: mvn-stack
        action:
          exec:
            command: mvn spring-boot:run
            workdir: /projects/spring-petclinic 
@benoitf
Copy link
Contributor

benoitf commented Nov 5, 2018

hello, do we already have complete examples of the new model ?

@mshaposhnik
Copy link
Contributor Author

@benoitf #11719 (comment)

@sleshchenko
Copy link
Member

@benoitf #11719 (comment)

@mshaposhnik I think it makes sense to duplicate plan what should be done in PR description or at least specify the comment in the description. Otherwise, DoD of the issue is not clear enough.

@mshaposhnik
Copy link
Contributor Author

@skabashnyuk repo_url is not related to this issue. And proposed schema is only a draft afaiu

@skabashnyuk
Copy link
Contributor

@mshaposhnik fixed

@mshaposhnik
Copy link
Contributor Author

Current Che7 command configuration is:

{ 
"commandLine": "mvn -f ${current.project.path} clean install", 
"name": "console-java-simple:build", 
"attributes": { 
          "goal": "Build", 
          "machineName": "...", 
          "previewUrl": "" 
          }
}

The good point that we can now specify the machine name on which the command will be run.
So as far as understand, devfile command with two nested tool commands like:

 name: run
    toolsCommands:
      - tool: mysql
        action: start
      - tool: mvn-stack
        action:
          exec:
            command: mvn spring-boot:run
            workdir: /projects/spring-petclinic 

should be mapped into workspace config as two different commands, with machine names somehow mapped from the tool field and command extracted from the command line. How to resolve workdir property is still unclear.

@mshaposhnik
Copy link
Contributor Author

mshaposhnik commented Nov 7, 2018

So the proposed both directions mapping:

Devfile WS Config
tool machineName attribute (If we have no such attribute when converting WS to devfile, put something like default)
commandLine command (including ${...} macroses)
workdir workingDir attribute if any (If we have no such attribute when converting WS to devfile, leave field empty, command will be executed on /home probably)

@skabashnyuk @l0rd is that seems ok for the beginning?

@sleshchenko
Copy link
Member

tool | machineName attribute (If we have no such attribute when converting WS to devfile, put something like default)

Why do not do tool as optional parameter, which could mean that user should choose machine (tool) where command should be run?

@garagatyi
Copy link

I wasn't involved into the discussion of the devfile functionality previously. Now I'm trying to understand how it should work and how to implement what is described here and don't understand that.
I discussed this devfile concept with @sleshchenko and it seems we are on the same page in not understanding how it would work :-)
Could we have a call with discussion of this or the idea is settled?
In particular, commands execution supposes that we know which tools bring a sidecar. But without plugin brokering process we can't figure out that. And plugin brokering process is also pluggable, so we don't know all the cases. For example in one of the cases we need to download tar archive and process files in it to know whether there are sidecars. In another use case, we need to download .theia file (zip archive), unpack it and analyze package.json to figure out whether it brings sidecar or not. In another case, plugin adds several sidecars. So, how all of that would be processed?
@skabashnyuk @l0rd could you clarify the flow in this issue since it seems that only you understand it at the moment.

Other points @sleshchenko and I came across during our discussion of dev file concept:

It doesn't mention recipe of the user environment, but shows that MySQL can be started from a compose file or dockerimage. Isn't this conflicts with the idea of adding tooling on top of a user production definition as we wanted to see in Che 7? Without any user recipe it looks like a new Che 6 feature. Should we have an action to run user recipe described in a file(OS templates or helm)?

Another thing we wanted to comment is that Che plugin is not equal to container since some of them do not bring any containers but just get integrated inTheia (or other IDE).

As to the currently described model we think that it can be improved with the follwing structure:

---
tools:
  - name: theia-ide
    definition:
      chePlugin:
        name: theia:0.0.3
  - name: jdt.ls
    definition:
      chePlugin:
        name: theia-jdtls:0.0.3
  - name: maven-sidecar
    definition:
      chePlugin:
        name: maven:3.5-jdk8
actions:
  - name: build 
    items:
      - tool: maven-sidecar
        action:
          type: exec
          command: mvn package
          workdir: /projects/spring-petclinic 
  - name: re-run
    items:
      - tool: maven-sidecar
        action:
          type: exec
          command: mvn spring-boot:stop
          workdir: /projects/spring-petclinic
      - tool: maven-sidecar
        action:
          type: exec
          command: mvn spring-boot:run
          workdir: /projects/spring-petclinic
          waitPrevious: code=1 # exit, seconds=20

Where the changes are:
We use field name actions instead of commands because there might be actions in a workspace other than exec in a container.
Do not use exec or other action type as a key because parsing would require checking whether there are all known action types as keys. Instead use type field for specifying action type.
Items of an action should be sorted in a order of execution since they can require a previous item to be called before. And this should be a part of the specification.
Add waitPrevious: in a case when we need to apply commands one by one and they depend on the result of the previous item. Conditions can be:

  • previous command exits
  • previous command exit code is equal to X
  • wait X seconds
  • execute another command to check state of the previous command (useful for non-stopping command)

We might want to postpone implementation of some of the features but would be great to think about the model structure at least. And having a spec or issue with all the described features would help to save the context and decisions implementation of which was postponed.

@l0rd
Copy link
Contributor

l0rd commented Nov 7, 2018

@mshaposhnik I think you have swapped commandLine (WS Config) and command (Devfile). And workdir should be optional (if it doesn't exist in the WS Config it should not be in the devfile neither). Besides that the mapping looks good.

@sleshchenko I see your point but it's not about making the tool name optional (the command should always be run inside a container) but making it possible to decide the tool name at runtime with a variable or prompting the user. So the author of a devfile should make it explicit:

 name: run
    toolsCommands:
      - tool: ${user.param1}
        action:
          exec:
            command: mvn spring-boot:run
            workdir: /projects/spring-petclinic 

@l0rd
Copy link
Contributor

l0rd commented Nov 7, 2018

@mshaposhnik @skabashnyuk given the feedbacks I would like to do some changes in the Devfile specification:

  1. use cheEditor instead of chePlugin for editors:
  - name: theia-ide
    definition:
      cheEditor:
        name: theia:0.0.3
  1. change toolsCommands to tools:
commands:
  - name: build
    tools:
      - name: mvn-stack
(...)
  1. use id instead of name for chePlugins and cheEditors and should have the format ${publisher-id}/${plugin-id}:${version} with respect to the plugin naming proposal Naming convention che-plugin-registry#55:
  - name: theia-ide
    definition:
      cheEditor:
        id: eclipse/theia:0.0.3

@l0rd
Copy link
Contributor

l0rd commented Nov 7, 2018

@garagatyi I somehow missed your (quite long and difficult to miss :-)) comment.

Anyway I like your changes for the model. I will reflect them in https://github.com/l0rd/devfile/ and ask your review from @skabashnyuk and @mshaposhnik for approval. I will also look at moving the devfile repository in the redhat-developer organisation.

For your concerns at the beginning of your comment:

  • plugin brokering: I probably don't understand your concern but, even if the models are not the same, a plugin in a che 7 ws config and in a devfile is defined in the same way (e.g. theia:0.0.3). There is not notion of broker in a che 7 workspace config and there is no notion of broker in a devfile. The extensibility is in the che plugin model and the devfile doesn't go into the details of che plugins and editors.
  • recipe of the user environment: Che 7 is about not injecting/installing tools in a user runtime application, it's about keeping tools in separate containers. As far as a user can define his development containers (call them runtimes, plugins or tools) and we don't make weird thing with it: that's Che 7. Now we could split tools in 2 distinct types: user runtime applications and development tools. We could also make more distinctions: build tools, test tools, security tools. But I felt that making these distinction in the devfile spec would have made it more complicated without any real benefit for users: does calling mysql a user runtime application instead of a tool make it easier to read the devfile? And to write it? I don't think so. There may be situation where we will need to distinguish among different tools types but I would rather use some more flexible labels (e.g. this what attributes in the devfile spec was ment for) instead of hardcoding the tools types in the spec.
  • Che plugin is not equal to container: right, and the devfile spec should not assume that a plugin is a container.

Let's do a call if you think there are still some aspects that need to be clarified.

@skabashnyuk skabashnyuk added kind/task Internal things, technical debt, and to-do tasks to be performed. team/platform sprint/current labels Nov 8, 2018
@garagatyi
Copy link

@l0rd let me explain our concerns about plugin brokering.
Devfile supposes someone would write there which command can be executed with which tool. Some che plugins would not bring sidecars, some of them would, some would bring a couple of them probably. And only plugin brokering process knows which plugins bring what amount of sidecars (0, 1, 2).
So, it is not clear who and how would write this dev file with the respect of described flow.
Would the user have to compose that file by trying out all of that on each restart of his dev environment?

As for the recipe. We saw that it is possible to configure something in your devfile repo, but this is not part of the description of this issue. This is our concern. Is the plan to add recipe related stuff to the spec later or just implement it later? What should we treat as a reference spec - issues from the platform team or your repo?

@garagatyi
Copy link

To summarize: the biggest concern, probably, is how this dev file would be written and used by an end user.

@l0rd
Copy link
Contributor

l0rd commented Nov 8, 2018

@garagatyi if I understand correctly you are worried about users that add command for tools that are not sidecars. I would not worry about that for now:

  1. 90% of the time users will create commands that target their runtime container rather then a plugin
  2. In 10% of the cases where the user will want to target a local theia plugins (not a sidecar) the command will be run in the theia container (or in whatever container will host the plugin).

@l0rd
Copy link
Contributor

l0rd commented Nov 8, 2018

@mshaposhnik @skabashnyuk @benoitf @garagatyi I have opened this PR to change the spec https://github.com/l0rd/devfile/pull/1/commits. Please have a look.

The petclinic example has become:

---
version: 0.0.1
name: petclinic-dev-environment
projects:
  - name: petclinic
    source:
      type: git
      location: 'git@github.com:spring-projects/spring-petclinic.git'
tools:
  - name: mvn-stack
    type: dockerImage
    image: maven:3.5.4-jdk-8
    entryPoint: sleep infinity
    persistentVolumes:
      - name: maven-repo
        containerPath: /root/.m2
    services:
      - name: spring-boot
        port: 8080
    mountProjectsSources: true
  - name: mysql
    type: composeFile
    local: docker-compose.yml
  - name: theia-ide
    type: cheEditor
    id: eclipse/theia:0.0.3
  - name: jdt.ls
    type: chePlugin
    id: eclipse/theia-jdtls:0.0.3
commands:
  - name: build
    actions:
      - type: exec
        tool: mvn-stack
        command: mvn package
        workdir: /projects/spring-petclinic
  - name: run
    attributes:
      runType: sequential
    actions:
      - type: start
        tool: mysql
        timeout: 20s
      - type: exec
        tool: mvn-stack
        command: mvn spring-boot:run
        workdir: /projects/spring-petclinic

Once the PR is merged I will move the repository to redhat-developer.

@mshaposhnik something that you may do as part of this issue is to create a json-schema for the devfile and add it to the devfile github repo. It's a well known standard for defining data formats. It will help with the discussion and allows to generate doc and API source code too.

@sleshchenko
Copy link
Member

@l0rd @skabashnyuk @mshaposhnik
FYI I've created a separate issue for creating JSON-schema for Devfile #11904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

No branches or pull requests

6 participants