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: Do not set 'ephemeral' storage for the 'Safe' mode #967

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Conversation

ibuziuk
Copy link
Member

@ibuziuk ibuziuk commented Oct 31, 2023

What does this PR do?

Do not set 'ephemeral' storage for the 'Safe' mode

What issues does this PR fix or reference?

Is it tested? How?

  • install Eclipse Che and patch the dashboard image
spec:
  components:
    dashboard:
      deployment:
        containers:
          - image: quay.io/eclipse/che-dashboard:pr-967
  • Start an empty workspace
  • Add a few files to the workspace and then add a devfile with an image that doesn't exist e.g.
schemaVersion: 2.2.0
metadata:
  name: devfile-with-typo
components:
  - name: tools
    container:
      image: quay.io/does/not/exist
  • Select restart from local Devfile
  • Wait for an error message
  • Click the "Restart with default devfile" button.
  • Wait for the target workspace to restart and open IDE
  • See that the devfile exist and the changes are not lost

Release Notes

N/A

Docs PR

N/A

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Oct 31, 2023

Click here to review and test in web IDE: Contribute

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #967 (78538ff) into main (a8820af) will increase coverage by 0.46%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #967      +/-   ##
==========================================
+ Coverage   84.71%   85.17%   +0.46%     
==========================================
  Files         363      380      +17     
  Lines       37514    39176    +1662     
  Branches     2440     2519      +79     
==========================================
+ Hits        31779    33369    +1590     
- Misses       5707     5780      +73     
+ Partials       28       27       -1     
Flag Coverage Δ
unittests 85.17% <100.00%> (+0.46%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...paceProgress/CreatingSteps/Apply/Devfile/index.tsx 93.10% <100.00%> (+3.08%) ⬆️
...ontend/src/store/Workspaces/devWorkspaces/index.ts 77.50% <100.00%> (-0.16%) ⬇️

... and 29 files with indirect coverage changes

@ibuziuk ibuziuk requested a review from dmytro-ndp October 31, 2023 14:35
Copy link

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

@ibuziuk
Copy link
Member Author

ibuziuk commented Oct 31, 2023

@olexii4 @akurinnoy it looks like smth. is still wrong - after doing the workspace restart based on the local devfile changes disappear from the workspace? am I missing smth. or this PR is not enough to fix the issue

@ibuziuk ibuziuk changed the title fix: Do not set 'ephemeral' storage for the 'Safe' mode [DO NOT MERGE] fix: Do not set 'ephemeral' storage for the 'Safe' mode Oct 31, 2023
@dmytro-ndp
Copy link
Contributor

I can confirm that workspace is still restarting with default devfile using ephemeral Storage Type: https://youtu.be/mFvwfgelhs8?si=43ZyfQOpn_JXtrlA

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

github-actions bot commented Nov 1, 2023

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

1 similar comment
Copy link

github-actions bot commented Nov 1, 2023

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

@olexii4
Copy link
Contributor

olexii4 commented Nov 1, 2023

it looks like smth. is still wrong - after doing the workspace restart based on the local devfile changes disappear from the workspace? am I missing smth. or this PR is not enough to fix the issue

@ibuziuk We still have an old issue: Restart based on the local Devfile doesn`t work properly

@akurinnoy
Copy link
Contributor

@ibuziuk @dmytro-ndp please test the PR.

Copy link
Contributor

@dmytro-ndp dmytro-ndp left a comment

Choose a reason for hiding this comment

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

Looks good to merge. Well done!
Verification screencast: https://youtu.be/XrIgFNEz0qY

@openshift-ci openshift-ci bot added the lgtm label Nov 1, 2023
@ibuziuk ibuziuk changed the title [DO NOT MERGE] fix: Do not set 'ephemeral' storage for the 'Safe' mode fix: Do not set 'ephemeral' storage for the 'Safe' mode Nov 2, 2023
@ibuziuk
Copy link
Member Author

ibuziuk commented Nov 2, 2023

@ibuziuk We still have an old issue: eclipse-che/che#22453

yeah, but this is unrelated to the ephemeral mode I believe. @olexii4 Could you please review & approve if all is good with this PR in regard to eclipse-che/che#22638 ?

Copy link

openshift-ci bot commented Nov 2, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dmytro-ndp, 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

@ibuziuk ibuziuk merged commit c2295c4 into main Nov 2, 2023
@ibuziuk ibuziuk deleted the che-22638 branch November 2, 2023 12:04
@devstudio-release
Copy link

Build 3.10 :: dashboard_3.10/12: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: dashboard_3.x/384: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: dashboard_3.x/384: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/5155 triggered

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.10 :: dashboard_3.10/12: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.10/85 triggered

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.10 :: update-digests_3.10/86: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.10 :: update-digests_3.10/86: UNSTABLE

No new images detected: nothing to do!

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: copyIIBsToQuay/2070: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: sync-to-downstream_3.x/5156: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/5039 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.11 :: operator-bundle_3.x/2247: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/5156 triggered

@devstudio-release
Copy link

Build 3.11 :: update-digests_3.x/4777: SUCCESS

Detected new images: rebuild operator-bundle
* dashboard; /DS_CI/operator-bundle_3.x/2247 triggered

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1513: Console, Changes, Git Data

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.

6 participants