Skip to content

Commit

Permalink
fix: Fix task related issues (#2479)
Browse files Browse the repository at this point in the history
Related issues: #2036 #2207 #2346.

- (#2207) The reason was the change in parsing the `SHOW TASKS` response
for `after` parameter. In earlier versions, the parsing was incorrect
for the identifiers containing dot characters. The new one fixed the dot
case but Snowflake identifiers surprised us one more time, and the basic
case, without any `"` was not supported correctly.
- (#2036) There is no way to unset `when` currently in Snowflake. We
don't want to always `ForceNew` for when change. For that reason we run
forceNew conditionally using `customdiff.ForceNewIfChange`.
- (#2346) Waiting for answers but maybe this fix will also solve this
issues.
  • Loading branch information
sfc-gh-asawicki authored Feb 8, 2024
1 parent eb20051 commit 0385650
Show file tree
Hide file tree
Showing 12 changed files with 226 additions and 18 deletions.
16 changes: 8 additions & 8 deletions docs/resources/user_password_policy_attachment.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,19 @@ Specifies the password policy to use for a certain user.

```terraform
resource "snowflake_user" "user" {
name = "USER_NAME"
name = "USER_NAME"
}
resource "snowflake_password_policy" "pp" {
database = "prod"
schema = "security"
name = "default_policy"
database = "prod"
schema = "security"
name = "default_policy"
}
resource "snowflake_user_password_policy_attachment" "ppa" {
password_policy_database = snowflake_password_policy.pp.database
password_policy_schema = snowflake_password_policy.pp.schema
password_policy_name = snowflake_password_policy.pp.name
user_name = snowflake_user.user.name
password_policy_database = snowflake_password_policy.pp.database
password_policy_schema = snowflake_password_policy.pp.schema
password_policy_name = snowflake_password_policy.pp.name
user_name = snowflake_user.user.name
}
```

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
resource "snowflake_user" "user" {
name = "USER_NAME"
name = "USER_NAME"
}
resource "snowflake_password_policy" "pp" {
database = "prod"
schema = "security"
name = "default_policy"
database = "prod"
schema = "security"
name = "default_policy"
}

resource "snowflake_user_password_policy_attachment" "ppa" {
password_policy_database = snowflake_password_policy.pp.database
password_policy_schema = snowflake_password_policy.pp.schema
password_policy_name = snowflake_password_policy.pp.name
user_name = snowflake_user.user.name
password_policy_database = snowflake_password_policy.pp.database
password_policy_schema = snowflake_password_policy.pp.schema
password_policy_name = snowflake_password_policy.pp.name
user_name = snowflake_user.user.name
}
4 changes: 4 additions & 0 deletions pkg/resources/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
)
Expand Down Expand Up @@ -149,6 +150,9 @@ func Task() *schema.Resource {
Read: ReadTask,
Update: UpdateTask,
Delete: DeleteTask,
CustomizeDiff: customdiff.ForceNewIfChange("when", func(ctx context.Context, old, new, meta any) bool {
return old.(string) != "" && new.(string) == ""
}),

Schema: taskSchema,
Importer: &schema.ResourceImporter{
Expand Down
107 changes: 107 additions & 0 deletions pkg/resources/task_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@ import (
"text/template"

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/config"
"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/hashicorp/terraform-plugin-testing/terraform"
"github.com/hashicorp/terraform-plugin-testing/tfversion"
)

type (
Expand Down Expand Up @@ -668,3 +672,106 @@ func checkInt64(name, key string, value int64) func(*terraform.State) error {
return resource.TestCheckResourceAttr(name, key, fmt.Sprintf("%v", value))(state)
}
}

func TestAcc_Task_issue2207(t *testing.T) {
prefix := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)
rootName := prefix + "_root_task"
childName := prefix + "_child_task"

m := func() map[string]config.Variable {
return map[string]config.Variable{
"root_name": config.StringVariable(rootName),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"warehouse": config.StringVariable(acc.TestWarehouseName),
"child_name": config.StringVariable(childName),
"comment": config.StringVariable("abc"),
}
}
m2 := m()
m2["comment"] = config.StringVariable("def")

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: nil,
Steps: []resource.TestStep{
{
ConfigDirectory: config.TestStepDirectory(),
ConfigVariables: m(),
Check: resource.ComposeTestCheckFunc(
checkBool("snowflake_task.root_task", "enabled", true),
checkBool("snowflake_task.child_task", "enabled", true),
),
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
plancheck.ExpectEmptyPlan(),
},
},
},
// change comment
{
ConfigDirectory: acc.ConfigurationSameAsStepN(1),
ConfigVariables: m2,
Check: resource.ComposeTestCheckFunc(
checkBool("snowflake_task.root_task", "enabled", true),
checkBool("snowflake_task.child_task", "enabled", true),
),
},
},
})
}

func TestAcc_Task_issue2036(t *testing.T) {
name := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)

