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

driver: remove unnecessary waitActionTimeout #429

Merged
merged 3 commits into from
May 12, 2022

Conversation

waynr
Copy link
Contributor

@waynr waynr commented May 12, 2022

This came up in discussion of https://jira.internal.digitalocean.com/browse/CON-6582 where we determined that this timeout is unnecessary due to the timeout that gets set on the gRPC method call context.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM with one comment that I'll let you address as you see fit.

err := wait.PollUntil(1*time.Second, wait.ConditionFunc(func() (done bool, err error) {
action, _, err := d.storageActions.Get(ctx, volumeID, actionID)
action, resp, err := d.storageActions.Get(ctx, volumeID, actionID)
Copy link
Contributor

Choose a reason for hiding this comment

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

resp is still unused, which might trigger certain Go linters to emit a warning.

@waynr waynr force-pushed the wwarren/handle-get-actions-404-responses branch from 10cca17 to 5f15606 Compare May 12, 2022 22:08
@waynr
Copy link
Contributor Author

waynr commented May 12, 2022

I created a new branch. 2.x and cherry-picked this PR onto that: #430

The new 2.x branch is based on tag v2.1.2 which was the latest 2.x tag I could find.

@waynr waynr merged commit 3744515 into master May 12, 2022
@waynr waynr deleted the wwarren/handle-get-actions-404-responses branch May 12, 2022 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants