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

[Synthetics] Fix follow redirects #256

Merged
merged 10 commits into from
Aug 7, 2019
Merged

[Synthetics] Fix follow redirects #256

merged 10 commits into from
Aug 7, 2019

Conversation

btoueg
Copy link
Contributor

@btoueg btoueg commented Jul 3, 2019

Considering:

  • TF nested schemas is limited to string values
  • follow_redirects is a boolean in Datadog json api

we need to convert boolean like this:

  • true => "true"
  • false => "false"

☝️better because follow_redirects is advertised as a boolean in the documentation so this PR:

  • enforces that its set as a boolean value in the config,
  • In TF 0.12.x the diff is improperly shown as going from a number to a boolean (i.e. 0/1 -> true/false) even if there was no diff. This worked fine in TF 0.11
  • Fixes ^ by converting any non bool values to bool

@ghost ghost added the size/M label Jul 3, 2019
@btoueg btoueg changed the title Fix follow redirects [Synthetics] Fix follow redirects Jul 3, 2019
Copy link
Contributor

@gabsn gabsn left a comment

Choose a reason for hiding this comment

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

@btoueg Did you run the acceptance tests? I'm very surprised nothing is broken whereas you did modify the schema itself. Also can you add a test on options too?

datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
@ghost ghost added size/XS and removed size/M labels Jul 9, 2019
@ghost ghost added size/S and removed size/XS labels Jul 30, 2019
@btoueg
Copy link
Contributor Author

btoueg commented Jul 30, 2019

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?       github.com/terraform-providers/terraform-provider-datadog       [no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDatadogScreenboard_update
--- PASS: TestAccDatadogScreenboard_update (9.37s)
=== RUN   TestAccDatadogSyntheticsAPITest_importBasic
=== PAUSE TestAccDatadogSyntheticsAPITest_importBasic
=== RUN   TestAccDatadogSyntheticsBrowserTest_importBasic
=== PAUSE TestAccDatadogSyntheticsBrowserTest_importBasic
=== RUN   TestAccDatadogSyntheticsAPITest_Basic
--- PASS: TestAccDatadogSyntheticsAPITest_Basic (7.35s)
=== RUN   TestAccDatadogSyntheticsAPITest_Updated
--- PASS: TestAccDatadogSyntheticsAPITest_Updated (12.94s)
=== RUN   TestAccDatadogSyntheticsBrowserTest_Basic
--- PASS: TestAccDatadogSyntheticsBrowserTest_Basic (7.20s)
=== RUN   TestAccDatadogSyntheticsBrowserTest_Updated
--- PASS: TestAccDatadogSyntheticsBrowserTest_Updated (12.57s)
=== CONT  TestAccDatadogSyntheticsAPITest_importBasic
=== CONT  TestAccDatadogSyntheticsBrowserTest_importBasic
--- PASS: TestAccDatadogSyntheticsAPITest_importBasic (7.83s)
--- PASS: TestAccDatadogSyntheticsBrowserTest_importBasic (7.83s)
PASS
ok      github.com/terraform-providers/terraform-provider-datadog/datadog       57.286s

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! Left a few comments, mostly about seeing if we can use ParseBool as I think it'll reduce the complexity here. Let me know what you think.

datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
return old == new
},
ValidateFunc: func(val interface{}, key string) (warns []string, errs []error) {
expectedValues := map[string]struct{}{"true": {}, "false": {}, "1": {}, "0": {}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use parseBool here too - https://golang.org/pkg/strconv/#ParseBool
If that function returns doesn't return an error then this validation passes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ParseBool does not fit my use case here, but I rewrote it with a switch à la ParseBool

@btoueg
Copy link
Contributor Author

btoueg commented Aug 1, 2019

TIL ParseBool thanks!

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?       github.com/terraform-providers/terraform-provider-datadog       [no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDatadogScreenboard_update
--- PASS: TestAccDatadogScreenboard_update (8.04s)
=== RUN   TestAccDatadogSyntheticsAPITest_importBasic
=== PAUSE TestAccDatadogSyntheticsAPITest_importBasic
=== RUN   TestAccDatadogSyntheticsBrowserTest_importBasic
=== PAUSE TestAccDatadogSyntheticsBrowserTest_importBasic
=== RUN   TestAccDatadogSyntheticsAPITest_Basic
--- PASS: TestAccDatadogSyntheticsAPITest_Basic (5.90s)
=== RUN   TestAccDatadogSyntheticsAPITest_Updated
--- PASS: TestAccDatadogSyntheticsAPITest_Updated (10.46s)
=== RUN   TestAccDatadogSyntheticsBrowserTest_Basic
--- PASS: TestAccDatadogSyntheticsBrowserTest_Basic (6.98s)
=== RUN   TestAccDatadogSyntheticsBrowserTest_Updated
--- PASS: TestAccDatadogSyntheticsBrowserTest_Updated (10.52s)
=== CONT  TestAccDatadogSyntheticsAPITest_importBasic
=== CONT  TestAccDatadogSyntheticsBrowserTest_importBasic
--- PASS: TestAccDatadogSyntheticsAPITest_importBasic (6.09s)
--- PASS: TestAccDatadogSyntheticsBrowserTest_importBasic (6.61s)
PASS
ok      github.com/terraform-providers/terraform-provider-datadog/datadog       48.537s```

ValidateFunc: func(val interface{}, key string) (warns []string, errs []error) {
followRedirectsRaw, ok := val.(map[string]interface{})["follow_redirects"]
if ok {
followRedirectsStr := followRedirectsRaw.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

As we chatted about offline, this type assertion causes an issue on TF 0.11 (works fine in 0.12) Lets try to convert this value into a string if possible to avoid breaking things.

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

Overall looks good to me and seems to work for TF 0.11 and 0.12, thanks for that. Currently it looks like the user shown diff is:

  ~ datadog_synthetics_test.test_synth
      options.follow_redirects: "false" => "1"

But there doesn't appear to be any issues with spurious non-empty diffs.

Though @btoueg since we learned that from the first point TF nested schemas is limited to string values actually can be boolean/int values and are just strings in the state, is this change explicitly needed then?

@nmuesch nmuesch merged commit 5262a2b into DataDog:master Aug 7, 2019
@nmuesch
Copy link
Contributor

nmuesch commented Aug 7, 2019

Discussed offline. The diff in 0.12 is:

"follow_redirects" = "true" -> "false"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants