Skip to content

Commit

Permalink
fix: Fix warehouse read and resource monitor empty set (#2319)
Browse files Browse the repository at this point in the history 
Fix warehouse read and resource monitor empty set:
- Warehouse object parameters were not read, so external changes to them
were not recognized by the resource.
- The resource monitor was always setting Set for alter (even when no
properties were altered), which led to syntax errors on SQL generation.
- The syntax described in the
[docs](https://docs.snowflake.com/en/sql-reference/sql/alter-resource-monitor#syntax)
is not right. Added test to check `ALTER` behavior for just
`NOTIFY_USERS` altered.

Fix #2318
Fix #2316
  • Loading branch information
@sfc-gh-asawicki
sfc-gh-asawicki authored Jan 8, 2024
1 parent 9a9ea33 commit 05f96c6
Show file tree
Hide file tree
Showing 8 changed files with 192 additions and 39 deletions.
2 changes: 1 addition & 1 deletion framework/provider/resource_monitor_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ func (r *ResourceMonitorResource) update(ctx context.Context, plan *resourceMoni
for _, e := range elements {
notifiedUsers = append(notifiedUsers, sdk.NotifiedUser{Name: e.ValueString()})
}
opts.NotifyUsers = &sdk.NotifyUsers{
opts.Set.NotifyUsers = &sdk.NotifyUsers{
Users: notifiedUsers,
}
}
Expand Down
41 changes: 23 additions & 18 deletions pkg/resources/resource_monitor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,24 +317,12 @@ func UpdateResourceMonitor(d *schema.ResourceData, meta interface{}) error {
ctx := context.Background()
var runSetStatement bool

opts := sdk.AlterResourceMonitorOptions{Set: &sdk.ResourceMonitorSet{}}

if d.HasChange("notify_users") {
runSetStatement = true

userNames := expandStringList(d.Get("notify_users").(*schema.Set).List())
users := []sdk.NotifiedUser{}
for _, name := range userNames {
users = append(users, sdk.NotifiedUser{Name: name})
}
opts.NotifyUsers = &sdk.NotifyUsers{
Users: users,
}
}
opts := sdk.AlterResourceMonitorOptions{}

set := sdk.ResourceMonitorSet{}
if d.HasChange("credit_quota") {
runSetStatement = true
opts.Set.CreditQuota = sdk.Pointer(d.Get("credit_quota").(int))
set.CreditQuota = sdk.Pointer(d.Get("credit_quota").(int))
}

if d.HasChange("frequency") || d.HasChange("start_timestamp") {
Expand All @@ -343,13 +331,30 @@ func UpdateResourceMonitor(d *schema.ResourceData, meta interface{}) error {
if err != nil {
return err
}
opts.Set.Frequency = frequency
opts.Set.StartTimestamp = sdk.Pointer(d.Get("start_timestamp").(string))
set.Frequency = frequency
set.StartTimestamp = sdk.Pointer(d.Get("start_timestamp").(string))
}

if d.HasChange("end_timestamp") {
runSetStatement = true
opts.Set.EndTimestamp = sdk.Pointer(d.Get("end_timestamp").(string))
set.EndTimestamp = sdk.Pointer(d.Get("end_timestamp").(string))
}

if d.HasChange("notify_users") {
runSetStatement = true

userNames := expandStringList(d.Get("notify_users").(*schema.Set).List())
users := []sdk.NotifiedUser{}
for _, name := range userNames {
users = append(users, sdk.NotifiedUser{Name: name})
}
set.NotifyUsers = &sdk.NotifyUsers{
Users: users,
}
}

if set != (sdk.ResourceMonitorSet{}) {
opts.Set = &set
}

// If ANY of the triggers changed, we collect all triggers and set them
Expand Down
20 changes: 16 additions & 4 deletions pkg/resources/resource_monitor_acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestAcc_ResourceMonitor(t *testing.T) {
},
// CHANGE PROPERTIES
{
Config: resourceMonitorConfig2(name),
Config: resourceMonitorConfig2(name, 75),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "name", name),
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "credit_quota", "150"),
Expand All @@ -46,6 +46,18 @@ func TestAcc_ResourceMonitor(t *testing.T) {
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "suspend_immediate_trigger", "95"),
),
},
// CHANGE JUST suspend_trigger; proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2316
{
Config: resourceMonitorConfig2(name, 60),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "name", name),
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "credit_quota", "150"),
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "set_for_account", "true"),
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "notify_triggers.0", "50"),
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "suspend_trigger", "60"),
resource.TestCheckResourceAttr("snowflake_resource_monitor.test", "suspend_immediate_trigger", "95"),
),
},
// IMPORT
{
ResourceName: "snowflake_resource_monitor.test",
Expand Down Expand Up @@ -198,7 +210,7 @@ resource "snowflake_resource_monitor" "test" {
`, accName)
}

func resourceMonitorConfig2(accName string) string {
func resourceMonitorConfig2(accName string, suspendTrigger int) string {
return fmt.Sprintf(`
resource "snowflake_warehouse" "warehouse" {
name = "test"
Expand All @@ -212,10 +224,10 @@ resource "snowflake_resource_monitor" "test" {
set_for_account = true
notify_triggers = [50]
warehouses = []
suspend_trigger = 75
suspend_trigger = %d
suspend_immediate_trigger = 95
}
`, accName)
`, accName, suspendTrigger)
}

// TestAcc_ResourceMonitor_issue2167 proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2167 issue.
Expand Down
38 changes: 38 additions & 0 deletions pkg/resources/warehouse.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"database/sql"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/logging"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
snowflakevalidation "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/validation"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -264,6 +265,12 @@ func ReadWarehouse(d *schema.ResourceData, meta interface{}) error {
if err = d.Set("enable_query_acceleration", w.EnableQueryAcceleration); err != nil {
return err
}

err = readWarehouseObjectProperties(d, id, client, ctx)
if err != nil {
return err
}

if w.EnableQueryAcceleration {
if err = d.Set("query_acceleration_max_scale_factor", w.QueryAccelerationMaxScaleFactor); err != nil {
return err
Expand All @@ -273,6 +280,37 @@ func ReadWarehouse(d *schema.ResourceData, meta interface{}) error {
return nil
}

func readWarehouseObjectProperties(d *schema.ResourceData, warehouseId sdk.AccountObjectIdentifier, client *sdk.Client, ctx context.Context) error {
statementTimeoutInSecondsParameter, err := client.Parameters.ShowObjectParameter(ctx, "STATEMENT_TIMEOUT_IN_SECONDS", sdk.Object{ObjectType: sdk.ObjectTypeWarehouse, Name: warehouseId})
if err != nil {
return err
}
logging.DebugLogger.Printf("[DEBUG] STATEMENT_TIMEOUT_IN_SECONDS parameter was fetched: %v", statementTimeoutInSecondsParameter)
if err = d.Set("statement_timeout_in_seconds", sdk.ToInt(statementTimeoutInSecondsParameter.Value)); err != nil {
return err
}

statementQueuedTimeoutInSecondsParameter, err := client.Parameters.ShowObjectParameter(ctx, "STATEMENT_QUEUED_TIMEOUT_IN_SECONDS", sdk.Object{ObjectType: sdk.ObjectTypeWarehouse, Name: warehouseId})
if err != nil {
return err
}
logging.DebugLogger.Printf("[DEBUG] STATEMENT_QUEUED_TIMEOUT_IN_SECONDS parameter was fetched: %v", statementQueuedTimeoutInSecondsParameter)
if err = d.Set("statement_queued_timeout_in_seconds", sdk.ToInt(statementQueuedTimeoutInSecondsParameter.Value)); err != nil {
return err
}

maxConcurrencyLevelParameter, err := client.Parameters.ShowObjectParameter(ctx, "MAX_CONCURRENCY_LEVEL", sdk.Object{ObjectType: sdk.ObjectTypeWarehouse, Name: warehouseId})
if err != nil {
return err
}
logging.DebugLogger.Printf("[DEBUG] MAX_CONCURRENCY_LEVEL parameter was fetched: %v", maxConcurrencyLevelParameter)
if err = d.Set("max_concurrency_level", sdk.ToInt(maxConcurrencyLevelParameter.Value)); err != nil {
return err
}

return nil
}

// UpdateWarehouse implements schema.UpdateFunc.
func UpdateWarehouse(d *schema.ResourceData, meta interface{}) error {
db := meta.(*sql.DB)
Expand Down
54 changes: 51 additions & 3 deletions pkg/resources/warehouse_acceptance_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
package resources_test

import (
"context"
"fmt"
"os"
"strings"
"testing"

acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
"github.com/hashicorp/terraform-plugin-testing/plancheck"
"github.com/stretchr/testify/require"
)

func TestAcc_Warehouse(t *testing.T) {
Expand All @@ -31,6 +35,7 @@ func TestAcc_Warehouse(t *testing.T) {
resource.TestCheckResourceAttr("snowflake_warehouse.w", "comment", "test comment"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "auto_suspend", "60"),
resource.TestCheckResourceAttrSet("snowflake_warehouse.w", "warehouse_size"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "max_concurrency_level", "8"),
),
},
// RENAME
Expand All @@ -45,14 +50,45 @@ func TestAcc_Warehouse(t *testing.T) {
},
// CHANGE PROPERTIES
{
Config: wConfig2(prefix2, "X-LARGE"),
Config: wConfig2(prefix2, "X-LARGE", 20),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_warehouse.w", "name", prefix2),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "comment", "test comment 2"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "auto_suspend", "60"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "warehouse_size", "XLARGE"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "max_concurrency_level", "20"),
),
},
// CHANGE JUST max_concurrency_level
{
Config: wConfig2(prefix2, "XLARGE", 16),
ConfigPlanChecks: resource.ConfigPlanChecks{
PreApply: []plancheck.PlanCheck{plancheck.ExpectNonEmptyPlan()},
},
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_warehouse.w", "name", prefix2),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "comment", "test comment 2"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "auto_suspend", "60"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "warehouse_size", "XLARGE"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "max_concurrency_level", "16"),
),
},
// CHANGE max_concurrency_level EXTERNALLY proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2318
{
PreConfig: func() { alterWarehouseMaxConcurrencyLevelExternally(t, prefix2, 10) },
ConfigPlanChecks: resource.ConfigPlanChecks{
PreApply: []plancheck.PlanCheck{plancheck.ExpectNonEmptyPlan()},
},
Config: wConfig2(prefix2, "XLARGE", 16),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_warehouse.w", "name", prefix2),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "comment", "test comment 2"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "auto_suspend", "60"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "warehouse_size", "XLARGE"),
resource.TestCheckResourceAttr("snowflake_warehouse.w", "max_concurrency_level", "16"),
),
},

// IMPORT
{
ResourceName: "snowflake_warehouse.w",
Expand Down Expand Up @@ -112,7 +148,7 @@ resource "snowflake_warehouse" "w" {
return fmt.Sprintf(s, prefix)
}

func wConfig2(prefix string, size string) string {
func wConfig2(prefix string, size string, maxConcurrencyLevel int) string {
s := `
resource "snowflake_warehouse" "w" {
name = "%s"
Expand All @@ -126,9 +162,10 @@ resource "snowflake_warehouse" "w" {
auto_resume = true
initially_suspended = true
wait_for_provisioning = false
max_concurrency_level = %d
}
`
return fmt.Sprintf(s, prefix, size)
return fmt.Sprintf(s, prefix, size, maxConcurrencyLevel)
}

func wConfigPattern(prefix string) string {
Expand All @@ -142,3 +179,14 @@ resource "snowflake_warehouse" "w2" {
`
return fmt.Sprintf(s, prefix, prefix)
}

func alterWarehouseMaxConcurrencyLevelExternally(t *testing.T, warehouseId string, level int) {
t.Helper()

client, err := sdk.NewDefaultClient()
require.NoError(t, err)
ctx := context.Background()

err = client.Warehouses.Alter(ctx, sdk.NewAccountObjectIdentifier(warehouseId), &sdk.AlterWarehouseOptions{Set: &sdk.WarehouseSet{MaxConcurrencyLevel: sdk.Int(level)}})
require.NoError(t, err)
}
21 changes: 12 additions & 9 deletions pkg/sdk/resource_monitors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ type AlterResourceMonitorOptions struct {
IfExists *bool `ddl:"keyword" sql:"IF EXISTS"`
name AccountObjectIdentifier `ddl:"identifier"`
Set *ResourceMonitorSet `ddl:"keyword" sql:"SET"`
NotifyUsers *NotifyUsers `ddl:"parameter,equals" sql:"NOTIFY_USERS"`
Triggers []TriggerDefinition `ddl:"keyword,no_comma" sql:"TRIGGERS"`
}

Expand All @@ -300,11 +299,14 @@ func (opts *AlterResourceMonitorOptions) validate() error {
if !ValidObjectIdentifier(opts.name) {
errs = append(errs, ErrInvalidObjectIdentifier)
}
if everyValueNil(opts.Set, opts.NotifyUsers, opts.Triggers) {
errs = append(errs, errAtLeastOneOf("AlterResourceMonitorOptions", "Set", "NotifyUsers", "Triggers"))
if everyValueNil(opts.Set, opts.Triggers) {
errs = append(errs, errAtLeastOneOf("AlterResourceMonitorOptions", "Set", "Triggers"))
}
if valueSet(opts.Set) {
if (opts.Set.Frequency != nil && opts.Set.StartTimestamp == nil) || (opts.Set.Frequency == nil && opts.Set.StartTimestamp != nil) {
if set := opts.Set; valueSet(set) {
if everyValueNil(set.CreditQuota, set.Frequency, set.StartTimestamp, set.EndTimestamp, set.NotifyUsers) {
errs = append(errs, errAtLeastOneOf("ResourceMonitorSet", "CreditQuota", "Frequency", "StartTimestamp", "EndTimestamp", "NotifyUsers"))
}
if (set.Frequency != nil && set.StartTimestamp == nil) || (set.Frequency == nil && set.StartTimestamp != nil) {
errs = append(errs, errors.New("must specify frequency and start time together"))
}
}
Expand All @@ -330,10 +332,11 @@ func (v *resourceMonitors) Alter(ctx context.Context, id AccountObjectIdentifier

type ResourceMonitorSet struct {
// at least one
CreditQuota *int `ddl:"parameter,equals" sql:"CREDIT_QUOTA"`
Frequency *Frequency `ddl:"parameter,equals" sql:"FREQUENCY"`
StartTimestamp *string `ddl:"parameter,equals,single_quotes" sql:"START_TIMESTAMP"`
EndTimestamp *string `ddl:"parameter,equals,single_quotes" sql:"END_TIMESTAMP"`
CreditQuota *int `ddl:"parameter,equals" sql:"CREDIT_QUOTA"`
Frequency *Frequency `ddl:"parameter,equals" sql:"FREQUENCY"`
StartTimestamp *string `ddl:"parameter,equals,single_quotes" sql:"START_TIMESTAMP"`
EndTimestamp *string `ddl:"parameter,equals,single_quotes" sql:"END_TIMESTAMP"`
NotifyUsers *NotifyUsers `ddl:"parameter,equals" sql:"NOTIFY_USERS"`
}

// dropResourceMonitorOptions is based on https://docs.snowflake.com/en/sql-reference/sql/drop-resource-monitor.
Expand Down
25 changes: 24 additions & 1 deletion pkg/sdk/resource_monitors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,15 @@ func TestResourceMonitorAlter(t *testing.T) {
opts := &AlterResourceMonitorOptions{
name: id,
}
assertOptsInvalidJoinedErrors(t, opts, errAtLeastOneOf("AlterResourceMonitorOptions", "Set", "NotifyUsers", "Triggers"))
assertOptsInvalidJoinedErrors(t, opts, errAtLeastOneOf("AlterResourceMonitorOptions", "Set", "Triggers"))
})

t.Run("validation: no option for set provided", func(t *testing.T) {
opts := &AlterResourceMonitorOptions{
name: id,
Set: &ResourceMonitorSet{},
}
assertOptsInvalidJoinedErrors(t, opts, errAtLeastOneOf("ResourceMonitorSet", "CreditQuota", "Frequency", "StartTimestamp", "EndTimestamp", "NotifyUsers"))
})

t.Run("with a single set", func(t *testing.T) {
Expand All @@ -76,6 +84,21 @@ func TestResourceMonitorAlter(t *testing.T) {
assertOptsValidAndSQLEquals(t, opts, "ALTER RESOURCE MONITOR %s SET CREDIT_QUOTA = %d", id.FullyQualifiedName(), *newCreditQuota)
})

t.Run("set notify users", func(t *testing.T) {
opts := &AlterResourceMonitorOptions{
name: id,
Set: &ResourceMonitorSet{
NotifyUsers: &NotifyUsers{
Users: []NotifiedUser{
{Name: "user1"},
{Name: "user2"},
},
},
},
}
assertOptsValidAndSQLEquals(t, opts, "ALTER RESOURCE MONITOR %s SET NOTIFY_USERS = (\"user1\", \"user2\")", id.FullyQualifiedName())
})

t.Run("with a multitple set", func(t *testing.T) {
newCreditQuota := Int(50)
newFrequency := FrequencyYearly
Expand Down
Loading

0 comments on commit 05f96c6

Please sign in to comment.