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: fix progress window for 'minikube start' #78

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

ComradeProgrammer
Copy link
Contributor

@ComradeProgrammer ComradeProgrammer commented Jun 19, 2023

FIX #73
feat: set status to starting when starting minikube

When minikube is being started (by minikube-gui), both the system tray and the main window will show status "Starting"

Like this:
06201

06202

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 19, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 19, 2023
Copy link
Contributor

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

@ComradeProgrammer thank you for this PR,

can we make it that, if "minikube start" fails, will it revert back to what the status was before?

to test it you can start minikube with Gui, and then Kill the minikube in terminal, and u should expect some failure...and it should no longer say starting..

to test it

@ComradeProgrammer if you like to be ambtious and do it more interesting I suggest using the "minikube start --output=json" 's step

for example

$ minikube start --output=json
{"specversion":"1.0","id":"2f43b5a2-40a4-40f3-8034-3d5b3614876f","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.step","datacontenttype":"application/json","data":{"currentstep":"0","message":"minikube v1.30.1 on Darwin 13.4 (arm64)","name":"Initial Minikube Setup","totalsteps":"19"}}
{"specversion":"1.0","id":"24a20084-d5b6-4508-b214-287b27b317d1","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.warning","datacontenttype":"application/json","data":{"message":"Specified Kubernetes version 1.27.2 is newer than the newest supported version: v1.27.0-rc.0. Use `minikube config defaults kubernetes-version` for details."}}
{"specversion":"1.0","id":"33c342c3-598d-43df-8408-f3679e1de359","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.step","datacontenttype":"application/json","data":{"currentstep":"1","message":"Using the qemu2 driver based on existing profile","name":"Selecting Driver","totalsteps":"19"}}
{"specversion":"1.0","id":"993d1a56-1bf9-478a-b7e7-f3b2cf5ae46c","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.step","datacontenttype":"application/json","data":{"currentstep":"3","message":"Starting control plane node minikube in cluster minikube","name":"Starting Node","totalsteps":"19"}}
{"specversion":"1.0","id":"78994e90-2037-47cd-b9be-1ada184c21bf","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.step","datacontenttype":"application/json","data":{"currentstep":"3","message":"Restarting existing qemu2 VM for \"minikube\" ...","name":"Starting Node","totalsteps":"19"}}
{"specversion":"1.0","id":"57c4aeb6-401a-4577-a66f-cb2a4deec445","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.step","datacontenttype":"application/json","data":{"currentstep":"11","message":"Preparing Kubernetes v1.27.2 on containerd 1.7.2 ...","name":"Preparing Kubernetes","totalsteps":"19"}}
{"specversion":"1.0","id":"bfc38935-aef2-49e6-8ed6-5a37f4e157da","source":"https://minikube.sigs.k8s.io/","type":"io.k8s.sigs.minikube.error","datacontenttype":"application/json","data":{"message":"Unable to load cached images: loading cached images: stat /Users/medya/.minikube/cache/images/arm64/registry.k8s.io/kube-proxy_v1.27.2: no such file or directory"}}

when using the start output json
you can see

"currentstep":"0"
"name":"Initial Minikube Setup"
""totalsteps":"19""

so we can use that...how about the Status would show

"Starting 1/19 Initial minikube stup"

I know this would require to have Capture the logs and read them in live format...so this would need the Runner that Starts minikube to start minikbue with --output=json and pass a pointer to the IO.Reader of the log stream

@ComradeProgrammer
Copy link
Contributor Author

ComradeProgrammer commented Jun 25, 2023

I am indeed always ambitious, but this feature is definitely not a good place to be ambitious(laugh, it is a joke).

After reading the code I just found that Steven has previously implemented a more complicated progress tracking window like this:

Screenshot from 2023-07-09 18-13-06

When the minikube cluster is starting, real time outputs generated by -o json are sent to this window by CommandRunner::output signal, which is connected with Operator::commandOutput. After that, Operator::commandOutput updates the text and progress bar of this window.

However, this window, which had already implemented the required feature, was not visible at all before. It was because that when this window was created , the WindowFlags was incorrectly set. It should be Qt::Dialog| Qt::FramelessWindowHint (which equals to m_dialog->windowFlags() | Qt::FramelessWindowHint in this PR), not solely Qt::FramelessWindowHint, which make this window disappear.

I just amended my PR and fixed this bug.

By the way, I am always looking forward to contributing to some more complicated features, if possible. :)

@medyagh @spowelljr

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2023
@medyagh
Copy link
Contributor

medyagh commented Jul 5, 2023

I am indeed always ambitious, but this feature is definitely not a good place to be ambitious(laugh, it is a joke).

After reading the code I just found that Steven has previously implemented a more complicated progress tracking window like this:

Screenshot from 2023-06-25 22-39-37

When the minikube cluster is starting, real time outputs generated by -o json are sent to this window by CommandRunner::output signal, which is connected with Operator::commandOutput. After that, Operator::commandOutput updates the text and progress bar of this window.

However, this window, which had already implemented the required feature, was not visible at all before. It was because that when this window was created , the WindowFlags was incorrectly set. It should be Qt::Dialog| Qt::FramelessWindowHint (which equals to m_dialog->windowFlags() | Qt::FramelessWindowHint in this PR), not solely Qt::FramelessWindowHint, which make this window disappear.

I just amended my PR and fixed this bug.

By the way, I am always looking forward to contributing to some more complicated features, if possible. :)

@medyagh @spowelljr

Excellent ! @ComradeProgrammer good work here ! I appreciate your contribution and look forward to find good issues for you to work on

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2023
@spowelljr
Copy link
Member

Thanks for fixing the progress loading bar!

However, for the status showing Starting I don't quite understand the change.

You're not manually setting the text to Starting and minikube profile list -o json doesn't get called until after the start command has completed, so how is the text becoming Starting?

@ComradeProgrammer
Copy link
Contributor Author

ComradeProgrammer commented Jul 9, 2023

Thanks for fixing the progress loading bar!

However, for the status showing Starting I don't quite understand the change.

You're not manually setting the text to Starting and minikube profile list -o json doesn't get called until after the start command has completed, so how is the text becoming Starting?

Oh sorry I removed that part of code.

On the one hand, perhaps with this progress bar blocking the main window, we no longer need to change this text to make user understand minikube is being started.

On the other hand, because "Running" is not a status returned from minikube, so changing this text is quite complicated (we have to consider how to change the text back if this minikube start command fails)

I have changed the picture, which no longer shows a "Running status"

If we still need this text to be changed, perhaps I can implement it in a new PR

@ComradeProgrammer ComradeProgrammer changed the title feat: set status to starting when starting minikube fix: fix progress window for 'minikube start' Jul 9, 2023
@medyagh medyagh merged commit 5a9a4af into kubernetes-sigs:main Jul 17, 2023
4 of 5 checks passed
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ComradeProgrammer, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when click on start it the status should show Starting ...
4 participants