Skip to content

compose plugin id - #14063

Merged
sleshchenko merged 7 commits into
eclipse-che:masterfrom
sparkoo:NPE-theia-command
Aug 15, 2019
Merged

compose plugin id#14063
sleshchenko merged 7 commits into
eclipse-che:masterfrom
sparkoo:NPE-theia-command

Conversation

@sparkoo

@sparkoo sparkoo commented Jul 30, 2019

Copy link
Copy Markdown
Member

Signed-off-by: Michal Vala mvala@redhat.com

What does this PR do?

Compose plugin id in ExtendedPluginFQN and ChePlugin when all parts are known. First caused NPE, second that task was not properly configured to one particular component, but was present in all components.

What issues does this PR fix or reference?

#13842

How to test

here's simple devfile
---
apiVersion: 1.0.0
metadata:
  name: task-in-theia-test
components:
  -
    type: dockerimage
    alias: golang
    image: quay.io/eclipse/che-golang-1.10:nightly
    memoryLimit: 64Mi
  - 
    alias: theia-editor
    reference: >-
      https://raw.githubusercontent.com/eclipse/che-plugin-registry/master/v3/plugins/eclipse/che-theia/7.0.0-next/meta.yaml
    type: cheEditor
commands:
  -
    name: golang task
    actions:
      -
        type: exec
        component: golang
        command: "echo hello from golang component && ls /"
  -
    name: theia task
    actions:
      -
        type: exec
        component: theia-editor
        command: "echo hello from theia component && ls /"

che-server image with PR changes quay.io/mvala/che-server:npefix

@musienko-maxim

Copy link
Copy Markdown
Contributor

Can one of the admins verify this PR?

@sparkoo

sparkoo commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

ci-build

@sleshchenko sleshchenko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general, looks good.
Please consider adding java doc to PluginFqn, ExtendedFqn and describe that getId() and other methods may return null and when.

Also, since getId() may return null, maybe it makes sense to add null check in invocation places, like this https://github.com/eclipse/che/blob/f601f1652795048641001387c1ef13192f38a224/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/convert/component/editor/EditorComponentToWorkspaceApplier.java#L102?

@sparkoo

sparkoo commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

@sleshchenko both PluginFQN and ExtendedPluginFQN are just simple entity classes, without any logic. Values are null when you set them null. I've wrote simple doc to the constructor.
about getId usage nullchecks, I think it would distract the PR and is out of scope. The chance of id being null is lower by this patch. If you think it is still good idea, I will do it as separate PR under same issue. I would probably need help to decide what to do if it is null at some places.

@sparkoo

sparkoo commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

there is still one remaining issue though. Tasks looks like properly assigned to components, but when I try to run one, task selection pops up:
runtask_issue
When I choose one, it properly runs.

@sleshchenko

Copy link
Copy Markdown
Member

there is still one remaining issue though. Tasks looks like properly assigned to components, but when I try to run one, task selection pops up:

@sparkoo Could you provide Runtime JSON?

@sparkoo

sparkoo commented Jul 31, 2019

