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

Fix editor update flow #1310

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix editor update flow #1310

wants to merge 2 commits into from

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Feb 4, 2025

What does this PR do?

This PR fixes the editor update flow and changes the approach of how to store editorYamlUrl variable in the devworkspace.metadata.annotations[REGISTRY_URL].
The existing implementation allows using editorId instead of URL.
With these changes, a correct API request http://127.0.0.1:8080/dashboard/api/editors/devfile?che-editor=${editorId} will be stored instead of an editorID.

What issues does this PR fix or reference?

fixes https://issues.redhat.com/browse/CRW-7936

Is it tested? How?

  1. Deploy Eclipse-Che with the image from this PR.
  2. Create a new workspace from the source:
https://github.com/eclipse-che/che-dashboard.git
  1. Stop the workspace. Open Swagger: {che-server}/dashboard/api/swagger.
    Use Devworkspace Template API and find the templateName.
  2. Modify(remove field /spec/commands) the target template with the next patch:
[
  {
    "op": "replace",
    "path": "/spec/commands",
    "value": []
  }
]
  1. Check the target template. The field /spec/commands should be empty.
  2. Start the target workspace. Check the additional request with the template/spec data:
    Знімок екрана 2025-02-04 о 01 47 16
  3. Check the target template. The field /spec/commands should be restored.

Signed-off-by: Oleksii Orel <[email protected]>
@che-bot
Copy link
Contributor

che-bot commented Feb 4, 2025

Click here to review and test in web IDE: Contribute

@olexii4 olexii4 requested a review from svor February 4, 2025 00:14
Copy link

github-actions bot commented Feb 4, 2025

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

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-1310", name: che-dashboard}]}}]"

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 12 lines in your changes missing coverage. Please review.

Project coverage is 91.76%. Comparing base (1635623) to head (9a617d5).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...orkspace-client/devworkspace/devWorkspaceClient.ts 77.35% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1310      +/-   ##
==========================================
+ Coverage   91.68%   91.76%   +0.08%     
==========================================
  Files         500      501       +1     
  Lines       45482    45532      +50     
  Branches     3164     3179      +15     
==========================================
+ Hits        41700    41784      +84     
+ Misses       3754     3715      -39     
- Partials       28       33       +5     

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

return Object.assign({ content: dump(editor), editorYamlUrl: editorIdOrPath });
return {
content: dump(editor),
editorYamlUrl: `http://127.0.0.1:8080/dashboard/api/editors/devfile?che-editor=${editorId}`,
Copy link
Member

Choose a reason for hiding this comment

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

@olexii4 could you clarify this change please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibuziuk The previous implementation allowed assigning editorId to a field named "editorYamlUrl". Based on the variable name 'editorYamlUrl', it is unexpected to have the value editorId there.

This misunderstanding led to an error. That is why I changed the implementation to use URLs instead of IDs.

This URL http://127.0.0.1:8080/dashboard/api/editors/devfile?che-editor=${editorId} is a part of dashboard API, which returns the editor devfile by ID. You can test it with dashboard swagger:

For example, an HTTP POST request {che-server}/dashboard/api/data/resolver with JSON data

{
  "url": "http://127.0.0.1:8080/dashboard/api/editors/devfile?che-editor=che-incubator/che-code/latest"
}

Знімок екрана 2025-02-04 о 15 27 15

PS
This request will be sent if we don't have the proper editor in the redux store.

@ibuziuk ibuziuk requested a review from dmytro-ndp February 4, 2025 15:14
Copy link
Contributor

@svor svor left a comment

Choose a reason for hiding this comment

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

@olexii4
I've tested the PR according to the description, and it works as expected.
Could you clarify the root cause of why the template wasn't patched with editorId?

Based on the variable name 'editorYamlUrl', it is unexpected to have the value editorId there.

I’d prefer to avoid hardcoding the API URL here. Would it make sense to rename the variable while still using editorId?

Copy link

github-actions bot commented Feb 5, 2025

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

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-1310", name: che-dashboard}]}}]"

@olexii4
Copy link
Contributor Author

olexii4 commented Feb 5, 2025

Could you clarify the root cause of why the template wasn't patched with editorId?

@svor The main reason was an error while converting devfile into the devworkspace template. The previous implementation worked well because we got a new editor devfile object before converting it into the devworkspace template. The existing implementation tried to change the editor devfile object in the redux store.

Copy link
Contributor

@svor svor left a comment

Choose a reason for hiding this comment

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

lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 5, 2025
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.

@olexii4 👍 LGTM
once approved by @dmytro-ndp please, backport to 7.98.x for 3.19

Copy link

openshift-ci bot commented Feb 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Feb 7, 2025

@olexii4, @ibuziuk : hello,
What do you think about verifying this PR using the version upgrade flow?
For example:

  1. Install Eclipse Che 7.98.x.
  2. Create a few workspaces.
  3. Update the Eclipse Che instance to the next version, with quay.io/eclipse/che-dashboard:pr-1310 on board.
  4. Start the old test workspaces and ensure the VS Code Editor version has been upgraded to the Eclipse Che next version

@olexii4
Copy link
Contributor Author

olexii4 commented Feb 7, 2025

@olexii4, @ibuziuk : hello, What do you think about verifying this PR using the version upgrade flow? For example:

  1. Install Eclipse Che 7.98.x.
  2. Create a few workspaces.
  3. Update the Eclipse Che instance to the next version, with quay.io/eclipse/che-dashboard:pr-1310 on board.
  4. Start the old test workspaces and ensure the VS Code Editor version has been upgraded to the Eclipse Che next version

This is a brilliant idea. But it's not that simple.
Note: The editors are stored in a namespaced ConfigMap. We have to update them.

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.

5 participants