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: allow ctrl+c in provider install #1219

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

divanshu-go
Copy link
Contributor

fix: allow ctrl+c in provider install

Description

  • This change requires a documentation update
  • I have made corresponding changes to the documentation

Related Issue(s)

Closes #1044

Screenshots

If relevant, please add screenshots.

Notes

Please add any relevant notes if necessary.

@divanshu-go
Copy link
Contributor Author

@quest-bot loot #1044

@quest-bot quest-bot bot added the ⚔️ Quest Tracks quest-bot quests label Oct 9, 2024
Copy link

quest-bot bot commented Oct 9, 2024

Quest PR submitted! image Quest PR submitted!

@bryans-go, you are attempting to solve the issue and loot this Quest. Will you be successful?


Questions? Check out the docs.

@divanshu-go
Copy link
Contributor Author

divanshu-go commented Oct 9, 2024

@Tpuljak
demo.webm
I think that this is the default behavior with the spinner we are using.
In daytona start I'm also not able to ctrl+c
this is same for all the loading screens using the inlinespinner and the full screen spinner func

@divanshu-go divanshu-go marked this pull request as ready for review October 9, 2024 10:08
@divanshu-go divanshu-go requested a review from a team as a code owner October 9, 2024 10:08
@Tpuljak
Copy link
Member

Tpuljak commented Oct 9, 2024

@Tpuljak demo.webm I think that this is the default behavior with the spinner we are using. In daytona start I'm also not able to ctrl+c this is same for all the loading screens using the inlinespinner and the full screen spinner func

@bryans-go you're right. This means that this PR is not ready for review? Please don't mark it as such until it is.

Are you wiling to solve this issue for the spinner in general so it's fixed in all the places where we use it? If so, we'll double the bounty amount through a tip.

@divanshu-go
Copy link
Contributor Author

@Tpuljak OK
I'm willing to solve it further

@Tpuljak Tpuljak marked this pull request as draft October 9, 2024 10:34
@divanshu-go divanshu-go marked this pull request as draft October 9, 2024 10:34
@divanshu-go
Copy link
Contributor Author

done!

@divanshu-go divanshu-go marked this pull request as ready for review October 9, 2024 14:56
@divanshu-go
Copy link
Contributor Author

divanshu-go commented Oct 10, 2024

demo9.webm
Now it is gracefully exiting

Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Screen.Recording.2024-10-11.at.14.39.26.mov

While this does solve allowing ctrl+c out of the spinner, it does not tackle the problem at hand which is aborting the provider installation. As you can see from my screen recording, the installation continues and completes even though I cancelled the request.
Please address that. Moving the PR to draft until you do so. Thanks.

@Tpuljak Tpuljak assigned Tpuljak and unassigned Tpuljak Oct 11, 2024
@Tpuljak Tpuljak marked this pull request as draft October 11, 2024 12:41
@divanshu-go divanshu-go marked this pull request as ready for review October 13, 2024 14:46
@divanshu-go divanshu-go requested a review from a team as a code owner October 13, 2024 14:46
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

The solution is very over-engineered 😅

I suggest you revert everything and go back to square one.
The easiest solution to this is to look at the DownloadFile function. I suggest that the functions takes in a context (that will be passed from the request). If that context is done before the download completes, it means that the user has aborted the request and that the download should stop and be cleaned up.
I'm pretty sure this is the only change (+ the spinner thing) that the PR will require.

Moving it back to draft.

pkg/cmd/provider/install.go Outdated Show resolved Hide resolved
@Tpuljak Tpuljak marked this pull request as draft October 14, 2024 08:13
@divanshu-go
Copy link
Contributor Author

divanshu-go commented Oct 14, 2024

@Tpuljak I also tried to implement the same thing that you said In your comment but once the request(POST in this case) is done by the user then how will the context get updated.
Can you please enumerate a more on your approach

@Tpuljak
Copy link
Member

Tpuljak commented Oct 14, 2024

@Tpuljak I also tried to implement the same thing that you said In your comment but once the request(POST in this case) is done by the user then how will the context get updated.

All requests (in gin) have their own context on the request. I believe (I'm 99% sure) that that context is Done() if the user cancels the request. You need to test that on your own.

@divanshu-go
Copy link
Contributor Author

divanshu-go commented Oct 14, 2024

Done !
@Tpuljak Thanks a lot . you did a great help

@divanshu-go divanshu-go marked this pull request as ready for review October 14, 2024 14:41
@divanshu-go
Copy link
Contributor Author

divanshu-go commented Oct 14, 2024

demo-2024-10-14_20.15.46.mov

pkg/os/download.go Outdated Show resolved Hide resolved
@lbrecic lbrecic merged commit ee7966b into daytonaio:main Oct 15, 2024
12 checks passed
@divanshu-go
Copy link
Contributor Author

@Tpuljak

@Tpuljak demo.webm I think that this is the default behavior with the spinner we are using. In daytona start I'm also not able to ctrl+c this is same for all the loading screens using the inlinespinner and the full screen spinner func

@bryans-go you're right. This means that this PR is not ready for review? Please don't mark it as such until it is.

Are you wiling to solve this issue for the spinner in general so it's fixed in all the places where we use it? If so, we'll double the bounty amount through a tip.

hey I think you forgot about the bounty part.

Copy link

quest-bot bot commented Oct 15, 2024

🧚 @bryans-go congratulations for completing Quest #1044

💰 A reward of $30 has been credited to you.

To claim your $30 reward follow the instructions here.

Questions? Check out the docs.

@divanshu-go
Copy link
Contributor Author

divanshu-go commented Oct 15, 2024

Are you wiling to solve this issue for the spinner in general so it's fixed in all the places where we use it? If so, we'll double the bounty amount through a tip.

tip ?
is it delayed
@Tpuljak you said that you will double the bounty amount through a tip

@daytonaio daytonaio deleted a comment from quest-bot bot Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚔️ Quest Tracks quest-bot quests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ctrl+c-ing out of a provider installation
3 participants