Copy link
Copy Markdown
Member Author
Runtime JSON
"runtime": {
    "machines": {
      "theia-idecdb": {
        "attributes": {
          "memoryLimitBytes": "512000000",
          "memoryRequestBytes": "512000000",
          "source": "tool",
          "plugin": "eclipse/che-theia/next"
        },
        "servers": {
          "theia-dev": {
            "url": "http://servers0k6t8yp-theia-idecdb-server-3130.192.168.99.105.nip.io/",
            "attributes": {
              "internal": "false",
              "type": "ide-dev",
              "port": "3130",
              "discoverable": "false"
            },
            "status": "UNKNOWN"
          },
          "theia": {
            "url": "http://servers0k6t8yp-theia-idecdb-server-3100.192.168.99.105.nip.io/",
            "attributes": {
              "internal": "false",
              "port": "3100",
              "discoverable": "false",
              "cookiesAuthEnabled": "true",
              "type": "ide",
              "secure": "true"
            },
            "status": "RUNNING"
          },
          "theia-redirect-3": {
            "url": "http://servers0k6t8yp-theia-idecdb-server-13133.192.168.99.105.nip.io/",
            "attributes": {
              "internal": "false",
              "port": "13133",
              "discoverable": "false"
            },
            "status": "UNKNOWN"
          },
          "theia-redirect-2": {
            "url": "http://servers0k6t8yp-theia-idecdb-server-13132.192.168.99.105.nip.io/",
            "attributes": {
              "internal": "false",
              "port": "13132",
              "discoverable": "false"
            },
            "status": "UNKNOWN"
          },
          "theia-redirect-1": {
            "url": "http://servers0k6t8yp-theia-idecdb-server-13131.192.168.99.105.nip.io/",
            "attributes": {
              "internal": "false",
              "port": "13131",
              "discoverable": "false"
            },
            "status": "UNKNOWN"
          }
        },
        "status": "RUNNING"
      },
      "vscode-javain7": {
        "attributes": {
          "memoryLimitBytes": "1572864000",
          "memoryRequestBytes": "1572864000",
          "source": "tool",
          "plugin": "redhat/java/latest"
        },
        "status": "RUNNING"
      },
      "che-machine-execxhu": {
        "attributes": {
          "memoryLimitBytes": "134217728",
          "memoryRequestBytes": "134217728",
          "source": "tool",
          "plugin": "eclipse/che-machine-exec-plugin/0.0.1"
        },
        "servers": {
          "che-machine-exec": {
            "url": "ws://serverw6uxhkan-che-machine-execxhu-server-4444.192.168.99.105.nip.io/",
            "attributes": {
              "internal": "false",
              "type": "terminal",
              "port": "4444",
              "discoverable": "false"
            },
            "status": "UNKNOWN"
          }
        },
        "status": "RUNNING"
      },
      "mysql": {
        "attributes": {
          "memoryLimitBytes": "268435456",
          "memoryRequestBytes": "268435456",
          "source": "recipe"
        },
        "status": "RUNNING"
      },
      "tools": {
        "attributes": {
          "memoryLimitBytes": "734003200",
          "memoryRequestBytes": "734003200",
          "source": "recipe"
        },
        "servers": {
          "8080/tcp": {
            "url": "http://serveriaa5xtix-tools-server-8080.192.168.99.105.nip.io/",
            "attributes": {
              "port": "8080"
            },
            "status": "UNKNOWN"
          }
        },
        "status": "RUNNING"
      }
    },
    "activeEnv": "default",
    "commands": [
      {
        "commandLine": "mvn clean install",
        "name": "maven build",
        "attributes": {
          "componentAlias": "tools",
          "machineName": "tools",
          "workingDir": "${CHE_PROJECTS_ROOT}/web-java-spring-petclinic"
        },
        "type": "exec"
      },
      {
        "commandLine": "SPRING_DATASOURCE_URL=jdbc:mysql://db/petclinic SPRING_DATASOURCE_USERNAME=petclinic SPRING_DATASOURCE_PASSWORD=password java -jar -Dspring.profiles.active=mysql target/*.jar\n",
        "name": "run webapp",
        "attributes": {
          "componentAlias": "tools",
          "machineName": "tools",
          "workingDir": "${CHE_PROJECTS_ROOT}/web-java-spring-petclinic"
        },
        "type": "exec"
      },
      {
        "commandLine": "/opt/rh/rh-mysql57/root/usr/bin/mysql -u root < ${CHE_PROJECTS_ROOT}/web-java-spring-petclinic/src/main/resources/db/mysql/schema.sql &&\necho -e \"\\e[32mDone.\\e[0m Database petclinic was configured!\"\n",
        "name": "prepare database",
        "attributes": {
          "componentAlias": "mysql",
          "machineName": "mysql"
        },
        "type": "exec"
      },
      {
        "commandLine": "echo hello",
        "name": "testtest",
        "attributes": {
          "componentAlias": "theia-editor",
          "plugin": "eclipse/che-theia/next",
          "machineName": "theia-idecdb"
        },
        "type": "exec"
      }
    ],
    "machineToken": ""
  },

@sparkoo

sparkoo commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

ci-build

@sparkoo

sparkoo commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

ci-test

1 similar comment
@skabashnyuk

Copy link
Copy Markdown
Contributor

ci-test

@che-bot

che-bot commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:14063
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sparkoo

sparkoo commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

ci-test

@sparkoo
sparkoo marked this pull request as ready for review July 31, 2019 13:17
@sparkoo

sparkoo commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

I've updated PR to compose plugin id at it's creation time rather than in getter. I'd rather keep entity class clean as possible.

@che-bot

che-bot commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:14063
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Jul 31, 2019
@l0rd

l0rd commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

Do not merge to master, the related issue is added to milestone 7.1.0

@skabashnyuk skabashnyuk added this to the 7.1.0 milestone Jul 31, 2019
@sparkoo

sparkoo commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

Do not merge to master, the related issue is added to milestone 7.1.0

Do we have an estimate when 7.0.0 will happen? It would be unfortunate if we have many PRs in this "do not merge yet" state.

@l0rd

l0rd commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

@sparkoo we will do the release as soon as all blockers with milestone 7.0.0 will be fixed. There are only 4 PRs labelled do-not-merge (including this one) so I am not that worried. We can create a 7.1.0 branch and merge those PRs there if the number increase.

But otherwise we should stick to the process. For your info the issue was labelled 7.1.0 a few weeks ago whereas there are still 20+ issues labelled 7.0.0.

@che-bot

che-bot commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14063

@skabashnyuk

Copy link
Copy Markdown
Contributor

LGTM. Is there a way to add a unit test?

@che-bot

che-bot commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
…e doc

Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
@che-bot

che-bot commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@sleshchenko sleshchenko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sleshchenko
sleshchenko merged commit 45f42e9 into eclipse-che:master Aug 15, 2019
@dmytro-ndp

dmytro-ndp commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

@sleshchenko, @sparkoo: FYI:
Happy path tests has been failed because of another PR broke Happy path test execution.

@sparkoo
sparkoo deleted the NPE-theia-command branch August 15, 2019 13:48
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 15, 2019
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants