-
Notifications
You must be signed in to change notification settings - Fork 118
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
Base64 encode response body #158
Conversation
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.
Aside from the nitpick over the suffix _std
, LGTM
docs/data-sources/http.md
Outdated
@@ -107,5 +107,6 @@ resource "random_uuid" "example" { | |||
|
|||
- `id` (String) The ID of this resource. | |||
- `response_body` (String) The response body returned as a string. | |||
- `response_body_base64_std` (String) The response body encoded as base64 (standard) as defined in [RFC 4648](https://datatracker.ietf.org/doc/html/rfc4648#section-4). |
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.
Nitpick: I know you are being precise here, but I don't think there can be any confusion as for what base64
is. Do we need the _std
suffix?
I know it's not a rule written in stone, but I haven't seen that prefix used in other providers that expose binaries that way.
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.
Ah, I looked at the RFC, and I think what you are tying to convey here is that we are using the standard Base64. But, even in the RFC, it says <<it might be referred to as "base64">>
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.
I don't think I've ever heard the "standard" part before (which very much intrigued me that there was another shorthand encoding term, "base64url", okay makes sense and today I learned), but take that with whatever grain of salt. 😅
If you're looking for something more concrete with respect to other large Terraform providers, it is safe to bet on calling it *_base64
. 👍
Some various references:
- https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_object#content_base64
- https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/key_vault_certificate#certificate_data_base64
- https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/endpoints_service#protoc_output_base64
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.
Have removed the _std
suffix.
Any chance we can get this PR merged and released? I am running into the same issue. |
7c90dcc
to
fd8ca19
Compare
}`, svr.URL), | ||
Check: resource.ComposeTestCheckFunc( | ||
// Note the replacement character in the string representation in `response_body`. | ||
resource.TestCheckResourceAttr("data.http.http_test", "response_body", "GIF89a\x01\x00\x01\x00�\x00\x00\x00\x00\x00\x00\x00\x00!�\x04\x01\x00\x00\x00\x00,\x00\x00\x00\x00\x01\x00\x01\x00\x00\x02\x02D\x01\x00;"), |
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.
@bendbennett what's the deal with the special characters instead of plain \xXX
(hex) notation?
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.
@mza08 response_body
contains the output from:
responseBody := string(bytes)
When an invalid UTF-8 sequence is encountered, it is replaced by the Unicode replacement character �.
I maintain a terraform provider for Xen Orchestra and leveraging this new functionality would allow me to avoid reimplementing similar logic in the terraform provider. What's the remaining work here? Is there any way I can help this effort? |
@bendbennett @detro Also asking about the state of this PR. Any chance to have this merged? |
Can we merge this? |
Hi all 👋, sorry about the delayed response. This PR is currently on-hold as there is a need to skip certain test scenarios for specific terraform versions ( 0.14.* ) and the testing framework currently does not have that capability. There has been some internal discussion about design of that testing functionality but our team's attention is currently elsewhere at the moment. |
Is this feature available in some sort of a fork? because i really need this |
Hi @kishaningithub 👋, As mentioned above there are some additional considerations so this PR is currently on-hold until we've addressed the issue of being able to skip certain test scenarios for specific terraform versions. Apologies for any inconvenience. |
# Conflicts: # CHANGELOG.md # internal/provider/data_source_http.go # internal/provider/data_source_http_test.go
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.
Just one small note, looks good! 🚀
# Conflicts: # go.mod # go.sum
Thanks for the hard works folks, what's the release schedule so we can get this out? 🙏 |
Hi there @darren-recentive 👋🏻 ! The team has released this feature with |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes: #157
Closes: #147
Closes: #145
Closes: #99
Closes: #72
Closes: #67
Closes: #38
Closes: #13
Closes: #11