Utility Providers Upgrade #136

bendbennett · 2022-05-19T13:29:32Z

bflad

Very exciting!

bflad · 2022-05-19T16:28:40Z

internal/provider/data_source_test.go

- },
+ Config: fmt.Sprintf(testDataSourceConfigBasic, testHttpMock.server.URL, 200),
+ Check: resource.ComposeTestCheckFunc(
+ testAccDataSourceBasic("data.http.http_test"),


Seems like this could be written as the below using the acceptance testing helper functions, rather than introducing indirection with a separate function:

resource.TestCheckResourceAttr("data.http.http_test", "body", "1.0.0"), resource.TestCheckResourceAttr("data.http.http_test", "response_headers.X-Single", "foobar"), resource.TestCheckResourceAttr("data.http.http_test", "response_headers.X-Double", "1, 2"),

Good point. Fixed

bflad · 2022-05-19T16:29:29Z

internal/provider/data_source_test.go

- },
+ Config: fmt.Sprintf(testDataSourceConfigWithHeaders, testHttpMock.server.URL, 200),
+ Check: resource.ComposeTestCheckFunc(
+ testAccDataSourceHeaders("data.http.http_test"),


Seems like this could be written as the below using the acceptance testing helper functions, rather than introducing indirection with a separate function:

resource.TestCheckResourceAttr("data.http.http_test", "body", "1.0.0"),

bflad · 2022-05-19T16:30:02Z

internal/provider/data_source_test.go

- },
+ Config: fmt.Sprintf(testDataSourceConfigUTF8, testHttpMock.server.URL, 200),
+ Check: resource.ComposeTestCheckFunc(
+ testAccDataSourceUTF8("data.http.http_test"),


Seems like this could be written as the below using the acceptance testing helper functions, rather than introducing indirection with a separate function:

resource.TestCheckResourceAttr("data.http.http_test", "body", "1.0.0"),

bflad · 2022-05-19T16:30:41Z

internal/provider/data_source_test.go

+
+output "body" {
+ value = data.http.http_test.body
+}
+
+output "response_headers" {
+ value = data.http.http_test.response_headers
+}


Testing outputs is generally extraneous as compared to just directly checking the data source in question. 👍

Suggested change

output "body" {

value = data.http.http_test.body

}

output "response_headers" {

value = data.http.http_test.response_headers

}

bflad · 2022-05-19T16:30:51Z

internal/provider/data_source_test.go

+
+output "body" {
+ value = data.http.http_test.body
+}


Testing outputs is generally extraneous as compared to just directly checking the data source in question. 👍

Suggested change

output "body" {

value = data.http.http_test.body

}

bflad · 2022-05-19T16:31:11Z

internal/provider/data_source_test.go

+
+output "body" {
+ value = "${data.http.http_test.body}"
+}


Testing outputs is generally extraneous as compared to just directly checking the data source in question. 👍

Suggested change

output "body" {

value = "${data.http.http_test.body}"

}

bflad · 2022-05-19T16:32:02Z

docs/data-sources/http.md

-* `response_headers` - A map of strings representing the response HTTP headers.
- Duplicate headers are contatenated with `, ` according to
- [RFC2616](https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2)


Should we still be capturing this additional RFC detail in the attribute description?

Error on my part. Have reinstated.

bflad · 2022-05-19T16:33:44Z

DESIGN.md

+## Goals
+
+* [_Stability over features_](.github/CONTRIBUTING.md)
+* Provide managed data source to issue HTTP GET request.


Is there IETF and/pr W3 RFCs to link to for this design choice? Which HTTP versions are supported? Are non-standard HTTP implementations supported (e.g. SPDY, QUIC)?

Very good points.

Given the influx of requests, and the fact that once you start merging things here new requests for features are going to come, it's important to pin point which specific HTTP we support here.

Have updated DESIGN.md.

bflad · 2022-05-19T16:35:21Z

DESIGN.md

+
+* [_Stability over features_](.github/CONTRIBUTING.md)
+* Provide managed data source to issue HTTP GET request.
+* Support usage of requests to URLs directed to either `http` or `https`.


What implementation details of HTTPS are supported? TLS 1.0 or 1.1 (😬 )? 1.3? Are there relevant IETF/W3 RFCs?

Likely this can be also done by looking at the godoc for the HTTP client we use. And probably they link to these information there.

Have updated DESIGN.md.

.golangci.yml

detro

Aside from a few comments on the DESING.md, I essentially echo what @bflad spotted, especially for testing (and we already touched upon this in our daily chat).

But overall this is already looking great: very glad we are able to bring more and more consistency across those providers.

detro · 2022-05-20T11:48:02Z

DESIGN.md

+## Goals
+
+* [_Stability over features_](.github/CONTRIBUTING.md)
+* Provide managed data source to issue HTTP GET request.


Very good points.

Given the influx of requests, and the fact that once you start merging things here new requests for features are going to come, it's important to pin point which specific HTTP we support here.

detro · 2022-05-20T11:48:45Z

DESIGN.md

+
+* [_Stability over features_](.github/CONTRIBUTING.md)
+* Provide managed data source to issue HTTP GET request.
+* Support usage of requests to URLs directed to either `http` or `https`.


Likely this can be also done by looking at the godoc for the HTTP client we use. And probably they link to these information there.

detro · 2022-05-20T11:49:30Z

DESIGN.md

+* Only idempotent GET requests are supported.
+* Only 200 status codes are considered successful.


And I assume this will be updated once your RFC is finalised and work begin to expand this bit?

…ed HTTP and TLS protocol versions (#135)

…135)

bflad

Looks good to me 🚀 Just some minor additional considerations for the design documentation.

bflad · 2022-05-24T14:30:39Z

DESIGN.md

+HTTP URLs and either [HTTP/1.1](https://datatracker.ietf.org/doc/html/rfc2616) or 
+[HTTP/2.0](https://datatracker.ietf.org/doc/html/rfc7540) for HTTPS URLs depending on whether the server supports


Nit: Maybe just me but the usage of "URLs" tripped me up slightly here, as I thought it was somehow relating to the actual URL (scheme, host, port, etc.) Maybe clearer to say some thing like HTTP (plaintext) requests and HTTPS (secure) requests?

Fair point. Have amended.

bflad · 2022-05-24T14:32:42Z

DESIGN.md

+* Support usage of requests to URLs directed to either `http` or `https`. The current version of this provider is built
+with [Go 1.17](https://go.dev/doc/go1.17) which [supports](https://go.dev/doc/go1.17#minor_library_changes) 


This is excellent! I wonder if it is worth calling out that Go updates may change this support over time based on the fluid security landscape? For example, I believe Go 1.18 removes TLS/1.0 and TLS/1.1 support. I don't think we need to call out the Go 1.18 thing explicitly here, but I think it is worth ensuring we are free to track Go upgrades according to the design.

Have added a statement reflecting that TLS support will track the underlying Go version the provider is built with and will likely change over time.

bflad · 2022-05-24T14:34:06Z

DESIGN.md

+[TLS/1.0](https://www.ietf.org/rfc/rfc2246.txt) ([deprecated](https://datatracker.ietf.org/doc/rfc8996/)), 
+[TLS/1.1](https://datatracker.ietf.org/doc/html/rfc4346) ([deprecated](https://datatracker.ietf.org/doc/rfc8996/)), 
+[TLS/1.2](https://datatracker.ietf.org/doc/html/rfc5246) and 
+[TLS/1.3](https://datatracker.ietf.org/doc/html/rfc8446) (supported since [Go 1.13](https://go.dev/doc/go1.13#tls_1_3)). 


I think we can omit the final parenthesis statement here since as saying Go 1.13 won't mean much for practitioners (as they'd have to determine which provider versions were built and released on that Go version) 👍

Good point, have removed.

github-actions · 2024-05-27T15:37:40Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added dependencies documentation size/XL labels May 19, 2022

bendbennett added 10 commits May 19, 2022 14:35

Update to Go 1.17 (#135)

d88848e

Test all Terraform minor versions >=0.12 (#135)

cff029f

Switch to using TestCheckFunc and ProviderFactories (#135)

891ec19

Adding golangci-lint.yml and updating GNUmakefile and test.yml (#135)

2241b0b

Fix linting issues (#135)

6b88586

Use terraform-plugin-docs (#135)

60ed815

Adding DESIGN.md (#135)

127a442

Updating README.md (#135)

aca5461

Updating CHANGELOG.md (#135)

b109d47

Updating SUPPORT.md (#135)

4b7b20e

bendbennett force-pushed the bendbennett/issues-135 branch from d32f857 to 4b7b20e Compare May 19, 2022 13:38

bendbennett marked this pull request as ready for review May 19, 2022 15:32

bendbennett requested a review from a team as a code owner May 19, 2022 15:32

bflad reviewed May 19, 2022

View reviewed changes

detro reviewed May 20, 2022

View reviewed changes

bendbennett added 4 commits May 23, 2022 12:45

Using TestCheck helper funcs and removing outputs from tests (#135)

b9cdf49

Reinstating RFC link in response_headers doc (#135)

df21f79

Update DESIGN.md to include relevant information and links on support…

3f8e114

…ed HTTP and TLS protocol versions (#135)

Suppress max issues per linter and max same issues for golangci-lint (#…

193b5a7

…135)

bendbennett requested review from detro and bflad May 24, 2022 06:50

bflad approved these changes May 24, 2022

View reviewed changes

Rewording DESIGN.md (#135)

70934a5

bendbennett merged commit fff9b1c into main May 25, 2022

bendbennett deleted the bendbennett/issues-135 branch May 25, 2022 10:18

bendbennett mentioned this pull request May 25, 2022

Utility Providers Upgrade #135

Closed

13 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utility Providers Upgrade #136

Utility Providers Upgrade #136

bendbennett commented May 19, 2022

bflad left a comment

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

bendbennett May 23, 2022

bflad May 19, 2022

detro May 20, 2022

bendbennett May 23, 2022

bflad May 19, 2022

detro May 20, 2022

bendbennett May 23, 2022

detro left a comment

detro May 20, 2022

detro May 20, 2022

detro May 20, 2022

bflad left a comment

bflad May 24, 2022

bendbennett May 25, 2022

bflad May 24, 2022

bendbennett May 25, 2022

bflad May 24, 2022

bendbennett May 25, 2022

github-actions bot commented May 27, 2024

		* Only idempotent GET requests are supported.
		* Only 200 status codes are considered successful.

		HTTP URLs and either [HTTP/1.1](https://datatracker.ietf.org/doc/html/rfc2616) or
		[HTTP/2.0](https://datatracker.ietf.org/doc/html/rfc7540) for HTTPS URLs depending on whether the server supports

		* Support usage of requests to URLs directed to either `http` or `https`. The current version of this provider is built
		with [Go 1.17](https://go.dev/doc/go1.17) which [supports](https://go.dev/doc/go1.17#minor_library_changes)

Utility Providers Upgrade #136

Utility Providers Upgrade #136

Conversation

bendbennett commented May 19, 2022

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 27, 2024