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

[TF-13396] Build new decoded error #871

Closed
wants to merge 15 commits into from

Conversation

laurenolivia
Copy link
Contributor

@laurenolivia laurenolivia commented Mar 14, 2024

Description

When an HTTP response returns a 404, the details of the error are not logged. Currently, the error returns a string indicating the resource is not found. This PR builds an error that logs more info to help investigate what may be causing 404s.

Testing plan

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" go test ./... -v -run TestFunctionsAffectedByChange

...

@laurenolivia laurenolivia requested a review from a team as a code owner March 14, 2024 23:39
errors.go Outdated
@@ -373,3 +376,24 @@ var (

ErrStateVersionUploadNotSupported = errors.New("upload not supported by this version of Terraform Enterprise")
)

type DecodeHTTPError struct {
Copy link
Collaborator

@brandonc brandonc Mar 15, 2024

Choose a reason for hiding this comment

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

This looks like a useful tool, but it doesn't help for types that are already response errors that can be decoded, like the ones returned by checkResponseCode.

You could add a function that can decode the contents of a variety of errors, starting with ErrResourceNotFound, that can be built with a response body and work in tandem with checkResponseCode.

It's important that the existing message does not change for ErrResourceNotFound. The additional decoded details should be available from a different field and not from the Error() method.

type ErrResourceNotFound {
  Details []string
}

func (ErrResourceNotFound) Error() string {
  return "resource not found" // <-- this is the important compatibility part
}

func NewErrorFromResponse(response *http.Response) error {
  decodedDetails, err := decodeErrorPayload(response)
  if err != nil {
    // There aren't any error details so this error isn't a 'jsonapi error'
    return fmt.Errorf("could not decode jsonapi error details from this response: %w", err)
  }
  
  switch response.StatusCode {
  case 404:
    return ErrResourceNotFound{Details: decodedDetails}
  // etc
  }
  
  return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also don't forget unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandonc I'm wondering what you think about refactoring checkResponseCode to use the new struct

type ResError struct {
StatusCode   int
Info        []string
Base        error 
}

and then return ResError in checkResponseCode?

For example:

switch r.StatusCode {
case 400:
    ResError.Base = ErrInvalidIncludeValue
case 404:
    ResError.Base = ErrResourceNotFound
// etc
}

return ResError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unit tests will need to be updated because the error will be wrapped with this 👆 solution.
Likely tests that check for equality:

assert.Equal(t, ErrResourceNotFound, err)

@laurenolivia laurenolivia force-pushed the laurenolivia/build-new-error-struct branch from 0ec5a3e to cbdae48 Compare March 18, 2024 21:26
@laurenolivia
Copy link
Contributor Author

It’s been determined that this fix is a breaking change in go-tfe. At this time, we don’t have any plans to release V2. The original intent was to force more context in the output of a 404 due to a recurring flaky test in the provider. We’ve determined that any solution breaks go-tfe. Closing this PR for now.

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.

None yet

2 participants