Filter workspace containers like tools and user's #12386

AndrienkoAleksandr · 2019-01-11T08:24:31Z

Referenced issue

#11804

Add ability filter workspace containers like 'user-container' and 'tool-container'.

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

...core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java

...core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java

l0rd · 2019-01-11T09:59:59Z

The final goal is to filter out containers where a user can open a terminal. I think we should make it explicit and instead of type: [user-container|tool-container] we should use terminal-available: [true|false]. Does that make sense?

sleshchenko · 2019-01-11T12:05:16Z

@l0rd It makes sense for me. Because plugin (tooling) may provide some CLI software in sidecar, like maven, git. And then it makes sense to propose user open terminal there.
But I also think that terminal-available: false should not deny terminal access but just hide such containers from a containers list by default. Terminal access to such containers may be useful while investigation plugins errors. WDYT?

garagatyi · 2019-01-11T12:06:50Z

The final goal is to filter out containers where a user can open a terminal. I think we should make it explicit and instead of type: [user-container|tool-container] we should use terminal-available: [true|false]. Does that make sense?

Why should we state that the terminal is NOT available in other containers? I think that there are cases when someone would want to have the terminal in those containers. At least Che/Theia contributors that found a bug while working with the project. We might skip those containers by default, but this doesn't mean we should say that the terminal is not available for them overall.

AndrienkoAleksandr · 2019-01-11T12:14:53Z

@l0rd My changes supposed to be used not only for terminal access but also for an ability for hiding tools containers on Theia Containers View that Stevan asks [1]
[1] eclipse-che/che-theia#13 (comment)
And it could be useful for che-theia tasks too.

l0rd · 2019-01-11T12:54:46Z

@sleshchenko yes it's just to filter them out from theia command palette. Maybe the attribute visible-in-terminal or enable-terminal would be better better? But anyway I think that we should avoid that common users get confused with containers where they should 99% of the time they will never open a terminal. For debugging purposes advanced contributors will oc rsh or k exec.

@garagatyi agreed. I see this PR as a first step (add the enable terminal attribute and make it true by default for users containers and false for plugins). A second PR should follow to make it possible to enable terminal for plugin containers. And as a third and final step we should implement the ability to disable terminal for some runtime containers.

@AndrienkoAleksandr I believe that the semantic is different and thus we need 2 distinct attributes. There will be situations where I don't want to enable a terminal but I want to see the container amongst the list and vice-versa. Think about plugins as well: jdt.ls sidecar for instance won't be a user container but the user will probably want to open a terminal there and we want to see it amongst the container list as well. Let's be explicit and use 2 different attributes (e.g. enable-terminal + view-in-container-list).

AndrienkoAleksandr · 2019-01-11T13:28:55Z

I guess make sense have two filter level:

First level: filter containers like 'user' and 'tooling'(current pull request).
Second level: filter only for tooling containers. Tool manifect yaml could contains some attribute, for example call it 'user-interactive'. If user-interactive = true than user could create task or could create terminal for this tool.

In the machine View we could use first filter level: display in the tree two items: user containers and tool containers nodes. User could expand each of them and will see list containers. By default 'user-containers' node is already open in the tree, 'tool containers' node is collapsed by default.
For terminal and task we could use both filters: create new terminal commands for all user containers and for tool containers marked like 'user-interactive'. What Do you think guys?

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

l0rd · 2019-01-11T17:26:09Z

