Add spinner ending for running command #5964
Conversation
<!-- Thank you for opening a PR! Here are some things you need to know before submitting: 1. Please read our developer guideline: https://github.com/redhat-developer/odo/wiki/Dev:-odo-Dev-Guidelines 2. Label this PR accordingly with the '/kind' line 3. Ensure you have written and ran the appropriate tests: https://github.com/redhat-developer/odo/wiki/Dev:-Writing-and-running-tests 4. Read how we approve and LGTM each PR: https://github.com/redhat-developer/odo/wiki/Pull-Requests:-Review-guideline Documentation: If you are pushing a change to documentation, please read: https://github.com/redhat-developer/odo/wiki/Documentation:-Contributing --> **What type of PR is this:** <!-- Add one of the following kinds: /kind feature /kind cleanup /kind tests /kind documentation Feel free to use other [labels](https://github.com/redhat-developer/odo/labels) as needed. However one of the above labels must be present or the PR will not be reviewed. This instruction is for reviewers as well. --> /kind bug **What does this PR do / why we need it:** When running `odo dev`, it will never show that the command has completed. ```sh ✓ Building your application in container on cluster (command: install) [3s] • Executing the application (command: run) ... - Forwarding from 127.0.0.1:40001 -> 3000 ``` `spinner.End` hadn't been added so it never shows at it ending. This PR fixes that: ```sh ✓ Building your application in container on cluster (command: install) [3s] • Executing the application (command: run) ... ✓ Executing the application (command: run) [5s] ``` **Which issue(s) this PR fixes:** <!-- Specifying the issue will automatically close it when this PR is merged --> N/A **How to test changes / Special notes to the reviewer:** Signed-off-by: Charlie Drage <charlie@charliedrage.com>
✅ Deploy Preview for odo-docusaurus-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
When running
odo dev, it will never show that the command has completed.✓ Building your application in container on cluster (command: install) [3s] • Executing the application (command: run) ... - Forwarding from 127.0.0.1:40001 -> 3000
spinner.Endhadn't been added so it never shows at it ending.This PR fixes that:
✓ Building your application in container on cluster (command: install) [3s] • Executing the application (command: run) ... ✓ Executing the application (command: run) [5s]
@cdrage With the removal of Supervisord, odo dev now executes the (potentially long-running) run or debug command in a separate goroutine. So to me, we cannot know for sure when the command is done (until the corresponding goroutine reports so). This is why we used a spinner that does not end.
So ending the spinner right after the command is executed (in the background) might be a bit misleading to the users, no? In your example, it makes me think that the run command is done running in 5s..
Let's say the command exits a few seconds/minutes later (simulated by the command below in my Devfile):
- exec:
commandLine: 'sleep 10 && echo Exiting... && exit 1'
component: runtime
group:
isDefault: true
kind: run
workingDir: $PROJECT_SOURCE
id: runWith the changes in this PR, the spinner has already ended, so it will never be updated again, and the user would never know that the command failed:
❯ odo dev
__
/ \__ Developing using the my-nodesjs-with-k8s-uri-with-var Devfile
\__/ \ Namespace: default
/ \__/ odo version: v3.0.0-beta2
\__/
↪ Deploying to the cluster in developer mode
✓ Waiting for Kubernetes resources [6s]
✓ Syncing files into the container [151ms]
✓ Building your application in container on cluster (command: install) [4s]
• Executing the application (command: run) ...
✓ Executing the application (command: run) [5s]
- Forwarding from 127.0.0.1:40003 -> 8080
Your application is now running on the cluster
Watching for changes in the current directory /home/asoro/work/tmp/5451-deploy-k8s-uri-var-substitution
Press Ctrl+c to exit `odo dev` and delete resources from the cluster
Previously, users had such information reported:
❯ odo dev
__
/ \__ Developing using the my-nodesjs-with-k8s-uri-with-var Devfile
\__/ \ Namespace: default
/ \__/ odo version: v3.0.0-beta2
\__/
↪ Deploying to the cluster in developer mode
✓ Waiting for Kubernetes resources [8s]
✓ Syncing files into the container [160ms]
✓ Building your application in container on cluster (command: install) [5s]
• Executing the application (command: run) ...
- Forwarding from 127.0.0.1:40003 -> 8080
Your application is now running on the cluster
Watching for changes in the current directory /home/asoro/work/tmp/5451-deploy-k8s-uri-var-substitution
Press Ctrl+c to exit `odo dev` and delete resources from the cluster
✗ Executing the application (command: run) [10s]
Ah, I get your point. The problem is.. it looks really out of place with the spinner. We should remove the spinner entirely and just have a prompt saying that the command would be running in the background? I don't like how it's out of place with all the other stuff in the workflow when running We should just have it as: ↪ Deploying to the cluster in developer mode
✓ Waiting for Kubernetes resources [8s]
✓ Syncing files into the container [160ms]
✓ Building your application in container on cluster (command: install) [5s]
✓ Executing the application (command: run) in the background
- Forwarding from 127.0.0.1:40003 -> 8080Instead of the spinner. What do you think? |
That would be better indeed, as long as we are able to report the command status later (as shown in my previous example), if the command exits at any later point in time (either successfully or not). |
|
Closing this in favour of #5972 |
What type of PR is this:
/kind bug
What does this PR do / why we need it:
When running
odo dev, it will never show that the command hascompleted.
spinner.Endhadn't been added so it never shows at it ending.This PR fixes that:
✓ Building your application in container on cluster (command: install) [3s] • Executing the application (command: run) ... ✓ Executing the application (command: run) [5s]Which issue(s) this PR fixes:
N/A
How to test changes / Special notes to the reviewer:
Signed-off-by: Charlie Drage charlie@charliedrage.com