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

feat: allow not waiting for actions #843

Closed
wants to merge 7 commits into from
Closed

feat: allow not waiting for actions #843

wants to merge 7 commits into from

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Aug 14, 2024

This PR adds the no-wait option, which allows to skip waiting for actions.

Closes #833

Related to #613, #489, #284

@phm07 phm07 added the feature label Aug 14, 2024
@phm07 phm07 self-assigned this Aug 14, 2024
@phm07 phm07 requested a review from a team as a code owner August 14, 2024 08:23
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 61.35%. Comparing base (f7c9ac6) to head (1b3d7dd).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
internal/state/actions.go 0.00% 9 Missing ⚠️
internal/ui/actions.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #843      +/-   ##
==========================================
- Coverage   61.43%   61.35%   -0.08%     
==========================================
  Files         237      237              
  Lines        8494     8505      +11     
==========================================
  Hits         5218     5218              
- Misses       2567     2578      +11     
  Partials      709      709              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/cmd/config/helptext/preferences.txt Outdated Show resolved Hide resolved
internal/state/actions.go Show resolved Hide resolved
@@ -14,7 +14,7 @@ import (
)

func (c *state) WaitForActions(cmd *cobra.Command, ctx context.Context, actions ...*hcloud.Action) error {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have "composite" commands in the CLI, where we wait for some actions and then make more API calls? These could fail if we do not wait for the initial actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for example when enabling protection after resource creation. How about we add a function like MustWaitForActions which doesn't include the wait option logic and then make WaitForActions a wrapper of that?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine to me.

We should also look at these cases and decide if its really necessary to wait for the running actions or the next call is independent and can still be made. (ie. is it necessary for the server to start before enabling the protection?)

@phm07
Copy link
Contributor Author

phm07 commented Aug 30, 2024

Since the scope of this PR is bigger than expected, I will close it for now. We plan to revisit this idea later.

@phm07 phm07 closed this Aug 30, 2024
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.

feat: allow not waiting for actions
3 participants