@AndrienkoAleksandr the problem with the types tool and user is that in the devfile definition a user defined container is a tool (c.f. https://github.com/redhat-developer/devfile/blob/master/Devfile.yaml#L10). This PR wording doesn't conform to the devfile wording.

If you really don't want to use attributes names that reference explicitly the filters (containers for the terminal and for the viewer) and you want to keep the attribute type I would suggest you change the allowed values to something like 'plugin' and 'runtime' rather then tooling and user.

AndrienkoAleksandr · 2019-01-14T12:07:35Z

@l0rd Thanks for sharing Devfile model. I was not aware of it before.
@skabashnyuk @sleshchenko and I discussed a bit how it can works with Devfile. Now, Devfile is converted to Workspace Config model. plugins, editor tools are stored in WorkspaceConfig#attributes and kubernetes/openshift tool is stored in WorkspaceConfig#recipe. Then we could auto provision source machine attribute that would indicate where machine comes from. Possible values: recipe , plugin, system (for jwt-proxy machine).
@l0rd WDYT?

l0rd · 2019-01-14T19:44:23Z

Well Devfile is supposed to become the new way to define a workspace and the wording should not change much. WorkspaceConfig attributes instead may change names in next releases. I am not sure how long we will keep the name recipe for instance.

sleshchenko · 2019-01-15T07:18:54Z

@l0rd Maybe later when Devfile will be the new way to define a workspace we could use another attribute marker like tool type, then we would have plugin, editor, kubernetes, dockerimage, etc. and client would not show containers of plugin and editor tools.
Since this attribute marker is not stored anywhere it should not be a big deal to rework Che Server and UI to use new attribute marker later. WDYT?

l0rd

@AndrienkoAleksandr let's merge this PR in its current status. We don't need to discuss too much about it right now and we can rework if needed in the future.

We can even use the tool attribute because we plan to rename tools to components in the devfile.

…ontainers

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

sleshchenko

LGTM

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java

...in/java/org/eclipse/che/api/workspace/server/spi/environment/InternalEnvironmentFactory.java

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

sleshchenko

LGTM

Please take a look my minor comments

sleshchenko · 2019-01-28T08:12:50Z

...ipse/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisioner.java

-            Long.toString(memoryLimitLong));
+            Long.toString(memoryLimitLong),
+            CONTAINER_SOURCE_ATTRIBUTE,
+            TOOL_CONTAINER_SOURCE);


It would be nice to have the corresponding check in JwtProxyProvisionerTest

sleshchenko · 2019-01-28T08:14:02Z

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java

