Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix tests and relax warehouse validations #2959

Merged
merged 6 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions MIGRATION_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ Added a new datasource enabling querying and filtering network policies. Notes:
The additional parameters call **DESC NETWORK POLICY** (with `with_describe` turned on) **per network policy** returned by **SHOW NETWORK POLICIES**.
It's important to limit the records and calls to Snowflake to the minimum. That's why we recommend assessing which information you need from the data source and then providing strong filters and turning off additional fields for better plan performance.

### *(fix)* snowflake_warehouse resource

Because of the issue [#2948](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2948), we are relaxing the validations for the Snowflake parameter values. Read more in [CHANGES_BEFORE_V1.md](v1-preparations/CHANGES_BEFORE_V1.md#validations).

## v0.92.0 ➞ v0.93.0

### general changes
Expand Down
22 changes: 12 additions & 10 deletions pkg/resources/cortex_search_service_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,27 +14,27 @@ import (
)

func TestAcc_CortexSearchService_basic(t *testing.T) {
t.Skipf("Skipped for now because of the <err: 090105 (22000): Cannot perform operation. This session does not have a current database. Call 'USE DATABASE', or use a qualified name.> problem.")
resourceName := "snowflake_cortex_search_service.css"
id := acc.TestClient().Ids.RandomSchemaObjectIdentifier()
tableId := acc.TestClient().Ids.RandomSchemaObjectIdentifier()
newWarehouseName := acc.TestClient().Ids.Alpha()
m := func() map[string]config.Variable {
return map[string]config.Variable{
"name": config.StringVariable(id.Name()),
"on": config.StringVariable("id"),
"on": config.StringVariable("SOME_TEXT"),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"warehouse": config.StringVariable(acc.TestWarehouseName),
"query": config.StringVariable(fmt.Sprintf(`select "id" from %s"`, tableId.FullyQualifiedName())),
"query": config.StringVariable(fmt.Sprintf("select SOME_TEXT from %s", tableId.FullyQualifiedName())),
"comment": config.StringVariable("Terraform acceptance test"),
"table_name": config.StringVariable(tableId.Name()),
}
}
variableSet2 := m()
variableSet2["attributes"] = config.SetVariable(config.StringVariable("type"))
variableSet2["attributes"] = config.SetVariable(config.StringVariable("SOME_OTHER_TEXT"))
variableSet2["warehouse"] = config.StringVariable(newWarehouseName)
variableSet2["comment"] = config.StringVariable("Terraform acceptance test - updated")
variableSet2["query"] = config.StringVariable(fmt.Sprintf("select SOME_TEXT, SOME_OTHER_TEXT from %s", tableId.FullyQualifiedName()))

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
Expand All @@ -56,16 +56,15 @@ func TestAcc_CortexSearchService_basic(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "name", id.Name()),
resource.TestCheckResourceAttr(resourceName, "database", acc.TestDatabaseName),
resource.TestCheckResourceAttr(resourceName, "schema", acc.TestSchemaName),
resource.TestCheckResourceAttr(resourceName, "on", "id"),
resource.TestCheckResourceAttr(resourceName, "on", "SOME_TEXT"),
resource.TestCheckNoResourceAttr(resourceName, "attributes"),
resource.TestCheckResourceAttr(resourceName, "warehouse", acc.TestWarehouseName),
resource.TestCheckResourceAttr(resourceName, "target_lag", "2 minutes"),
resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test"),
resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select \"id\" from %s", tableId.FullyQualifiedName())),
resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select SOME_TEXT from %s", tableId.FullyQualifiedName())),
resource.TestCheckResourceAttrSet(resourceName, "created_on"),
),
},

