Update go client, use new synthetics target #571

therve · 2020-07-08T16:15:04Z

This updates the go client to the latest version to be able to use the new
SyntheticsTest model, in particular the validatesJSONPath operator.

Closes ##545, #566

nmuesch

Thanks for this 🙇 Did a high level review, i'll spend some time next week playing around with test scenarios since we're modifying schema types. Left a couple questions/comments inline.

nmuesch · 2020-07-10T20:42:13Z

datadog/resource_datadog_synthetics_test_.go

+						} else if assertionTarget.GetOperator() == datadogV1.SYNTHETICSASSERTIONOPERATOR_VALIDATES {
+							assertionTarget.SetTarget(v.(string))
+						} else {
+							assertionTarget.SetTarget(v.(string))
+						}


Should this just be one else?

Yeah most likely.

nmuesch · 2020-07-10T20:42:34Z

datadog/resource_datadog_synthetics_test_.go

 			assertionMap := attr.(map[string]interface{})
 			if v, ok := assertionMap["type"]; ok {
 				assertionType := v.(string)
-				assertion.SetType(datadogV1.SyntheticsAssertionType(assertionType))


Any reason to change this logic if we're deprecating it?

That's because of the api client change I believe.

datadog/resource_datadog_synthetics_test_.go

nmuesch · 2020-07-10T20:53:26Z

datadog/resource_datadog_synthetics_test_.go

@@ -466,20 +562,48 @@ func updateSyntheticsTestLocalState(d *schema.ResourceData, syntheticsTest *data
 	d.Set("request_headers", actualRequest.Headers)

 	actualAssertions := syntheticsTest.GetConfig().Assertions
-	var localAssertions []map[string]string


Do we need to keep any of the existing logic for users that still use assertions for a bit?

Yeah I think I found what was needed there.

therve · 2020-07-11T08:29:23Z

Imports don't work properly just yet.

therve · 2020-07-11T16:34:09Z

OK I believe I found a correct path forward. The trick is to keep using the old field if it's referenced in the current state, otherwise use the new field everywhere. AFAIU it just a bit weird if you use import with the old schema.

datadog/resource_datadog_synthetics_test_.go

jirikuncar

@therve can you bump the version of datadog-api-client-go in separate PR?

nmuesch

I apologize I haven't done a full manual test with this yet, but I did take a deeper look and left another review with some followup questions

nmuesch · 2020-07-17T19:07:40Z

datadog/resource_datadog_synthetics_test_.go

+							Type:     schema.TypeString,
+							Optional: true,
+						},
+						"targetjsonpath": {


Should we have this conflictsWith target? They seem mutually exclusive

It doesn't work with nested fields unfortunately.

Can we try to bump the SDK version to https://github.com/hashicorp/terraform-plugin-sdk/releases/tag/v1.15.0

nmuesch · 2020-07-17T19:16:19Z

datadog/resource_datadog_synthetics_test_.go

-			if v, ok := assertionMap["operator"]; ok {
-				assertionOperator := v.(string)
-				assertion.SetOperator(datadogV1.SyntheticsAssertionOperator(assertionOperator))
+				if v, ok := assertionMap["operator"]; ok {


It looks like this code path is pretty much the same as before, but some is further indented now. From the schema aren't type and operator siblings? Do the operator and target checks need to happen inside the type check?

They are now required, so it's indented because we only create the structure when we already have the previous fields.

nmuesch · 2020-07-17T19:44:36Z

datadog/resource_datadog_synthetics_test_test.go

@@ -49,6 +73,8 @@ func TestAccDatadogSyntheticsSSLTest_importBasic(t *testing.T) {
 				ResourceName:      "datadog_synthetics_test.ssl",
 				ImportState:       true,
 				ImportStateVerify: true,
+				// Assertions will be imported into the new schema by default, but we can ignore them as users need to update the local config in this case
+				ImportStateVerifyIgnore: []string{"assertions", "assertion"},


Is the issue here that on import we will have both assertions and assertion in the state? Is this going to cause an unempty diff on future applys?

Actually, re-reading some of the code above, it looks like you check for the config before saving this field, in updateSyntheticsTestLocalState, so is this ignore still a requirement?

I check the config before saving the field, but it only works if I already have state. IIUC, the ImportStateVerify compares to a plain import, which doesn't compare to anything locally.

datadog/resource_datadog_synthetics_test_.go

nmuesch · 2020-07-17T19:55:05Z

datadog/resource_datadog_synthetics_test_.go

-			localAssertion["target"] = convertToString(target)
+		localAssertions[i] = localAssertion
+	}
+	// If the config still uses assertions, keep using that in the state to not generate useless diffs


I'd have to test around with this a bit more, but I'm not sure if d is always from the config. I think depending on which lifecycle piece we're in, d can reflect the object from the state instead.

Yeah it mostly comes from the state, but that's what I care about here.

therve · 2020-07-20T07:05:19Z

@therve can you bump the version of datadog-api-client-go in separate PR?

It needs a code change so I'd rather merge that one if we're close.

Update go client, use new synthetics target

4d4bc7b

ghost added size/XXL dependencies labels Jul 8, 2020

therve added the do-not-merge/WIP Do not merge this PR label Jul 8, 2020

therve added 4 commits July 9, 2020 18:53

New assertion schema

0f0d82f

Start tests

a5287da

Fixing tests, add missing cassettes

7e82d8a

Try test workaround

b3a56a0

therve force-pushed the therve/synthetics-json-path branch from 2447693 to b3a56a0 Compare July 10, 2020 20:19

therve added resource/synthetics and removed do-not-merge/WIP Do not merge this PR labels Jul 10, 2020

nmuesch reviewed Jul 10, 2020

View reviewed changes

therve added the do-not-merge/WIP Do not merge this PR label Jul 11, 2020

therve added 3 commits July 11, 2020 18:14

Try to use the new state

db53552

Fix tests

7ed7ef6

Handle review

5e77ef2

therve removed the do-not-merge/WIP Do not merge this PR label Jul 11, 2020

jirikuncar reviewed Jul 13, 2020

View reviewed changes

datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved

therve added 2 commits July 16, 2020 12:03

Other test fix

61f72de

Rename variable

2e9f4a9

jirikuncar reviewed Jul 16, 2020

View reviewed changes

nmuesch reviewed Jul 17, 2020

View reviewed changes

therve mentioned this pull request Jul 19, 2020

resource_datadog_synthetics_test should accept hash values as target variable. #545

Closed

therve added 2 commits July 20, 2020 09:15

Put conditional in else

ffec724

Add log messages

6b60428

jirikuncar approved these changes Jul 20, 2020

View reviewed changes

therve merged commit 7de6c6e into master Jul 20, 2020

therve deleted the therve/synthetics-json-path branch July 20, 2020 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update go client, use new synthetics target #571

Update go client, use new synthetics target #571

therve commented Jul 8, 2020 •

edited

Loading

nmuesch left a comment

nmuesch Jul 10, 2020

therve Jul 11, 2020

nmuesch Jul 10, 2020

therve Jul 11, 2020

nmuesch Jul 10, 2020

therve Jul 11, 2020

therve commented Jul 11, 2020

therve commented Jul 11, 2020

jirikuncar left a comment

nmuesch left a comment

nmuesch Jul 17, 2020

therve Jul 20, 2020 •

edited

Loading

jirikuncar Jul 20, 2020

nmuesch Jul 17, 2020

therve Jul 20, 2020

nmuesch Jul 17, 2020

therve Jul 20, 2020

nmuesch Jul 17, 2020

therve Jul 20, 2020

therve commented Jul 20, 2020

Update go client, use new synthetics target #571

Update go client, use new synthetics target #571

Conversation

therve commented Jul 8, 2020 • edited Loading

nmuesch 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

therve commented Jul 11, 2020

therve commented Jul 11, 2020

jirikuncar left a comment

Choose a reason for hiding this comment

nmuesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therve Jul 20, 2020 • edited Loading

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

therve commented Jul 20, 2020

therve commented Jul 8, 2020 •

edited

Loading

therve Jul 20, 2020 •

edited

Loading