search

eclipse-che / che-theia

Eclipse Public License 2.0
124 stars 110 forks source link

feat(terminal): Filter the containers list at a terminal creation #1318

Closed RomanNikitenko closed 2 years ago

RomanNikitenko commented 2 years ago

What does this PR do?

Screenshot 2022-02-17 at 17 50 08

Screenshot 2022-02-17 at 17 53 04

Screenshot 2022-02-17 at 18 02 33

There are few ways to create a terminal in a Developer container:

There are few ways to create a terminal in a Tooling container:

Screenshot/screencast of this PR

There are no dev containers - a terminal should be created in the editor container, don't ask user which container should be used

https://user-images.githubusercontent.com/5676062/154535958-0a3c35e5-4fc2-457d-b560-4da7723ba779.mp4

There is only one dev container - a terminal should be created in the container, don't ask user which container should be used

https://user-images.githubusercontent.com/5676062/154539637-477b3bfd-25b1-4083-8258-3eb827fe611e.mp4

There are few Dev containers

https://user-images.githubusercontent.com/5676062/154542904-e5305db6-66cf-4747-8974-77b4aedd4403.mp4

What issues does this PR fix or reference?

https://github.com/eclipse/che/issues/21117

How to test this PR?

You can use my images for testing, just add to your instance the following: #https://github.com/che-samples/java-spring-petclinic/tree/devfilev2?che-editor=https://raw.githubusercontent.com/RomanNikitenko/che-editor-test/main/.che/che-editor.yaml, like: https://che-dogfooding.apps.che-dev.x6e0.p1.openshiftapps.com/#https://github.com/che-samples/java-spring-petclinic/tree/devfilev2?che-editor=https://raw.githubusercontent.com/RomanNikitenko/che-editor-test/main/.che/che-editor.yaml

  1. A workspace has few Developer containers and few Tooling containers:

    • New Terminal command should display list of Developer containers and Show All Containers item
    • click on Show All Containers item should display all containers
    • New Terminal in specific container command should display all containers

  2. A workspace has only one Developer container:

    • New Terminal command should create a terminal in the Developer container and don't ask user about containers (don't display the list of containers)

  3. A workspace doesn't have any Developer containers:

    • New Terminal command should create a terminal in the editor container and don't ask user about containers (don't display the list of containers)

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=stable

codecov[bot] commented 2 years ago

Codecov Report

Merging #1318 (461caa6) into main (c299f59) will increase coverage by 3.97%. The diff coverage is 38.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1318      +/-   ##
==========================================
+ Coverage   32.78%   36.75%   +3.97%     
==========================================
  Files         290      330      +40     
  Lines        9885    11331    +1446     
  Branches     1457     1480      +23     
==========================================
+ Hits         3241     4165     +924     
- Misses       6641     7161     +520     
- Partials        3        5       +2
Impacted Files Coverage Δ
...theia-about/src/browser/about-che-theia-dialog.tsx 0.00% <0.00%> (ø)
...credentials/src/browser/che-credentials-service.ts 0.00% <0.00%> (ø)
...entials/src/browser/credentials-frontend-module.ts 0.00% <0.00%> (ø)
...eia-credentials/src/common/credentials-protocol.ts 0.00% <0.00%> (ø)
...eia-credentials/src/node/che-credentials-server.ts 0.00% <0.00%> (ø)
...s/src/node/che-theia-credentials-backend-module.ts 0.00% <0.00%> (ø)
...ashboard/src/browser/che-theia-dashboard-module.ts 0.00% <0.00%> (ø)
...ia-dashboard/src/browser/theia-dashboard-client.ts 0.00% <0.00%> (ø)
...rowser/src/browser/che-mini-browser-environment.ts 0.00% <0.00%> (ø)
...in-ext/src/browser/che-sidecar-file-system-main.ts 100.00% <ø> (ø)
... and 294 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e1c6b10...461caa6. Read the comment docs.

RomanNikitenko commented 2 years ago

@benoitf @l0rd @azatsarynnyy please let me know if you have any comments/idea/feedback related to the new behaviour - I will change/improve it if so. For example - from your point of view - is it OK to have Show All containers item at displaying Dev containers? or Do you prefer to remove it and the only way for a terminal creation in any container should be a specific command from command palette and the corresponding hotkey?

l0rd commented 2 years ago

Thank you @RomanNikitenko for the detailed videos. Great job. I don't think there is any value to have "show all containers" and "run terminal in a specific container". The developer using the IDE should not be aware of "tooling containers" or he will be confused. The expert Che contributor that once in a while needs to open a shell in a tooling container can use kubectl exec without any problem. Tooling containers should be "hidden" by Che. When there is no "dev container" we should default to the container where the IDE is running.

RomanNikitenko commented 2 years ago

Within my last commit I removed:

About the Workspace panel - let's rework it within a separate issue/PR.