{
ConfigDirectory: config.TestStepDirectory(),
ConfigVariables: variableSet2,
Expand All @@ -78,12 +77,13 @@ func TestAcc_CortexSearchService_basic(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "name", id.Name()),
resource.TestCheckResourceAttr(resourceName, "database", acc.TestDatabaseName),
resource.TestCheckResourceAttr(resourceName, "schema", acc.TestSchemaName),
resource.TestCheckResourceAttr(resourceName, "on", "id"),
resource.TestCheckResourceAttr(resourceName, "attributes", "type"),
resource.TestCheckResourceAttr(resourceName, "on", "SOME_TEXT"),
resource.TestCheckResourceAttr(resourceName, "attributes.#", "1"),
resource.TestCheckResourceAttr(resourceName, "attributes.0", "SOME_OTHER_TEXT"),
resource.TestCheckResourceAttr(resourceName, "warehouse", newWarehouseName),
resource.TestCheckResourceAttr(resourceName, "target_lag", "2 minutes"),
resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test - updated"),
resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select \"id\" from %s", tableId.FullyQualifiedName())),
resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select SOME_TEXT, SOME_OTHER_TEXT from %s", tableId.FullyQualifiedName())),
resource.TestCheckResourceAttrSet(resourceName, "created_on"),
),
},
Expand All @@ -94,6 +94,8 @@ func TestAcc_CortexSearchService_basic(t *testing.T) {
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
// currently not set in read because the early implementation on Snowflake side did not return these values on SHOW/DESCRIBE
ImportStateVerifyIgnore: []string{"attributes", "on", "query", "target_lag", "warehouse"},
},
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@ resource "snowflake_table" "t" {
schema = var.schema
name = var.table_name
change_tracking = true

column {
name = "id"
name = "ID"
type = "NUMBER(38,0)"
}

column {
name = "SOME_TEXT"
type = "VARCHAR"
}
}

