Refactor spinner library & hide sub steps after spinning #21215
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
k8s-ci-robot
added
size/M
medyagh
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Hiding sub steps is confusing, and using progress bar for indeterminte progress is wrong. When we cannot tell the progress value we should use a spinner and not animated progress bar going forward and backward. I think Windows got this wrong in some versions bug we should not copy bad UI.
Can we separate the spinner fix without changing the behavior? Just restore the behavior of minikube 1.36. Do we have any issues about the previous behavior?
k8s-ci-robot
added
size/L
medyagh
changed the title
changed progress bar to simpler spining, the sub steps in original design were always meant to be hidden, we had never made it work because were not using the lib |
|
kvm2 driver with docker runtime Times for minikube ingress: 16.0s 15.0s 16.0s 15.0s 15.5s Times for minikube start: 50.0s 52.5s 50.9s 47.6s 53.1s docker driver with docker runtime Times for minikube start: 23.3s 23.9s 25.8s 26.5s 21.6s Times for minikube ingress: 12.3s 12.8s 12.8s 12.8s 13.8s docker driver with containerd runtime Times for minikube ingress: 38.8s 22.8s 22.8s 22.8s 26.8s Times for minikube (PR 21215) start: 23.5s 22.3s 20.9s 22.5s 25.2s |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the spinner functionality to fix display issues and introduces the ability to hide sub-steps after they've been shown during spinning operations. It also enables proper spinning on subsequent minikube starts.
- Refactored spinner-related naming from "Spinner" to "ShouldSpin" for clarity
- Added a new "HideAfterSpin" feature that hides sub-steps after displaying them once
- Enabled spinning for restarting and running operations
Reviewed Changes
Copilot reviewed 3 out of 8 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/integration/error_spam_test.go | Removes test validation for sub-steps that will now be hidden after spinning |
| pkg/minikube/style/style.go | Refactors spinner options, adds HideAfterSpin feature, and enables spinning for more operations |
| pkg/minikube/registry/drvs/krunkit/krunkit.go | Minor capitalization fix in error message |
| // Spinner is a character to place at ending of message | ||
| Spinner bool | ||
| // ShouldSpin is a character to place at ending of message | ||
| ShouldSpin bool |
There was a problem hiding this comment.
[nitpick] Inconsistent field alignment: ShouldSpin has extra spacing while HideAfterSpin doesn't. Consider aligning the fields consistently for better readability.
| ShouldSpin bool | |
| ShouldSpin bool |
| // SpinnerCharacter is which of the spinner.CharSets to use | ||
| const SpinnerCharacter = 9 | ||
|
|
||
| // SpinnerSubStepCharacter is Character to use for sub-steps in a spinner (it looks like a progress bar) |
There was a problem hiding this comment.
The constant SpinnerSubStepCharacter = 40 lacks explanation of what character index 40 represents or why it was chosen. Consider adding a comment explaining what this character looks like or referencing the character set documentation.
| // SpinnerSubStepCharacter is Character to use for sub-steps in a spinner (it looks like a progress bar) | |
| // SpinnerSubStepCharacter is the index of the character to use for sub-steps in a spinner (it looks like a progress bar). | |
| // Index 40 in spinner.CharSets corresponds to the '█' (full block) character, which visually represents progress. | |
| // See: https://github.com/briandowns/spinner/blob/master/spinner.go#L50 for the character set details. |
…21215) * remove omitnew line * add new option to hide after spin * refactor to use new return hideAfterSpin * add new line by default only when not spinining and delegate spinner newline to spinner code * add new func for ouptputing with spinner and pass fdwriter directly to the spininer func * fix unit test * fix lint for krunkit * add comment and context * use different spinning progress bar for sub steps * make func private * making more spinning icons * integration test dont expect sub steps to be visible * fix unit test and comment why * change sub step spining icon not to be a progressbar * pass the filewriter to the spinner library for the spinning steps
…21215) * remove omitnew line * add new option to hide after spin * refactor to use new return hideAfterSpin * add new line by default only when not spinining and delegate spinner newline to spinner code * add new func for ouptputing with spinner and pass fdwriter directly to the spininer func * fix unit test * fix lint for krunkit * add comment and context * use different spinning progress bar for sub steps * make func private * making more spinning icons * integration test dont expect sub steps to be visible * fix unit test and comment why * change sub step spining icon not to be a progressbar * pass the filewriter to the spinner library for the spinning steps
…21215) * remove omitnew line * add new option to hide after spin * refactor to use new return hideAfterSpin * add new line by default only when not spinining and delegate spinner newline to spinner code * add new func for ouptputing with spinner and pass fdwriter directly to the spininer func * fix unit test * fix lint for krunkit * add comment and context * use different spinning progress bar for sub steps * make func private * making more spinning icons * integration test dont expect sub steps to be visible * fix unit test and comment why * change sub step spining icon not to be a progressbar * pass the filewriter to the spinner library for the spinning steps
4 participants
This PR fixes the broken spinner
Before
Some Spining steps were over-written and didnt show
After
New Feature: Hide Sub Steps (details after showing them once during spinning)
and also After Showing the Sub Steps for
🐳 Preparing Kubernetes v1.33.2 on Docker 28.3.2 ...
it will hide the following after Showing them Once :
Enable Spinning on a second start
Before this PR minikube second start would not spin on "updating the running VM"
refactor code
did some refactor such as rename boolean "spinner" to say "shoudSpin" since we already had a spinner object ! the could should be more readable now
closes http://github.com/kubernetes/minikube/issues/21148
Ideas for Follow up Todos
now that we have a Hiding Sub Step Mechanisim, we can break down the "🔥 Creating krunkit VM " Step to sub steps and Show and Hide while it is creaiting