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 Editor Selector panel #1055

Merged
merged 10 commits into from
Mar 6, 2024
Merged

Add Editor Selector panel #1055

merged 10 commits into from
Mar 6, 2024

Conversation

akurinnoy
Copy link
Contributor

@akurinnoy akurinnoy commented Feb 20, 2024

What does this PR do?

This PR adds the Editor Selector panel to the Getting Started page. With the Editor Selector users can choose to use one of the provided editors, or define their editor by specifying the editor ID and/or editor container image.

Screenshot 2024-02-20 at 09 14 07

Screenshot 2024-02-20 at 09 13 52

Screenshot 2024-02-20 at 09 14 16

movie-2.mp4
movie-1.mp4

What issues does this PR fix or reference?

resolves eclipse-che/che#22766

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Feb 20, 2024

Click here to review and test in web IDE: Contribute

@akurinnoy akurinnoy self-assigned this Feb 20, 2024
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1055

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1055", name: che-dashboard}]}}]"

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: Patch coverage is 97.92531% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 88.86%. Comparing base (7ff8f04) to head (75f8b56).
Report is 2 commits behind head on main.

Files Patch % Lines
...ard-frontend/src/store/Plugins/chePlugins/index.ts 40.62% 19 Missing ⚠️
...src/pages/GetStarted/SamplesList/Gallery/index.tsx 94.69% 7 Missing ⚠️
...dashboard-frontend/src/Layout/Navigation/index.tsx 0.00% 1 Missing ⚠️
...d-frontend/src/components/EditorSelector/index.tsx 99.50% 1 Missing ⚠️
...es/GetStarted/SamplesList/Gallery/filterEditors.ts 98.11% 1 Missing ⚠️
...hboard-frontend/src/pages/WorkspacesList/index.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
+ Coverage   88.41%   88.86%   +0.45%     
==========================================
  Files         390      396       +6     
  Lines       40080    40484     +404     
  Branches     2707     2732      +25     
==========================================
+ Hits        35437    35978     +541     
+ Misses       4617     4480     -137     
  Partials       26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Really nice improvement, but let's merge it after 7.82.0 release (might be too risky to merge it for 3.12)

@ibuziuk
Copy link
Member

ibuziuk commented Feb 21, 2024

@artaleks9 @dmytro-ndp folks, will need your approval before merge

@ibuziuk ibuziuk removed the request for review from dmytro-ndp February 21, 2024 14:16
};