resource "snowflake_cortex_search_service" "css" {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,19 @@ resource "snowflake_table" "t" {
schema = var.schema
name = var.table_name
change_tracking = true

column {
name = "id"
name = "ID"
type = "NUMBER(38,0)"
}

column {
name = "SOME_TEXT"
type = "VARCHAR"
}

column {
name = "type"
name = "SOME_OTHER_TEXT"
type = "VARCHAR(32)"
}
}
Expand Down
23 changes: 19 additions & 4 deletions pkg/resources/testdata/TestAcc_CortexSearchService_basic/3/test.tf
Original file line number Diff line number Diff line change
@@ -1,20 +1,35 @@

resource "snowflake_warehouse" "t" {
name = var.warehouse
warehouse_size = "XSMALL"
}

resource "snowflake_table" "t" {
database = var.database
schema = var.schema
name = var.table_name
change_tracking = true

column {
name = "id"
name = "ID"
type = "NUMBER(38,0)"
}

column {
name = "SOME_TEXT"
type = "VARCHAR"
}

column {
name = "SOME_OTHER_TEXT"
type = "VARCHAR(32)"
}
}

resource "snowflake_cortex_search_service" "css" {
depends_on = [snowflake_table.t]
name = var.name
depends_on = [snowflake_table.t, snowflake_warehouse.t]
on = var.on
attributes = var.attributes
name = var.name
database = var.database
schema = var.schema
target_lag = "2 minutes"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ variable "on" {
type = string
}

variable "attributes" {
type = set(string)
}

variable "database" {
type = string
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/resources/warehouse.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ var warehouseSchema = map[string]*schema.Schema{
"max_cluster_count": {
Type: schema.TypeInt,
Optional: true,
ValidateDiagFunc: validation.ToDiagFunc(validation.IntBetween(1, 10)),
ValidateDiagFunc: validation.ToDiagFunc(validation.IntAtLeast(1)),
DiffSuppressFunc: IgnoreChangeToCurrentSnowflakeValueInShow("max_cluster_count"),
Description: "Specifies the maximum number of server clusters for the warehouse.",
},
"min_cluster_count": {
Type: schema.TypeInt,
Optional: true,
ValidateDiagFunc: validation.ToDiagFunc(validation.IntBetween(1, 10)),
ValidateDiagFunc: validation.ToDiagFunc(validation.IntAtLeast(1)),
DiffSuppressFunc: IgnoreChangeToCurrentSnowflakeValueInShow("min_cluster_count"),
Description: "Specifies the minimum number of server clusters for the warehouse (only applies to multi-cluster warehouses).",
},
Expand Down
8 changes: 4 additions & 4 deletions pkg/resources/warehouse_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -665,12 +665,12 @@ func TestAcc_Warehouse_Validations(t *testing.T) {
ExpectError: regexp.MustCompile("invalid warehouse size: SMALLa"),
},
{
Config: warehouseConfigWithMaxClusterCount(id.Name(), 100),
ExpectError: regexp.MustCompile(`expected max_cluster_count to be in the range \(1 - 10\), got 100`),
Config: warehouseConfigWithMaxClusterCount(id.Name(), 0),
ExpectError: regexp.MustCompile(`expected max_cluster_count to be at least \(1\), got 0`),
},
{
Config: warehouseConfigWithMinClusterCount(id.Name(), 100),
ExpectError: regexp.MustCompile(`expected min_cluster_count to be in the range \(1 - 10\), got 100`),
Config: warehouseConfigWithMinClusterCount(id.Name(), 0),
ExpectError: regexp.MustCompile(`expected min_cluster_count to be at least \(1\), got 0`),
},
{
Config: warehouseConfigWithScalingPolicy(id.Name(), "unknown"),
Expand Down
8 changes: 4 additions & 4 deletions pkg/sdk/testint/application_packages_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ func TestInt_ApplicationPackagesVersionAndReleaseDirective(t *testing.T) {
e := createApplicationPackageHandle(t)
stage, stageCleanup := testClientHelper().Stage.CreateStage(t)
t.Cleanup(stageCleanup)
testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "manifest.yml", "")
testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "setup.sql", "")
testClientHelper().Stage.PutOnStage(t, stage.ID(), "manifest.yml")
testClientHelper().Stage.PutOnStage(t, stage.ID(), "setup.sql")

version := "V001"
using := "@" + stage.ID().FullyQualifiedName()
Expand Down Expand Up @@ -253,8 +253,8 @@ func TestInt_ApplicationPackagesVersionAndReleaseDirective(t *testing.T) {
e := createApplicationPackageHandle(t)
stage, stageCleanup := testClientHelper().Stage.CreateStage(t)
t.Cleanup(stageCleanup)
testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "manifest.yml", "")
testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "setup.sql", "")
testClientHelper().Stage.PutOnStage(t, stage.ID(), "manifest.yml")
testClientHelper().Stage.PutOnStage(t, stage.ID(), "setup.sql")

version := "V001"
using := "@" + stage.ID().FullyQualifiedName()
Expand Down
42 changes: 40 additions & 2 deletions pkg/sdk/testint/grants_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ func TestInt_GrantAndRevokePrivilegesToAccountRole(t *testing.T) {
t.Run("on all: cortex search service", func(t *testing.T) {
roleTest, roleCleanup := testClientHelper().Role.CreateRole(t)
t.Cleanup(roleCleanup)
table, tableTestCleanup := testClientHelper().Table.CreateTableWithPredefinedColumns(t)
t.Cleanup(tableTestCleanup)
cortex, cortexCleanup := testClientHelper().CortexSearchService.CreateCortexSearchService(t, table.ID())
t.Cleanup(cortexCleanup)

privileges := &sdk.AccountRoleGrantPrivileges{
SchemaObjectPrivileges: []sdk.SchemaObjectPrivilege{sdk.SchemaObjectPrivilegeUsage},
Expand All @@ -254,7 +258,31 @@ func TestInt_GrantAndRevokePrivilegesToAccountRole(t *testing.T) {
},
}
err := client.Grants.GrantPrivilegesToAccountRole(ctx, privileges, on, roleTest.ID(), nil)
require.ErrorContains(t, err, "unexpected 'SEARCH'")
require.NoError(t, err)

grants, err := client.Grants.Show(ctx, &sdk.ShowGrantOptions{
To: &sdk.ShowGrantsTo{
Role: roleTest.ID(),
},
})
require.NoError(t, err)
usagePrivilege, err := collections.FindOne[sdk.Grant](grants, func(g sdk.Grant) bool {
return g.Privilege == sdk.SchemaObjectPrivilegeUsage.String()
})
require.NoError(t, err)
assert.Equal(t, cortex.ID().FullyQualifiedName(), usagePrivilege.Name.FullyQualifiedName())
assert.Equal(t, sdk.ObjectTypeCortexSearchService, usagePrivilege.GrantedOn)

// now revoke and verify that the grant(s) are gone
err = client.Grants.RevokePrivilegesFromAccountRole(ctx, privileges, on, roleTest.ID(), nil)
require.NoError(t, err)
grants, err = client.Grants.Show(ctx, &sdk.ShowGrantOptions{
To: &sdk.ShowGrantsTo{
Role: roleTest.ID(),
},
})
require.NoError(t, err)
assert.Equal(t, 0, len(grants))
})

t.Run("on future schema object", func(t *testing.T) {
Expand Down Expand Up @@ -1117,6 +1145,10 @@ func TestInt_GrantOwnership(t *testing.T) {
return ownershipGrantOnObject(sdk.ObjectTypePipe, pipe.ID())
}

ownershipGrantOnCortexSearchService := func(cortexSearchService *sdk.CortexSearchService) sdk.OwnershipGrantOn {
return ownershipGrantOnObject(sdk.ObjectTypeCortexSearchService, cortexSearchService.ID())
}

ownershipGrantOnTask := func(task *sdk.Task) sdk.OwnershipGrantOn {
return ownershipGrantOnObject(sdk.ObjectTypeTask, task.ID())
}
Expand Down Expand Up @@ -1281,7 +1313,13 @@ func TestInt_GrantOwnership(t *testing.T) {
},
new(sdk.GrantOwnershipOptions),
)
require.ErrorContains(t, err, "Invalid object type 'CORTEX_SEARCH_SERVICE' for privilege 'OWNERSHIP'")
require.NoError(t, err)
checkOwnershipOnObjectToRole(t, ownershipGrantOnCortexSearchService(cortex), role.ID())

currentRole := testClientHelper().Context.CurrentRole(t)

grantOwnershipToRole(t, currentRole, ownershipGrantOnCortexSearchService(cortex), nil)
checkOwnershipOnObjectToRole(t, ownershipGrantOnCortexSearchService(cortex), currentRole)
})

t.Run("on pipe - with operate and monitor privileges granted", func(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions pkg/sdk/testint/users_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func TestInt_Users(t *testing.T) {
HasQueryTag("some_tag").
HasQuotedIdentifiersIgnoreCase(true).
HasRowsPerResultset(2).
HasS3StageVpceDnsName("vpce-some_dns-vpce.amazonaws.com").
HasS3StageVpceDnsName("vpce-id.s3.region.vpce.amazonaws.com").
HasSearchPath("$public, $current").
HasSimulatedDataSharingConsumer("some_consumer").
HasStatementQueuedTimeoutInSeconds(10).
Expand Down Expand Up @@ -485,7 +485,7 @@ func TestInt_Users(t *testing.T) {
QueryTag: sdk.String("some_tag"),
QuotedIdentifiersIgnoreCase: sdk.Bool(true),
RowsPerResultset: sdk.Int(2),
S3StageVpceDnsName: sdk.String("vpce-some_dns-vpce.amazonaws.com"),
S3StageVpceDnsName: sdk.String("vpce-id.s3.region.vpce.amazonaws.com"),
SearchPath: sdk.String("$public, $current"),
SimulatedDataSharingConsumer: sdk.String("some_consumer"),
StatementQueuedTimeoutInSeconds: sdk.Int(10),
Expand Down Expand Up @@ -700,7 +700,7 @@ func TestInt_Users(t *testing.T) {
QueryTag: sdk.String("some_tag"),
QuotedIdentifiersIgnoreCase: sdk.Bool(true),
RowsPerResultset: sdk.Int(2),
S3StageVpceDnsName: sdk.String("vpce-some_dns-vpce.amazonaws.com"),
S3StageVpceDnsName: sdk.String("vpce-id.s3.region.vpce.amazonaws.com"),
SearchPath: sdk.String("$public, $current"),
SimulatedDataSharingConsumer: sdk.String("some_consumer"),
StatementQueuedTimeoutInSeconds: sdk.Int(10),
Expand Down
6 changes: 6 additions & 0 deletions v1-preparations/CHANGES_BEFORE_V1.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ create a resource with slightly different configuration in Snowflake (depending
current account configuration, and most-likely other factors). That is why we recommend setting optional fields where
you want to ensure that the specified value has been set on the Snowflake side.

## Validations

This point connects with the one on about the [default values](#default-values). First of all, we want to reduce the coupling between Snowflake and the provider. Secondly, some of the value limits are soft (consult issues [#2948](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2948) and [#1919](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/1919)) which makes it difficult to align provider validations with the custom setups. Lastly, some values depend on the Snowflake edition used.

Because of all that, we plan to reduce the number of validations (mostly numeric) on the provider side. We won't get rid of them entirely, so that successful plans but apply failures can be limited, but please be aware that you may encounter them.

## "Empty" values
The [Terraform SDK v2](https://github.com/hashicorp/terraform-plugin-sdk) that is currently used in our provider detects the presence of the attribute based on its non-zero Golang value. This means, that it is not possible to distinguish the removal of the value inside a config from setting it explicitely to a zero value, e.g. `0` for the numeric value (check [this thread](https://discuss.hashicorp.com/t/is-it-possible-to-differentiate-between-a-zero-value-and-a-removed-property-in-the-terraform-provider-sdk/43131)). Before we migrate to the new recommended [Terraform Plugin Framework](https://github.com/hashicorp/terraform-plugin-framework) we want to handle such cases the same way inside the provider. It means that:
- boolean attributes will be migrated to the string attributes with two values: `"true"` and `"false"` settable in the config and the special third value `"default"` that will mean, that the given attribute is not set inside the config.
Expand Down
Loading