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

Check if error is retryable before returning that it is #1934

Merged
merged 2 commits into from
Sep 20, 2018

Conversation

carlpett
Copy link
Contributor

Fixes #1757.

This PR checks if errors returned in resource_arm_role_assignment are actually retryable, rather than always retrying. If the approach is ok, it should also be applied to resource_arm_storage_container, which uses the same "always retry" logic.
Currently, I just check if the error (or nested error) is a *url.Error and if so if it is a temporary or timeout error. Anything else is deemed non-retryable. Are there other cases that should be retried?

@ghost ghost added the size/S label Sep 17, 2018
@carlpett
Copy link
Contributor Author

Also, this probably needs a test. Is the accepted way there to just write an acceptance test and push it and see if it works? I don't have an Azure account that I can/am allowed to run them on.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @carlpett,

Thank you for this fix. Might I suggest we move this to the utils/response package and create a new function ResponseErrorIsRetryable? I would also combine the switch and function here into this new call.

if err != nil {
  if utils.ResponseErrorIsRetryable(err) {
    return resource.RetryableError(err)
  } else {
    return resource.NonRetryableError(err)
  }
}

What do you think?

@carlpett
Copy link
Contributor Author

Sure! I mostly threw it together without having too much insights into the repo at large, your suggestions look very good. Will push an update soon!
Regarding the acceptance tests, do you trigger them for PRs that look good?

@katbyte
Copy link
Collaborator

katbyte commented Sep 19, 2018

@carlpett,

Once the PR LGTM (or almost does) I will run the acceptance test internally for the entire resource. If you write an acceptance test it will be verified at that time.

@katbyte katbyte added this to the 1.16.0 milestone Sep 19, 2018
@katbyte katbyte added the bug label Sep 19, 2018
@ghost ghost added the size/S label Sep 20, 2018
@ghost ghost added size/M and removed size/S labels Sep 20, 2018
@carlpett
Copy link
Contributor Author

Pushed the implementation of your suggestions. I also noticed that net/url.Error is an implementation of net.Error, which covers a much wider range of errors, so I switched it out for that instead.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @carlpett, LGTM 👍

@katbyte katbyte merged commit 0e1280c into hashicorp:master Sep 20, 2018
katbyte added a commit that referenced this pull request Sep 20, 2018
@ghost
Copy link

ghost commented Mar 6, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants