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

Provide links to [stop/go to] running workspace when trying to start a workspace when workspace limit reached #607

Merged
merged 7 commits into from
Aug 26, 2022

Conversation

dkwon17
Copy link
Contributor

@dkwon17 dkwon17 commented Aug 15, 2022

Signed-off-by: dkwon17 [email protected]

What does this PR do?

When starting a workspace when there is already a workspace running, since the running workspace limit is 1 by default, there are now new links to:

  • go to the running workspace
  • stop the running workspace and restart the current workspace

image

When clicking "Close running workspace (bash) and restart golang-example", if there is an error when trying to stop the workspace, and alert with an error message will be displayed on the top right:

image

When there is more than one running workspace, there is a 'Return to Dashboard' link instead:

image

Demo video
Screen.Recording.2022-08-15.at.5.27.06.PM.mov

What issues does this PR fix or reference?

eclipse-che/che#21359

Is it tested? How?

  1. Check out this PR and run locally: See readme (or, test with this docker image: quay.io/eclipse/che-dashboard:pr-607)
  2. Start any workspace
  3. Start another workspace and verify that you see the following:

image

  1. Verify that both links work
  2. Start a second workspace (ie. set a DevWorkspace's spec.started to true)
  3. Start another workspace from the dashboard and verify that the dashboard prevents you from starting a workspace, and that you see the "Return to dashboard" link
  4. Verify that the "Return to dashboard" link works

To test the alert that appears on the top right of the dashboard when there is an error when stopping a workspace, I've created a dashboard image: quay.io/dkwon17/che-dashboard:multiworkspace-improvement-error that implements the following change:

diff --git a/packages/dashboard-frontend/src/store/Workspaces/devWorkspaces/index.ts b/packages/dashboard-frontend/src/store/Workspaces/devWorkspaces/index.ts
index 5cfea278..ad2d8b3b 100644
--- a/packages/dashboard-frontend/src/store/Workspaces/devWorkspaces/index.ts
+++ b/packages/dashboard-frontend/src/store/Workspaces/devWorkspaces/index.ts
@@ -417,6 +417,7 @@ export const actionCreators: ActionCreators = {
     (workspace: devfileApi.DevWorkspace): AppThunk<KnownAction, Promise<void>> =>
     async (dispatch): Promise<void> => {
       try {
+        throw '<error message here>';
         await devWorkspaceClient.changeWorkspaceStatus(workspace, false);
         dispatch({
           type: 'DELETE_DEVWORKSPACE_LOGS',
  1. Run the dashboard with the following image: quay.io/dkwon17/che-dashboard:multiworkspace-improvement-error
  2. Start any workspace
  3. Start another workspace and verify that the dashboard prevents you from starting the workspace and that you see the Close running workspace and restart [current workspace] link.
  4. Click on the link, verify that an error alert appears on the top right.

Release Notes

Docs PR

@openshift-ci
Copy link

openshift-ci bot commented Aug 15, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@che-bot
Copy link
Contributor

che-bot commented Aug 15, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Aug 15, 2022

Click here to review and test in web IDE: Contribute

@dkwon17 dkwon17 changed the title Working state, but need to decide what to do when failed to stop a wo… Provide links to [stop/go to] running workspace when trying to start a workspace when workspace limit reached Aug 15, 2022
@dkwon17 dkwon17 requested a review from nickboldt August 15, 2022 21:55
@che-bot
Copy link
Contributor

che-bot commented Aug 15, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Aug 15, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Aug 17, 2022

Click here to review and test in web IDE: Contribute

@dkwon17
Copy link
Contributor Author

dkwon17 commented Aug 17, 2022

My latest push also adds an alert on the top right corner if there was an error with the

Close running workspace (bash) and restart golang-example

link:
image

@che-bot
Copy link
Contributor

che-bot commented Aug 18, 2022

Click here to review and test in web IDE: Contribute

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #607 (27e3e07) into main (fd90af0) will increase coverage by 0.11%.
The diff coverage is 73.17%.

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
+ Coverage   58.42%   58.53%   +0.11%     
==========================================
  Files         245      245              
  Lines        7939     7997      +58     
  Branches     1314     1320       +6     
==========================================
+ Hits         4638     4681      +43     
- Misses       3115     3129      +14     
- Partials      186      187       +1     
Flag Coverage Δ
unittests 58.53% <73.17%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
...rd-frontend/src/containers/Loader/AbstractStep.tsx 100.00% <ø> (ø)
...s/dashboard-frontend/src/services/helpers/types.ts 100.00% <ø> (ø)
...ainers/Loader/Factory/Steps/FetchDevfile/index.tsx 87.68% <42.85%> (-1.21%) ⬇️
...ainers/Loader/Factory/Steps/ApplyDevfile/index.tsx 92.13% <50.00%> (-1.99%) ⬇️
...ners/Loader/Factory/Steps/ApplyResources/index.tsx 93.33% <50.00%> (-2.02%) ⬇️
...ners/Loader/Factory/Steps/FetchResources/index.tsx 90.00% <50.00%> (-2.11%) ⬇️
...ontend/src/store/Workspaces/devWorkspaces/index.ts 10.11% <50.00%> (+0.95%) ⬆️
...rs/Loader/Workspace/Steps/StartWorkspace/index.tsx 95.18% <60.00%> (-2.32%) ⬇️
...oard-frontend/src/pages/Loader/Workspace/index.tsx 89.79% <88.23%> (-4.95%) ⬇️
...ers/Loader/Factory/Steps/CreateWorkspace/index.tsx 96.29% <100.00%> (+0.14%) ⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

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

@dkwon17 dkwon17 marked this pull request as ready for review August 18, 2022 21:59
@dkwon17
Copy link
Contributor Author

dkwon17 commented Aug 18, 2022

This PR is now ready for review, I've updated the original PR description with steps to test this PR

@che-bot
Copy link
Contributor

che-bot commented Aug 18, 2022

Click here to review and test in web IDE: Contribute

@dkwon17 dkwon17 requested a review from ibuziuk August 18, 2022 22:11
@github-actions
Copy link

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

@che-bot
Copy link
Contributor

che-bot commented Aug 19, 2022

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

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

Signed-off-by: David Kwon <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label Aug 24, 2022
@che-bot
Copy link
Contributor

che-bot commented Aug 24, 2022

Click here to review and test in web IDE: Contribute

@dkwon17
Copy link
Contributor Author

dkwon17 commented Aug 24, 2022

Latest commit updates the link order and link text:
image

@che-bot
Copy link
Contributor

che-bot commented Aug 24, 2022

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

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

@nickboldt
Copy link
Contributor

nickboldt commented Aug 25, 2022

My only concern here (and maybe you've accounted for this already) is...

what if the user is allowed to run more than one concurrent workspace?

oc patch checluster/devspaces -n "${NAMESPACE}" --type='merge' -p '{"spec":{"components":{"devWorkspace":{"runningLimit":"5"}}}}'

or

oc patch checluster/che -n "${NAMESPACE}" --type='merge' -p '{"spec":{"components":{"devWorkspace":{"runningLimit":"5"}}}}'

Do you...

  • check for the # of concurrent running workspaces before redirecting to this message page? (eg., if 1 open and limit is 5, just open the 2nd workspace)
  • include a list of multiple open workspaces to close/switch to, or
  • grab the first one and provide choices for closing/switching to that first one, ignoring the others?

@dkwon17
Copy link
Contributor Author

dkwon17 commented Aug 25, 2022

@nickboldt in that case the first case happens:

  • check for the # of concurrent running workspaces before redirecting to this message page? (eg., if 1 open and limit is 5, just open the 2nd workspace)

The only time the new links introduced in this PR appears is when the runningLimit has been reached.

If the limit is more than 1, and the limit has been reached, at the moment this PR doesn't introduce a list of multiple open workspaces to close/switch to, but just shows a "Return to dashboard" link instead:

Screenshot

image

But the list idea does sound like a very nice enhancement

Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

Return to dashboard

That works for this PR! +1, good enough.

But the list idea does sound like a very nice enhancement

Feel free to open a separate GH issue to track / prioritize that enhancement. Thanks!

(Note that I'm not a TS expert so my review is based on screenshots and descriptions, not a technical code review.)

Signed-off-by: David Kwon <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label Aug 25, 2022
@che-bot
Copy link
Contributor

che-bot commented Aug 25, 2022

Click here to review and test in web IDE: Contribute

@openshift-ci
Copy link

openshift-ci bot commented Aug 25, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dkwon17, ibuziuk, nickboldt, 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

@github-actions
Copy link

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

@openshift-ci
Copy link

openshift-ci bot commented Aug 25, 2022

@dkwon17: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v8-dashboard-happy-path 186e326 link true /test v8-dashboard-happy-path

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Aug 25, 2022

Thank you everyone for the feedback 👍 , I plan to merge this PR tomorrow

@nickboldt I made an issue for the enhancement: eclipse-che/che#21660

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