Fix inability to update flow variables (#325)

* Fix inability to update flow variables on update

* changelog

* changelog

* update tests

.changelog/325.txt

@@ -0,0 +1,11 @@
+```release-note:bug
+`resource/davinci_flow`: Fix issue whereby flow variables cannot be updated, leading to error.
+```
+
+```release-note:bug
+`resource/davinci_flow`: Fixed `flow_variables.type` so that it refers to the data type of the variable (as is the original intention), rather than the type of the variable object.
+```
+
+```release-note:enhancement
+`resource/davinci_flow`: Added `flow_variables.value` to allow the variable's default value to be updated.
+```

docs/resources/flow.md

@@ -72,7 +72,7 @@ resource "davinci_flow" "my_awesome_main_flow" {
 
 - `flow_configuration_json` (String, Sensitive) The parsed configuration of the DaVinci Flow import JSON.  Drift is calculated based on this attribute.
 - `flow_export_json` (String, Sensitive) The DaVinci Flow export in raw JSON format following successful import, including target environment metadata.
-- `flow_variables` (Attributes Set) Returned list of Flow Context variables. These are variable resources that are created and managed by the Flow resource via `flow_json`. (see [below for nested schema](#nestedatt--flow_variables))
+- `flow_variables` (Attributes Set) List of Flow variables that will be updated in the DaVinci instance. These are variable resources that are created and managed by the Flow resource, where variables are embedded in the `flow_json` DaVinci export file. (see [below for nested schema](#nestedatt--flow_variables))
 - `id` (String) The ID of this resource.
 
 <a id="nestedblock--connection_link"></a>
@@ -115,6 +115,7 @@ Read-Only:
 - `mutable` (Boolean) A boolean that specifies whether the variable is mutable.  If `true`, the variable can be modified by the flow. If `false`, the variable is read-only and cannot be modified by the flow.
 - `name` (String) The user friendly name of the variable in the UI.
 - `type` (String) The variable's data type.  Expected to be one of `string`, `number`, `boolean`, `object`.
+- `value` (String) A string that represents the variable's default value.
 
 ## Import
 

go.mod

@@ -5,14 +5,18 @@ go 1.21.1
 require (
 	github.com/bflad/tfproviderlint v0.29.0
 	github.com/golangci/golangci-lint v1.59.0
+	github.com/google/go-cmp v0.6.0
 	github.com/hashicorp/go-changelog v0.0.0-20221013053416-ba40b3a8c7ff
+	github.com/hashicorp/go-multierror v1.1.1
 	github.com/hashicorp/terraform-plugin-docs v0.19.2
 	github.com/hashicorp/terraform-plugin-framework v1.8.0
 	github.com/hashicorp/terraform-plugin-framework-validators v0.12.0
+	github.com/hashicorp/terraform-plugin-go v0.23.0
 	github.com/hashicorp/terraform-plugin-log v0.9.0
 	github.com/hashicorp/terraform-plugin-mux v0.16.0
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0
 	github.com/katbyte/terrafmt v0.5.3
+	github.com/patrickcping/pingone-go-sdk-v2 v0.11.8
 	github.com/patrickcping/pingone-go-sdk-v2/management v0.39.0
 	github.com/pavius/impi v0.0.3
 	github.com/samir-gandhi/davinci-client-go v0.3.0
@@ -284,14 +288,12 @@ require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/fatih/color v1.17.0 // indirect
 	github.com/golang/protobuf v1.5.4 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/hashicorp/errwrap v1.1.0 // indirect
 	github.com/hashicorp/go-checkpoint v0.5.0 // indirect
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
 	github.com/hashicorp/go-cty v1.4.1-0.20200414143053-d3edf31b6320 // indirect
 	github.com/hashicorp/go-hclog v1.5.0 // indirect
-	github.com/hashicorp/go-multierror v1.1.1
 	github.com/hashicorp/go-plugin v1.6.0 // indirect
 	github.com/hashicorp/go-uuid v1.0.3 // indirect
 	github.com/hashicorp/go-version v1.7.0 // indirect
@@ -300,7 +302,6 @@ require (
 	github.com/hashicorp/logutils v1.0.0 // indirect
 	github.com/hashicorp/terraform-exec v0.20.0 // indirect
 	github.com/hashicorp/terraform-json v0.21.0 // indirect
-	github.com/hashicorp/terraform-plugin-go v0.23.0
 	github.com/hashicorp/terraform-registry-address v0.2.3 // indirect
 	github.com/hashicorp/terraform-svchost v0.1.1 // indirect
 	github.com/hashicorp/yamux v0.1.1 // indirect
@@ -314,7 +315,6 @@ require (
 	github.com/mitchellh/mapstructure v1.5.0 // indirect
 	github.com/mitchellh/reflectwalk v1.0.2 // indirect
 	github.com/oklog/run v1.1.0 // indirect
-	github.com/patrickcping/pingone-go-sdk-v2 v0.11.8
 	github.com/posener/complete v1.2.3 // indirect
 	github.com/shopspring/decimal v1.3.1 // indirect
 	github.com/spf13/cast v1.5.0 // indirect

internal/service/davinci/resource_flow.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/hashicorp/terraform-plugin-framework-validators/setvalidator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
 	"github.com/hashicorp/terraform-plugin-framework/attr"
@@ -53,18 +54,6 @@ type FlowSubflowLinkResourceModel struct {
 	Name                   types.String `tfsdk:"name"`
 }
 
-type FlowVariablesResourceModel struct {
-	Id          types.String `tfsdk:"id"`
-	Name        types.String `tfsdk:"name"`
-	Description types.String `tfsdk:"description"`
-	FlowId      types.String `tfsdk:"flow_id"`
-	Context     types.String `tfsdk:"context"`
-	Type        types.String `tfsdk:"type"`
-	Mutable     types.Bool   `tfsdk:"mutable"`
-	Min         types.Int64  `tfsdk:"min"`
-	Max         types.Int64  `tfsdk:"max"`
-}
-
 var (
 	flowVariablesTFObjectTypes = map[string]attr.Type{
 		"id":          types.StringType,
@@ -73,6 +62,7 @@ var (
 		"flow_id":     types.StringType,
 		"context":     types.StringType,
 		"type":        types.StringType,
+		"value":       types.StringType,
 		"mutable":     types.BoolType,
 		"min":         types.Int64Type,
 		"max":         types.Int64Type,
@@ -170,7 +160,7 @@ func (r *FlowResource) Schema(ctx context.Context, req resource.SchemaRequest, r
 	)
 
 	flowVariablesDescription := framework.SchemaAttributeDescriptionFromMarkdown(
-		"Returned list of Flow Context variables. These are variable resources that are created and managed by the Flow resource via `flow_json`.",
+		"List of Flow variables that will be updated in the DaVinci instance. These are variable resources that are created and managed by the Flow resource, where variables are embedded in the `flow_json` DaVinci export file.",
 	)
 
 	flowVariablesContextDescription := framework.SchemaAttributeDescriptionFromMarkdown(
@@ -181,6 +171,10 @@ func (r *FlowResource) Schema(ctx context.Context, req resource.SchemaRequest, r
 		"The variable's data type.  Expected to be one of `string`, `number`, `boolean`, `object`.",
 	)
 
+	flowVariablesValueDescription := framework.SchemaAttributeDescriptionFromMarkdown(
+		"A string that represents the variable's default value.",
+	)
+
 	flowVariablesMutableDescription := framework.SchemaAttributeDescriptionFromMarkdown(
 		"A boolean that specifies whether the variable is mutable.  If `true`, the variable can be modified by the flow. If `false`, the variable is read-only and cannot be modified by the flow.",
 	)
@@ -306,6 +300,12 @@ func (r *FlowResource) Schema(ctx context.Context, req resource.SchemaRequest, r
 							Computed:            true,
 						},
 
+						"value": schema.StringAttribute{
+							Description:         flowVariablesValueDescription.Description,
+							MarkdownDescription: flowVariablesValueDescription.MarkdownDescription,
+							Computed:            true,
+						},
+
 						"mutable": schema.BoolAttribute{
 							Description:         flowVariablesMutableDescription.Description,
 							MarkdownDescription: flowVariablesMutableDescription.MarkdownDescription,
@@ -797,6 +797,93 @@ func (r *FlowResource) Update(ctx context.Context, req resource.UpdateRequest, r
 		}
 	}
 
+	// Variables
+	if !plan.FlowVariables.Equal(state.FlowVariables) {
+		var flowVarPlan, flowVarState []VariableResourceModel
+		resp.Diagnostics.Append(plan.FlowVariables.ElementsAs(ctx, &flowVarPlan, false)...)
+		resp.Diagnostics.Append(state.FlowVariables.ElementsAs(ctx, &flowVarState, false)...)
+		// If there are errors, keep going as it's a read to state left
+
+		if !resp.Diagnostics.HasError() {
+			for _, flowVar := range flowVarPlan {
+				var flowVarStateFound bool
+				for _, flowVarState := range flowVarState {
+					if flowVar.Id.Equal(flowVarState.Id) {
+						flowVarStateFound = true
+
+						// test for modifications
+						if !cmp.Equal(flowVar, flowVarState) {
+							flowVariable := flowVar.expand()
+
+							_, err := sdk.DoRetryable(
+								ctx,
+								r.Client,
+								environmentID,
+								func() (interface{}, *http.Response, error) {
+									return r.Client.UpdateVariableWithResponse(environmentID, flowVariable)
+								},
+							)
+							if err != nil {
+								resp.Diagnostics.AddError(
+									"Error adding flow variable",
+									fmt.Sprintf("Error adding flow variable as part of flow update: %s", err),
+								)
+							}
+						}
+					}
+				}
+
+				if !flowVarStateFound {
+					// add the new variable
+					flowVariable := flowVar.expand()
+
+					_, err := sdk.DoRetryable(
+						ctx,
+						r.Client,
+						environmentID,
+						func() (interface{}, *http.Response, error) {
+							return r.Client.CreateVariableWithResponse(environmentID, flowVariable)
+						},
+					)
+					if err != nil {
+						resp.Diagnostics.AddError(
+							"Error adding flow variable",
+							fmt.Sprintf("Error adding flow variable as part of flow update: %s", err),
+						)
+					}
+				}
+			}
+
+			for _, flowVar := range flowVarState {
+				var flowVarPlanFound bool
+				for _, flowVarPlan := range flowVarPlan {
+					if flowVar.Id.Equal(flowVarPlan.Id) {
+						flowVarPlanFound = true
+						break
+					}
+				}
+
+				if !flowVarPlanFound {
+					// remove the variable
+					_, err := sdk.DoRetryable(
+						ctx,
+						r.Client,
+						environmentID,
+						func() (interface{}, *http.Response, error) {
+							return r.Client.DeleteVariableWithResponse(environmentID, flowVar.Id.ValueString())
+						},
+					)
+					if err != nil {
+						resp.Diagnostics.AddError(
+							"Error removing flow variable",
+							fmt.Sprintf("Error removing flow variable as part of flow update: %s", err),
+						)
+					}
+				}
+			}
+		}
+	}
+
 	// Do an export for state
 	// Run the API call
 	sdkRes, err := sdk.DoRetryable(
@@ -1183,7 +1270,8 @@ func flowVariablesToTF(apiObject []davinci.FlowVariable) (types.Set, diag.Diagno
 			"description": framework.StringOkToTF(variable.Label, true),
 			"flow_id":     framework.StringOkToTF(variable.FlowID, true),
 			"context":     framework.StringOkToTF(variable.Context, true),
-			"type":        framework.StringToTF(variable.Type),
+			"type":        framework.StringOkToTF(variable.Fields.Type, true),
+			"value":       framework.StringOkToTF(variable.Fields.Value, true),
 			"mutable":     framework.BoolOkToTF(variable.Fields.Mutable, true),
 			"min":         framework.Int32OkToTF(variable.Fields.Min, true),
 			"max":         framework.Int32OkToTF(variable.Fields.Max, true),

internal/service/davinci/resource_flow_test.go

@@ -120,8 +120,7 @@ func testAccResourceFlow_Basic(t *testing.T, withBootstrapConfig bool) {
 				"min":     regexp.MustCompile(`^0$`),
 				"mutable": regexp.MustCompile(`^true$`),
 				"name":    regexp.MustCompile(`^fdgdfgfdg$`),
-				"type":    regexp.MustCompile(`^property$`),
-				//"type":    regexp.MustCompile(`^string$`),
+				"type":    regexp.MustCompile(`^string$`),
 			}),
 			resource.TestMatchTypeSetElemNestedAttrs(resourceFullName, "flow_variables.*", map[string]*regexp.Regexp{
 				"context": regexp.MustCompile(`^flow$`),
@@ -131,9 +130,8 @@ func testAccResourceFlow_Basic(t *testing.T, withBootstrapConfig bool) {
 				"min":     regexp.MustCompile(`^4$`),
 				"mutable": regexp.MustCompile(`^true$`),
 				"name":    regexp.MustCompile(`^test123$`),
-				"type":    regexp.MustCompile(`^property$`),
-				//"type":    regexp.MustCompile(`^number$`),
-				//"value":   regexp.MustCompile(`^10$`),
+				"type":    regexp.MustCompile(`^number$`),
+				"value":   regexp.MustCompile(`^10$`),
 			}),
 		),
 	}
@@ -194,21 +192,21 @@ func testAccResourceFlow_Basic(t *testing.T, withBootstrapConfig bool) {
 		Steps: []resource.TestStep{
 			// Create full from scratch
 			fullStep,
+			minimalStep,
 			{
-				Config:  fullStepHcl,
+				Config:  minimalStepHcl,
 				Destroy: true,
 			},
 			// Create minimal from scratch
 			minimalStep,
+			fullStep,
 			{
-				Config:  minimalStepHcl,
+				Config:  fullStepHcl,
 				Destroy: true,
 			},
 			// Test updates
-			fullStep,
 			minimalStep,
 			updateStep,
-			fullStep,
 			// Test importing the resource
 			{
 				ResourceName: resourceFullName,
@@ -372,6 +370,7 @@ func testAccResourceFlow_ConnectionSubflowLinks_WithMappingIDs(t *testing.T, wit
 					"connection_link",
 					"subflow_link",
 					"flow_json",
+					"flow_export_json",
 				},
 			},
 		},
@@ -503,6 +502,7 @@ func testAccResourceFlow_ConnectionSubflowLinks_WithoutMappingIDs(t *testing.T,
 					"connection_link",
 					"subflow_link",
 					"flow_json",
+					"flow_export_json",
 				},
 			},
 		},

internal/service/davinci/resource_variable.go

@@ -7,6 +7,7 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -361,3 +362,57 @@ func getVariableAttributes(d *schema.ResourceData) dv.VariablePayload {
 	}
 	return variablePayload
 }
+
+// Framework
+type VariableResourceModel struct {
+	Id          types.String `tfsdk:"id"`
+	Name        types.String `tfsdk:"name"`
+	Description types.String `tfsdk:"description"`
+	FlowId      types.String `tfsdk:"flow_id"`
+	Context     types.String `tfsdk:"context"`
+	Type        types.String `tfsdk:"type"`
+	Value       types.String `tfsdk:"value"`
+	Mutable     types.Bool   `tfsdk:"mutable"`
+	Min         types.Int64  `tfsdk:"min"`
+	Max         types.Int64  `tfsdk:"max"`
+}
+
+func (p *VariableResourceModel) expand() *dv.VariablePayload {
+
+	data := dv.VariablePayload{
+		Context: p.Context.ValueString(),
+		Type:    p.Type.ValueString(),
+	}
+
+	if !p.Name.IsNull() {
+		data.Name = p.Name.ValueStringPointer()
+	}
+
+	if !p.Description.IsNull() {
+		data.Description = p.Description.ValueStringPointer()
+	}
+
+	if !p.FlowId.IsNull() {
+		data.FlowId = p.FlowId.ValueStringPointer()
+	}
+
+	if !p.Value.IsNull() {
+		data.Value = p.Value.ValueStringPointer()
+	}
+
+	if !p.Mutable.IsNull() {
+		data.Mutable = p.Mutable.ValueBoolPointer()
+	}
+
+	if !p.Min.IsNull() {
+		min := int(p.Min.ValueInt64())
+		data.Min = &min
+	}
+
+	if !p.Max.IsNull() {
+		max := int(p.Max.ValueInt64())
+		data.Max = &max
+	}
+
+	return &data
+}