m := func() map[string]config.Variable {
return map[string]config.Variable{
"name": config.StringVariable(name),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"warehouse": config.StringVariable(acc.TestWarehouseName),
}
}

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: nil,
Steps: []resource.TestStep{
// create without when
{
ConfigDirectory: config.TestStepDirectory(),
ConfigVariables: m(),
Check: resource.ComposeTestCheckFunc(
checkBool("snowflake_task.test_task", "enabled", true),
resource.TestCheckResourceAttr("snowflake_task.test_task", "when", ""),
),
},
// add when
{
ConfigDirectory: config.TestStepDirectory(),
ConfigVariables: m(),
Check: resource.ComposeTestCheckFunc(
checkBool("snowflake_task.test_task", "enabled", true),
resource.TestCheckResourceAttr("snowflake_task.test_task", "when", "TRUE"),
),
},
// remove when
{
ConfigDirectory: acc.ConfigurationSameAsStepN(1),
ConfigVariables: m(),
Check: resource.ComposeTestCheckFunc(
checkBool("snowflake_task.test_task", "enabled", true),
resource.TestCheckResourceAttr("snowflake_task.test_task", "when", ""),
),
},
},
})
}
9 changes: 9 additions & 0 deletions pkg/resources/testdata/TestAcc_Task_issue2036/1/test.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
resource "snowflake_task" "test_task" {
name = var.name
database = var.database
schema = var.schema
warehouse = var.warehouse
sql_statement = "SELECT 1"
enabled = true
schedule = "5 MINUTE"
}
15 changes: 15 additions & 0 deletions pkg/resources/testdata/TestAcc_Task_issue2036/1/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
variable "database" {
type = string
}

variable "schema" {
type = string
}

variable "warehouse" {
type = string
}

variable "name" {
type = string
}
10 changes: 10 additions & 0 deletions pkg/resources/testdata/TestAcc_Task_issue2036/2/test.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
resource "snowflake_task" "test_task" {
name = var.name
database = var.database
schema = var.schema
warehouse = var.warehouse
sql_statement = "SELECT 1"
enabled = true
schedule = "5 MINUTE"
when = "TRUE"
}
15 changes: 15 additions & 0 deletions pkg/resources/testdata/TestAcc_Task_issue2036/2/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
variable "database" {
type = string
}

variable "schema" {
type = string
}

variable "warehouse" {
type = string
}

variable "name" {
type = string
}
20 changes: 20 additions & 0 deletions pkg/resources/testdata/TestAcc_Task_issue2207/1/test.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
resource "snowflake_task" "root_task" {
name = var.root_name
database = var.database
schema = var.schema
warehouse = var.warehouse
sql_statement = "SELECT 1"
enabled = true
schedule = "5 MINUTE"
}

resource "snowflake_task" "child_task" {
name = var.child_name
database = snowflake_task.root_task.database
schema = snowflake_task.root_task.schema
warehouse = snowflake_task.root_task.warehouse
sql_statement = "SELECT 1"
enabled = true
after = [snowflake_task.root_task.name]
comment = var.comment
}
23 changes: 23 additions & 0 deletions pkg/resources/testdata/TestAcc_Task_issue2207/1/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
variable "database" {
type = string
}

variable "schema" {
type = string
}

variable "warehouse" {
type = string
}

variable "root_name" {
type = string
}

variable "child_name" {
type = string
}

variable "comment" {
type = string
}
5 changes: 4 additions & 1 deletion pkg/sdk/tasks_impl_gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,10 @@ func getPredecessors(predecessors string) ([]string, error) {
for i, predecessorName := range predecessorNames {
formattedName := strings.Trim(predecessorName, "\\\"")
idx := strings.LastIndex(formattedName, "\"") + 1
if strings.LastIndex(formattedName, ".\"")+2 < idx {
// -1 because of not found +1 is 0
if idx == 0 {
idx = strings.LastIndex(formattedName, ".") + 1
} else if strings.LastIndex(formattedName, ".\"")+2 < idx {
idx++
}
formattedName = formattedName[idx:]
Expand Down
4 changes: 3 additions & 1 deletion pkg/sdk/tasks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ func Test_getPredecessors(t *testing.T) {
{predecessorsRaw: "[\n \"\\\"qgb)Z1KcNWJ(\\\".\\\"glN@JtR=7dzP$7\\\".Ls.T7-(bt{.lWd@DRWkyA6<6hNdh\"\n]", expectedPredecessors: []string{"Ls.T7-(bt{.lWd@DRWkyA6<6hNdh"}},
{predecessorsRaw: fmt.Sprintf("[\n \"\\\"a\\\".\\\"b\\\".\\\"%s\\\"\"\n]", special), expectedPredecessors: []string{special}},
{predecessorsRaw: "[\n \"\\\"a\\\".\\\"b\\\".\\\"c\\\"\",\"\\\"a\\\".\\\"b\\\".\\\"d\\\"\",\"\\\"a\\\".\\\"b\\\".\\\"e\\\"\"\n]", expectedPredecessors: []string{"c", "d", "e"}},
{predecessorsRaw: "[\"\\\"a\\\".\\\"b\\\".\\\".PHo,k:%Sz8tdx,9?23xTsgHLYxe\\\"\"]", expectedPredecessors: []string{".PHo,k:%Sz8tdx,9?23xTsgHLYxe"}},
{predecessorsRaw: `["\"a\".\"b\".\".PHo,k:%Sz8tdx,9?23xTsgHLYxe\""]`, expectedPredecessors: []string{".PHo,k:%Sz8tdx,9?23xTsgHLYxe"}},
{predecessorsRaw: `["MY_DB.MY_SCH.MY_PARENT_TASK"]`, expectedPredecessors: []string{"MY_PARENT_TASK"}},
{predecessorsRaw: `["CTG_DEV_PLK.INGESTION.COPY_FROM_KINESIS_S3_DELIVERY"]`, expectedPredecessors: []string{"COPY_FROM_KINESIS_S3_DELIVERY"}},
}
for i, tt := range tests {
t.Run(fmt.Sprintf("test number %d for input: [%s]", i, tt.predecessorsRaw), func(t *testing.T) {
Expand Down

0 comments on commit 0385650

Please sign in to comment.