export type State = {
editorId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not just the editor ID. It could be the link to a container editor definition, URL, or editor ID.

I propose to change:

editorId -> editorDefinition

WDYT?

type State = {
activeTabKey: CreateWorkspaceTab;
editorId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
editorId: string;
editorDefinition: string;

@artaleks9
Copy link
Contributor

artaleks9 commented Feb 23, 2024

A verification of PR is still in progress, but I couldn't run python-django sample with che-idea/next and che-pycharm/next editor. It the same time this sample runs with default editor
Any ideas, why it happening ?

devworkspace-python-django.yaml.zip

Eclipse-Che-Starting-workspace-python-django

@artaleks9
Copy link
Contributor

Also, I can't run Go sample with any editor and Python sample with che-idea/next and che-pycharm/next.
(may be there is a some restriction, which I don't know?)

Eclipse-Che-Starting-workspace-go

Eclipse-Che-Starting-workspace-python

@olexii4
Copy link
Contributor

olexii4 commented Feb 23, 2024

We had the next mockup In the main issue eclipse-che/che#22784:
Знімок екрана 2024-02-23 о 18 31 31

I can see the link to the che-docs How to specify and use a custom editor. We already have this part in the che-docs.

@olexii4
Copy link
Contributor

olexii4 commented Feb 23, 2024

@akurinnoy I found a bug:

Try to create a new workspace with the Import from Git widget from the Git repo which includes the custom editor in the /.che/che-editor.yaml file.

For example web-java-spring-boot.

And default editor will be applied. So, we can't create a new workspace with the custom editor from the /.che/che-editor.yaml file at all.

Comment on lines 72 to 76
if (
factory.searchParams.has(EDITOR_ATTR) === false &&
factory.searchParams.has(EDITOR_IMAGE_ATTR) === false
) {
factory.searchParams.set(EDITOR_ATTR, editorId);
Copy link
Contributor

@olexii4 olexii4 Feb 23, 2024

Choose a reason for hiding this comment

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

We can't create a new workspace with the custom editor from the /.che/che-editor.yaml file with these changes at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With these changes, you are still able to create new workspaces with a custom editor. On any dashboard page, paste your repo link into the browser address bar to create a location like this and hit Enter: https://<che_fqdn>#https://github.com/olexii4/web-java-spring-boot.git. A new workspace with custom editor will be created within a few minutes.

However, I'm not sure if a .che/ file in a repository should have precedence over an explicitly defined editor, rather than vice versa.

cc @ibuziuk

Copy link
Contributor

Choose a reason for hiding this comment

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

@akurinnoy /.che/che-editor.yaml should define the default editor for a Git repo. Am I right or am I wrong?

@akurinnoy
Copy link
Contributor Author

@artaleks9 thanks for trying to test the pull request. About the workspace start failures you observed - that is something not related to this pull request. This issue is reproducible on the dogfooding cluster with Devfile.io samples and an Intellij IDE.

Screenshot 2024-02-26 at 11 51 17

As a workaround, I can suggest testing the Editor Selector with the Empty Workspace sample or using any repo of your choice with the Import from Git panel.

@akurinnoy
Copy link
Contributor Author

@olexii4 thanks for the review!

We had the next mockup In the main issue eclipse-che/che#22784
I can see the link to the che-docs How to specify and use a custom editor. We already have this part in the che-docs.

I'll update the PR to add this link

So, we can't create a new workspace with the custom editor from the /.che/che-editor.yaml file at all.

I believe this statement is not true... You're still able to create workspaces with custom editors defined in a che-editor.yaml by utilizing the factory link to the repository.

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
@akurinnoy
Copy link
Contributor Author

@olexii4
The che-docs page you posted describes how to define editor by setting up che-editor.yaml, which seems to be irrelevant to the Editor Selector. Because the editor definition field is the same as the che-editor URL parameter, I would use the link to URL parameter for the IDE page instead, do you mind?

@olexii4
Copy link
Contributor

olexii4 commented Feb 26, 2024

@olexii4 The che-docs page you posted describes how to define editor by setting up che-editor.yaml, which seems to be irrelevant to the Editor Selector. Because the editor definition field is the same as the che-editor URL parameter, I would use the link to URL parameter for the IDE page instead, do you mind?

From the user perspective, specifying the custom editor in the che-editor.yaml would be interesting. It is relevant to the Editor Selector because it helps to change the editor by default for each Git repo.

@dmytro-ndp
Copy link
Contributor

It's interesting to know:

  1. Is creation of workspace from editor going to have fail-safe mode with an ability to start workspace using default editor after workspace failed to start?
  2. Will Editor selection override editor definition in factory repo which workspace created from?
  3. Will Workspaces page show used custom editor information like the Editor definition URL and Editor? Will there be another way to find the information about used custom editor?
  4. Does the task include disabling tile of Editor which particular devfile sample doesn't support, like che-idea for ansible devfile sample?

@eclipse-che eclipse-che deleted a comment from ibuziuk Feb 26, 2024
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
@dkwon17
Copy link
Contributor

dkwon17 commented Feb 27, 2024

I don't know if this was mentioned yet but I have a small UI suggestion,

When an editor is selected, it would be nice if the editor title was black instead of grey:

Screenshot

Untitled drawing

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1055

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1055", name: che-dashboard}]}}]"

Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1055

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1055", name: che-dashboard}]}}]"

@akurinnoy
Copy link
Contributor Author

@dmytro-ndp, thanks for these questions

Is creation of workspace from editor going to have fail-safe mode with an ability to start workspace using default editor after workspace failed to start?

There is no such mode implemented in the dashboard, but this can be a really good improvement. Can you please open an issue?

Will Editor selection override editor definition in factory repo which workspace created from?

I discussed this question with @ibuziuk and @olexii4 and we decided to implement it as follows:

  • when the editor selector panel (title Choose an Editor) is opened, the selected editor will override the editor definition in the factory repo;
  • when the editor definition panel (title Use an Editor Definition) is opened and the fields are empty the editor definition in the factory repo will be applied.

Will Workspaces page show used custom editor information like the Editor definition URL and Editor? Will there be another way to find the information about used custom editor?

This info is not available on the workspaces page, but it is stored in the DevWorkspace object as an annotation. The che.eclipse.org/che-editor annotation contains the editor definition, and the che.eclipse.org/devfile annotation contains among other things all the query parameters provided by the editor selector component or by the user.

Does the task include disabling tile of Editor which particular devfile sample doesn't support, like che-idea for ansible devfile sample?

Disabling editor tiles is not part of this issue, and I believe it's not possible in the current implementation. Sample cards are not selectable but clicking on any immediately starts the new workspace creation, so there is no chance to choose an editor after.

@akurinnoy
Copy link
Contributor Author

@ibuziuk @artaleks9 @olexii4 @dkwon17 folks, I hope I fixed all the feedback, please review the pull request.

@akurinnoy
Copy link
Contributor Author

/retest

Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

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

These changes work as expected.
Good job!

@openshift-ci openshift-ci bot added the lgtm label Feb 29, 2024
@artaleks9
Copy link
Contributor

Verified on Eclipse Che with quay.io/eclipse/che-dashboard:pr-1055 - the functionality works properly.

Copy link

openshift-ci bot commented Mar 5, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, artaleks9, ibuziuk, olexii4

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@akurinnoy akurinnoy merged commit 49efdad into main Mar 6, 2024
18 checks passed
@akurinnoy akurinnoy deleted the editor-selector branch March 6, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a user I want to select the editor from UI when starting a workspace from UD
7 participants