Changes after review

Snowflake-Labs · Sep 13, 2024 · 4520675 · 4520675
1 parent 86260ac
commit 4520675
Show file tree

Hide file tree

Showing 7 changed files with 221 additions and 10 deletions.
diff --git a/docs/resources/resource_monitor.md b/docs/resources/resource_monitor.md
@@ -7,9 +7,11 @@ description: |-
 
 !> **V1 release candidate** This resource was reworked and is a release candidate for the V1. We do not expect significant changes in it before the V1. We will welcome any feedback and adjust the resource if needed. Any errors reported will be resolved with a higher priority. We encourage checking this resource out before the V1 release. Please follow the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0950--v0960) to use it.
 
+~> **Note** For more details about resource monitor usage, please visit [this guide on Snowflake documentation page](https://docs.snowflake.com/en/user-guide/resource-monitors).
+
 **! Warning !** Due to Snowflake limitations, the following actions are not supported:
 - Cannot create resource monitors with only triggers set, any other attribute has to be set.
-- Once a resource monitor has at least one trigger assigned, it cannot fully unset them (has to have at least one trigger, doesn't matter of which type). It has to be re-created without triggers to fully unset them.
+- Once a resource monitor has at least one trigger assigned, it cannot fully unset them (has to have at least one trigger, doesn't matter of which type). That's why when you unset all the triggers on a resource monitor, it will be automatically recreated.
 
 # snowflake_resource_monitor (Resource)
 
@@ -18,7 +20,14 @@ description: |-
 ## Example Usage
 
 ```terraform
+// Note: Without credit quota and triggers specified in the configuration, the resource monitor is not performing any work.
+// More on resource monitor usage: https://docs.snowflake.com/en/user-guide/resource-monitors.
 resource "snowflake_resource_monitor" "minimal" {
+  name = "resource-monitor-name"
+}
+
+// Note: Resource monitors have to be attached to account or warehouse to be able to track credit usage.
+resource "snowflake_resource_monitor" "minimal_working" {
   name            = "resource-monitor-name"
   credit_quota    = 100
   suspend_trigger = 100

diff --git a/examples/resources/snowflake_resource_monitor/resource.tf b/examples/resources/snowflake_resource_monitor/resource.tf
@@ -1,4 +1,11 @@
+// Note: Without credit quota and triggers specified in the configuration, the resource monitor is not performing any work.
+// More on resource monitor usage: https://docs.snowflake.com/en/user-guide/resource-monitors.
 resource "snowflake_resource_monitor" "minimal" {
+  name = "resource-monitor-name"
+}
+
+// Note: Resource monitors have to be attached to account or warehouse to be able to track credit usage.
+resource "snowflake_resource_monitor" "minimal_working" {
   name            = "resource-monitor-name"
   credit_quota    = 100
   suspend_trigger = 100

diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go
@@ -165,3 +165,15 @@ func ParametersCustomDiff[T ~string](parametersProvider func(context.Context, Re
 		return customdiff.All(diffFunctions...)(ctx, d, meta)
 	}
 }
+
+func ForceNewIfAllKeysAreNotSet(key string, keys ...string) schema.CustomizeDiffFunc {
+	return customdiff.ForceNewIf(key, func(ctx context.Context, d *schema.ResourceDiff, meta any) bool {
+		allUnset := true
+		for _, k := range keys {
+			if _, ok := d.GetOk(k); ok {
+				allUnset = false
+			}
+		}
+		return allUnset
+	})
+}
diff --git a/pkg/resources/custom_diffs_test.go b/pkg/resources/custom_diffs_test.go
@@ -5,6 +5,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
+
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/resources"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
@@ -553,3 +555,85 @@ func Test_ComputedIfAnyAttributeChanged(t *testing.T) {
 		assert.Nil(t, diff.Attributes["computed_value"])
 	})
 }
+
+func TestForceNewIfAllKeysAreNotSet(t *testing.T) {
+	tests := []struct {
+		name           string
+		stateValue     map[string]string
+		rawConfigValue map[string]any
+		wantForceNew   bool
+	}{
+		{
+			name: "all values set to unset",
+			stateValue: map[string]string{
+				"value":  "123",
+				"value2": "string value",
+				"value3": "[one two]",
+			},
+			rawConfigValue: map[string]any{},
+			wantForceNew:   true,
+		},
+		{
+			name: "only value set to unset",
+			stateValue: map[string]string{
+				"value": "123",
+			},
+			rawConfigValue: map[string]any{},
+			wantForceNew:   true,
+		},
+		{
+			name: "only value2 set to unset",
+			stateValue: map[string]string{
+				"value2": "string value",
+			},
+			rawConfigValue: map[string]any{},
+			wantForceNew:   true,
+		},
+		{
+			name: "only value3 set to unset",
+			stateValue: map[string]string{
+				"value3": "[one two]",
+			},
+			rawConfigValue: map[string]any{},
+			// We expect here to not re-create because value3 doesn't have a custom diff on it
+			// and the rest custom diffs don't work when the values are not set.
+			wantForceNew: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := &schema.Provider{
+				ResourcesMap: map[string]*schema.Resource{
+					"test": {
+						Schema: map[string]*schema.Schema{
+							"value": {
+								Type: schema.TypeInt,
+							},
+							"value2": {
+								Type: schema.TypeString,
+							},
+							"value3": {
+								Type: schema.TypeList,
+								Elem: &schema.Schema{
+									Type: schema.TypeString,
+								},
+							},
+						},
+						CustomizeDiff: customdiff.All(
+							resources.ForceNewIfAllKeysAreNotSet("value", "value", "value2", "value3"),
+							resources.ForceNewIfAllKeysAreNotSet("value2", "value", "value2", "value3"),
+						),
+					},
+				},
+			}
+			diff := calculateDiffFromAttributes(
+				t,
+				p,
+				tt.stateValue,
+				tt.rawConfigValue,
+			)
+			assert.Equal(t, tt.wantForceNew, diff.RequiresNew())
+		})
+	}
+}
diff --git a/pkg/resources/resource_monitor.go b/pkg/resources/resource_monitor.go
@@ -109,6 +109,9 @@ func ResourceMonitor() *schema.Resource {
 
 		CustomizeDiff: customdiff.All(
 			ComputedIfAnyAttributeChanged(resourceMonitorSchema, ShowOutputAttributeName, "notify_users", "credit_quota", "frequency", "start_timestamp", "end_timestamp", "notify_triggers", "suspend_trigger", "suspend_immediate_trigger"),
+			ForceNewIfAllKeysAreNotSet("notify_triggers", "notify_triggers", "suspend_trigger", "suspend_immediate_trigger"),
+			ForceNewIfAllKeysAreNotSet("suspend_trigger", "notify_triggers", "suspend_trigger", "suspend_immediate_trigger"),
+			ForceNewIfAllKeysAreNotSet("suspend_immediate_trigger", "notify_triggers", "suspend_trigger", "suspend_immediate_trigger"),
 		),
 	}
 }
@@ -383,15 +386,8 @@ func UpdateResourceMonitor(ctx context.Context, d *schema.ResourceData, meta any
 
 		if len(triggers) > 0 {
 			opts.Triggers = triggers
-		} else {
-			return diag.Diagnostics{
-				diag.Diagnostic{
-					Severity: diag.Error,
-					Summary:  "Failed to update resource monitor.",
-					Detail:   "Due to Snowflake limitations triggers cannot be completely removed form resource monitor after having at least 1 trigger. The only way it to re-create resource monitor without any triggers specified.",
-				},
-			}
 		}
+		// Else ForceNew, because Snowflake doesn't allow fully unsetting the triggers
 	}
 
 	// This is to prevent SQL compilation errors from Snowflake, because you cannot only alter triggers.

diff --git a/pkg/resources/resource_monitor_acceptance_test.go b/pkg/resources/resource_monitor_acceptance_test.go
@@ -781,6 +781,107 @@ func TestAcc_ResourceMonitor_Issue1500_AlteringWithOnlyTriggers(t *testing.T) {
 	})
 }
 
+func TestAcc_ResourceMonitor_RemovingAllTriggers(t *testing.T) {
+	id := acc.TestClient().Ids.RandomAccountObjectIdentifier()
+
+	configModelWithNotifyTriggers := model.ResourceMonitor("test", id.Name()).
+		WithCreditQuota(100).
+		WithNotifyTriggersValue(configvariable.SetVariable(
+			configvariable.IntegerVariable(100),
+			configvariable.IntegerVariable(110),
+		))
+
+	configModelWithSuspendTrigger := model.ResourceMonitor("test", id.Name()).
+		WithCreditQuota(100).
+		WithSuspendTrigger(120)
+
+	configModelWithSuspendImmediateTrigger := model.ResourceMonitor("test", id.Name()).
+		WithCreditQuota(100).
+		WithSuspendImmediateTrigger(120)
+
+	configModelWithAllTriggers := model.ResourceMonitor("test", id.Name()).
+		WithCreditQuota(100).
+		WithNotifyTriggersValue(configvariable.SetVariable(
+			configvariable.IntegerVariable(100),
+			configvariable.IntegerVariable(110),
+		)).
+		WithSuspendTrigger(120).
+		WithSuspendImmediateTrigger(150)
+
+	configModelWithoutTriggers := model.ResourceMonitor("test", id.Name()).
+		WithCreditQuota(100)
+
+	resource.Test(t, resource.TestCase{
+		PreCheck: func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: acc.CheckDestroy(t, resources.ResourceMonitor),
+		Steps: []resource.TestStep{
+			// Config with all triggers
+			{
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithAllTriggers),
+			},
+			// No triggers (force new expected)
+			{
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("snowflake_resource_monitor.test", plancheck.ResourceActionDestroyBeforeCreate),
+					},
+				},
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithoutTriggers),
+			},
+			// Config with only notify triggers
+			{
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithNotifyTriggers),
+			},
+			// No triggers (force new expected)
+			{
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("snowflake_resource_monitor.test", plancheck.ResourceActionDestroyBeforeCreate),
+					},
+				},
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithoutTriggers),
+			},
+			// Config with only suspend trigger
+			{
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithSuspendTrigger),
+			},
+			// No triggers (force new expected)
+			{
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("snowflake_resource_monitor.test", plancheck.ResourceActionDestroyBeforeCreate),
+					},
+				},
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithoutTriggers),
+			},
+			// Config with only suspend immediate trigger
+			{
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithSuspendImmediateTrigger),
+			},
+			// No triggers (force new expected)
+			{
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("snowflake_resource_monitor.test", plancheck.ResourceActionDestroyBeforeCreate),
+					},
+				},
+				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+				Config:                   config.FromModel(t, configModelWithoutTriggers),
+			},
+		},
+	})
+}
+
 // proves that fields that were present in the previous versions are not kept in the state after the upgrade
 func TestAcc_ResourceMonitor_SetForWarehouse(t *testing.T) {
 	id := acc.TestClient().Ids.RandomAccountObjectIdentifier()

diff --git a/templates/resources/resource_monitor.md.tmpl b/templates/resources/resource_monitor.md.tmpl
@@ -11,9 +11,11 @@ description: |-
 
 !> **V1 release candidate** This resource was reworked and is a release candidate for the V1. We do not expect significant changes in it before the V1. We will welcome any feedback and adjust the resource if needed. Any errors reported will be resolved with a higher priority. We encourage checking this resource out before the V1 release. Please follow the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0950--v0960) to use it.
 
+~> **Note** For more details about resource monitor usage, please visit [this guide on Snowflake documentation page](https://docs.snowflake.com/en/user-guide/resource-monitors).
+
 **! Warning !** Due to Snowflake limitations, the following actions are not supported:
 - Cannot create resource monitors with only triggers set, any other attribute has to be set.
-- Once a resource monitor has at least one trigger assigned, it cannot fully unset them (has to have at least one trigger, doesn't matter of which type). It has to be re-created without triggers to fully unset them.
+- Once a resource monitor has at least one trigger assigned, it cannot fully unset them (has to have at least one trigger, doesn't matter of which type). That's why when you unset all the triggers on a resource monitor, it will be automatically recreated.
 
 # {{.Name}} ({{.Type}})