integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
886 stars 726 forks source link

fix(data_source_github_rest_api): just read body and convert bytes into string #2152

Closed riezebosch closed 6 months ago

riezebosch commented 6 months ago

Resolves #1776

Pull request checklist

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

riezebosch commented 6 months ago

I was unable to get the the tests running locally, validated the work by testing it as a local provider. It is related to this PR: #2110 and also this would also solve #1776 and #1953

I expect the test could also be expanded to validate for some or actual value instead of only presence in state file.

By not swallowing the err from the Do method you see the source of the original bug:

Error: json: cannot unmarshal object into Go value of type string

By passing nil into v the client leaves the body untouched.

Update: I was able to get the tests running, updated the test to check for value and added another test to check for collection results.

Removed the err check so this change is not breaking, but would encourage to change that behaviour since it is now swallowing all exceptions.

riezebosch commented 6 months ago

@kfcampbell ☝🏻 🥺

kfcampbell commented 6 months ago

@riezebosch thanks for the attention here! The added test looks good to me. I'd be comfortable with adding an error check here; I think the user experience would be better if there was some kind of useful output rather than a swallowed error. Would you be comfortable adding that to this PR or would you prefer it to go in separately?

riezebosch commented 6 months ago

@kfcampbell I already had added it in a separate PR: https://github.com/integrations/terraform-provider-github/pull/2154 (but can combine the 2 if you prefer)

riezebosch commented 6 months ago

@kfcampbell thanks for merging #2154, I've updated this one accordingly.