@@ -194,6 +196,7 @@ private void addSidecar(
            .build();

    InternalMachineConfig machineConfig = machineResolver.resolve();
+    machineConfig.getAttributes().put(CONTAINER_SOURCE_ATTRIBUTE, TOOL_CONTAINER_SOURCE);


It would be nice to have the corresponding test in KubernetesPluginsToolingApplierTest

sleshchenko · 2019-01-28T08:15:34Z

ci-test

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

che-bot · 2019-01-28T12:38:24Z

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

…ontainers

AndrienkoAleksandr · 2019-01-29T10:18:25Z

ci-test

che-bot · 2019-01-29T12:13:37Z

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

sleshchenko · 2019-01-29T12:38:16Z

@AndrienkoAleksandr ci-test job failed because there is file with broken format /home/codenvy/workspace/che-pullrequests-test-ocp/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/environment/InternalEnvironmentFactoryTest.java. Please execute mvn sortpom:sort license:format fmt:format

Signed-off-by: Anna Shumilova <ashumilo@redhat.com>

ashumilova · 2019-01-29T14:13:27Z

ci-test

che-bot · 2019-01-29T17:48:36Z

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

AndrienkoAleksandr · 2019-01-30T08:58:30Z

@eclipse/eclipse-che-qa Please verify the last test-report: #12386 (comment)

Filter workspace containers like tools and user's

a89aa69

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

AndrienkoAleksandr requested review from skabashnyuk, garagatyi and sleshchenko January 11, 2019 08:24

AndrienkoAleksandr mentioned this pull request Jan 11, 2019

Apply ability filter user containers from among others #11804

Closed

sleshchenko reviewed Jan 11, 2019

View reviewed changes

...core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java Outdated Show resolved Hide resolved

garagatyi reviewed Jan 11, 2019

View reviewed changes

Addressed changes.

1ea56cb

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 14, 2019

l0rd mentioned this pull request Jan 21, 2019

[Devfile] Support commands for Os/K8s tool #12479

Closed

l0rd approved these changes Jan 23, 2019

View reviewed changes

AndrienkoAleksandr added 2 commits January 24, 2019 11:51

Merge branch 'master' of github.com:eclipse/che into filterWorkspaceC…

6d6fbb6

…ontainers

Address proposed changes.

4d08037

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

AndrienkoAleksandr changed the title ~~[WIP] Filter workspace containers like tools and user's~~ Filter workspace containers like tools and user's Jan 25, 2019

sleshchenko approved these changes Jan 25, 2019

View reviewed changes

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java Outdated Show resolved Hide resolved

sleshchenko reviewed Jan 25, 2019

View reviewed changes

...in/java/org/eclipse/che/api/workspace/server/spi/environment/InternalEnvironmentFactory.java Outdated Show resolved Hide resolved

garagatyi suggested changes Jan 25, 2019

View reviewed changes

...in/java/org/eclipse/che/api/workspace/server/spi/environment/InternalEnvironmentFactory.java Outdated Show resolved Hide resolved

Fixed bug detect during review. Add more tests.

d153718

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

sleshchenko approved these changes Jan 28, 2019

View reviewed changes

Fix typo.

449528e

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

garagatyi approved these changes Jan 28, 2019

View reviewed changes

Merge branch 'master' of github.com:eclipse/che into filterWorkspaceC…

862b77a

…ontainers

Fix formatter

e70889b

Signed-off-by: Anna Shumilova <ashumilo@redhat.com>

musienko-maxim approved these changes Jan 30, 2019

View reviewed changes

AndrienkoAleksandr merged commit e0b4df0 into master Jan 30, 2019

AndrienkoAleksandr deleted the filterWorkspaceContainers branch January 30, 2019 12:11

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 Jan 30, 2019

che-bot added this to the 6.18.0 milestone Jan 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter workspace containers like tools and user's #12386

Filter workspace containers like tools and user's #12386

AndrienkoAleksandr commented Jan 11, 2019

l0rd commented Jan 11, 2019

sleshchenko commented Jan 11, 2019

garagatyi commented Jan 11, 2019

AndrienkoAleksandr commented Jan 11, 2019

l0rd commented Jan 11, 2019

AndrienkoAleksandr commented Jan 11, 2019 •

edited

Loading

l0rd commented Jan 11, 2019

AndrienkoAleksandr commented Jan 14, 2019

l0rd commented Jan 14, 2019

sleshchenko commented Jan 15, 2019

l0rd left a comment

sleshchenko left a comment

sleshchenko left a comment

sleshchenko Jan 28, 2019

sleshchenko Jan 28, 2019

sleshchenko commented Jan 28, 2019

che-bot commented Jan 28, 2019

AndrienkoAleksandr commented Jan 29, 2019

che-bot commented Jan 29, 2019

sleshchenko commented Jan 29, 2019

ashumilova commented Jan 29, 2019

che-bot commented Jan 29, 2019

AndrienkoAleksandr commented Jan 30, 2019

Filter workspace containers like tools and user's #12386

Filter workspace containers like tools and user's #12386

Conversation

AndrienkoAleksandr commented Jan 11, 2019

Referenced issue

l0rd commented Jan 11, 2019

sleshchenko commented Jan 11, 2019

garagatyi commented Jan 11, 2019

AndrienkoAleksandr commented Jan 11, 2019

l0rd commented Jan 11, 2019

AndrienkoAleksandr commented Jan 11, 2019 • edited Loading

l0rd commented Jan 11, 2019

AndrienkoAleksandr commented Jan 14, 2019

l0rd commented Jan 14, 2019

sleshchenko commented Jan 15, 2019

l0rd left a comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko Jan 28, 2019

Choose a reason for hiding this comment

sleshchenko Jan 28, 2019

Choose a reason for hiding this comment

sleshchenko commented Jan 28, 2019

che-bot commented Jan 28, 2019

AndrienkoAleksandr commented Jan 29, 2019

che-bot commented Jan 29, 2019

sleshchenko commented Jan 29, 2019

ashumilova commented Jan 29, 2019

che-bot commented Jan 29, 2019

AndrienkoAleksandr commented Jan 30, 2019

AndrienkoAleksandr commented Jan 11, 2019 •

edited

Loading