-
Notifications
You must be signed in to change notification settings - Fork 45
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
refactor: ignore HTTP response errors by status code #784
Conversation
This was an extra flaky CI AccTest run, mostly around network resources but also devices that timed out during provisioning and projects not being cleaned up as a result. Triggering a retest. (out of pr scope)
|
Looking at the failures again, they seem relevant to the change:
404s should be considered successful deletes. It wasn't picked up. This happens for a few different resource types. A device_timeout test fails with a 403, which may be exactly what we would have expected. |
Conversion of error filtering for impacted resources should be complete as of now, and this PR is ready for review. |
33af581
to
c7f86b1
Compare
bdc9b8d
to
dcbe220
Compare
dcbe220
to
9512026
Compare
9512026
to
648d135
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good from Fabric standpoint. Thanks @ctreatma !
// deletedMarker is a terraform-provider-only value that is used by the waiter | ||
// to indicate that the gateway appears to be deleted successfully based on | ||
// status code | ||
deletedMarker := "tf-marker-for-deleted-gateway" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
This was broken out of #782 since it involves a widely-used helper function and will impact more than just the Metal VLAN resource.
Previously, the
IgnoreHttpResponseErrors
helper function accepted as arguments a number of other, existing helper functions that are meant to evaluate whether a particular API response has a certain HTTP status. Here's an example helper function for checking if the response is a 403:This function immediately returns false if the response status code is not 403; otherwise it tries to convert the error argument to an
ErrorResponse
or apackngo.ErrorResponse
and then use a different helper function to determine if that error represents a 403. This assumes that the error object contains a copy of the response, which is only certain to be the case for code that usespackngo
. In practice we almost never did the necessary conversion toErrorResponse
for non-packngo
resources and data sources, so in many cases we were probably not successfully ignoring the desired response codes.This refactors
IgnoreHttpResponseErrors
to inspect HTTP status codes directly in the response instead of relying on helper functions for specific custom error types.This also removes the
FriendlyErrorForMetalGo
function which only existed in order to convert a separate HTTP response and error into anErrorResponse
, which is no longer necessary. Some usage ofFriendlyError
is also removed in cases where that function had no or minimal effect.