Repository: chanzuckerberg/terraform-provider-snowflake
Pull request #795 (2021-12-22 23:41:02)

fix: tagging for db, external_table, schema
tagging for database, external_table and schema were not working. this is caused by two separate problems 1) the command for updating tags never runs and 2) the command itself was incorrect. This PR aims to fix both of these issues. In addition, tags were previously marked as force new, when they really shouldn't be. It would be very bad to accidentally delete a database just because the tags are getting changed, for example. A consequence of removing force new means that we now have to implement update command for external_table, which uses tags, but does not have its own update command. 

## Test Plan
* [ ] acceptance tests

## References

* 

alldoami: /ok-to-test sha=3e8b1cf
alldoami: Could you add tests to test your new update functions?
github-actions[bot]: Integration tests success for [3e8b1cf](https://github.com/chanzuckerberg/terraform-provider-snowflake/actions/runs/1613501144)
sfc-gh-swinkler: > Could you add tests to test your new update functions?

sure i will work on this
sfc-gh-swinkler: i added a test case for update and also simplified the update to only handle tags, as that is the purpose of this PR. leaving force new for all other attributes, as that should be handled in a separate pr.
alldoami: /ok-to-test sha=dffc3ac
github-actions[bot]: Integration tests success for [dffc3ac](https://github.com/chanzuckerberg/terraform-provider-snowflake/actions/runs/1650759533)

Patch:
diff --git a/pkg/resources/database.go b/pkg/resources/database.go
index aec76ebf..fae960ec 100644
--- a/pkg/resources/database.go
+++ b/pkg/resources/database.go
@@ -44,7 +44,7 @@ var databaseSchema = map[string]*schema.Schema{
 	"tag": tagReferenceSchema,
 }
 
-var databaseProperties = []string{"comment", "data_retention_time_in_days"}
+var databaseProperties = []string{"comment", "data_retention_time_in_days", "tag"}
 
 // Database returns a pointer to the resource representing a database
 func Database() *schema.Resource {
@@ -152,6 +152,7 @@ func ReadDatabase(d *schema.ResourceData, meta interface{}) error {
 }
 
 func UpdateDatabase(d *schema.ResourceData, meta interface{}) error {
+	log.Printf("[DEBUG] updating database %v", d.Id())
 	return UpdateResource("database", databaseProperties, databaseSchema, snowflake.Database, ReadDatabase)(d, meta)
 }
 
diff --git a/pkg/resources/external_table.go b/pkg/resources/external_table.go
index 4a98a882..ac359370 100644
--- a/pkg/resources/external_table.go
+++ b/pkg/resources/external_table.go
@@ -135,6 +135,7 @@ func ExternalTable() *schema.Resource {
 	return &schema.Resource{
 		Create: CreateExternalTable,
 		Read:   ReadExternalTable,
+		Update: UpdateExternalTable,
 		Delete: DeleteExternalTable,
 
 		Schema: externalTableSchema,
@@ -291,6 +292,41 @@ func ReadExternalTable(data *schema.ResourceData, meta interface{}) error {
 	return nil
 }
 
+// UpdateExternalTable implements schema.UpdateFunc
+func UpdateExternalTable(data *schema.ResourceData, meta interface{}) error {
+	db := meta.(*sql.DB)
+	database := data.Get("database").(string)
+	dbSchema := data.Get("schema").(string)
+	name := data.Get("name").(string)
+
+	builder := snowflake.ExternalTable(name, database, dbSchema)
+
+	if data.HasChange("tag") {
+		v := data.Get("tag")
+		tags := getTags(v)
+		builder.WithTags(tags.toSnowflakeTagValues())
+	}
+
+	stmt := builder.Update()
+	err := snowflake.Exec(db, stmt)
+	if err != nil {
+		return errors.Wrapf(err, "error updating externalTable %v", name)
+	}
+
+	externalTableID := &externalTableID{
+		DatabaseName:      database,
+		SchemaName:        dbSchema,
+		ExternalTableName: name,
+	}
+	dataIDInput, err := externalTableID.String()
+	if err != nil {
+		return err
+	}
+	data.SetId(dataIDInput)
+
+	return ReadExternalTable(data, meta)
+}
+
 // DeleteExternalTable implements schema.DeleteFunc
 func DeleteExternalTable(data *schema.ResourceData, meta interface{}) error {
 	db := meta.(*sql.DB)
diff --git a/pkg/resources/resource.go b/pkg/resources/resource.go
index 23edbe95..6d314970 100644
--- a/pkg/resources/resource.go
+++ b/pkg/resources/resource.go
@@ -2,6 +2,7 @@ package resources
 
 import (
 	"database/sql"
+	"log"
 
 	"github.com/chanzuckerberg/terraform-provider-snowflake/pkg/snowflake"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -34,15 +35,13 @@ func CreateResource(
 				case schema.TypeInt:
 					valInt := val.(int)
 					qb.SetInt(field, valInt)
-				case schema.TypeList:
-					tags, ok := val.([]snowflake.TagValue)
-					if !ok {
-						continue
-					}
-					qb.SetTags(tags)
 				}
 			}
 		}
+		if v, ok := d.GetOk("tag"); ok {
+			tags := getTags(v)
+			qb.SetTags(tags.toSnowflakeTagValues())
+		}
 		err := snowflake.Exec(db, qb.Statement())
 
 		if err != nil {
@@ -103,12 +102,19 @@ func UpdateResource(
 					qb.SetInt(field, valInt)
 				}
 			}
+			if d.HasChange("tag") {
+				log.Printf("[DEBUG] updating tags")
+				v := d.Get("tag")
+				tags := getTags(v)
+				qb.SetTags(tags.toSnowflakeTagValues())
+			}
 
 			err := snowflake.Exec(db, qb.Statement())
 			if err != nil {
 				return errors.Wrapf(err, "error altering %s", t)
 			}
 		}
+		log.Printf("[DEBUG] performing read")
 		return read(d, meta)
 	}
 }
diff --git a/pkg/resources/tag.go b/pkg/resources/tag.go
index cf28e65e..f073737a 100644
--- a/pkg/resources/tag.go
+++ b/pkg/resources/tag.go
@@ -48,7 +48,6 @@ var tagReferenceSchema = &schema.Schema{
 	Type:        schema.TypeList,
 	Required:    false,
 	Optional:    true,
-	ForceNew:    true,
 	MinItems:    0,
 	Description: "Definitions of a tag to associate with the resource.",
 	Elem: &schema.Resource{
@@ -56,27 +55,23 @@ var tagReferenceSchema = &schema.Schema{
 			"name": {
 				Type:        schema.TypeString,
 				Required:    true,
-				ForceNew:    true,
 				Description: "Tag name, e.g. department.",
 			},
 			"value": {
 				Type:        schema.TypeString,
 				Required:    true,
-				ForceNew:    true,
 				Description: "Tag value, e.g. marketing_info.",
 			},
 			"database": {
 				Type:        schema.TypeString,
 				Required:    false,
 				Optional:    true,
-				ForceNew:    true,
 				Description: "Name of the database that the tag was created in.",
 			},
 			"schema": {
 				Type:        schema.TypeString,
 				Required:    false,
 				Optional:    true,
-				ForceNew:    true,
 				Description: "Name of the schema that the tag was created in.",
 			},
 		},
diff --git a/pkg/snowflake/external_table.go b/pkg/snowflake/external_table.go
index 64209434..efebcdad 100644
--- a/pkg/snowflake/external_table.go
+++ b/pkg/snowflake/external_table.go
@@ -155,6 +155,22 @@ func (tb *ExternalTableBuilder) Create() string {
 		q.WriteString(fmt.Sprintf(` COMMENT = '%v'`, EscapeString(tb.comment)))
 	}
 
+	if len(tb.tags) > 0 {
+		q.WriteString(fmt.Sprintf(` WITH TAG (%s)`, tb.GetTagValueString()))
+	}
+
+	return q.String()
+}
+
+// Update returns the SQL statement required to update an externalTable
+func (tb *ExternalTableBuilder) Update() string {
+	q := strings.Builder{}
+	q.WriteString(fmt.Sprintf(`ALTER EXTERNAL TABLE %v`, tb.QualifiedName()))
+
+	if len(tb.tags) > 0 {
+		q.WriteString(fmt.Sprintf(` TAG %s`, tb.GetTagValueString()))
+	}
+
 	return q.String()
 }
 
@@ -168,6 +184,22 @@ func (tb *ExternalTableBuilder) Show() string {
 	return fmt.Sprintf(`SHOW EXTERNAL TABLES LIKE '%v' IN SCHEMA "%v"."%v"`, tb.name, tb.db, tb.schema)
 }
 
+func (tb *ExternalTableBuilder) GetTagValueString() string {
+	var q strings.Builder
+	for _, v := range tb.tags {
+		fmt.Println(v)
+		if v.Schema != "" {
+			if v.Database != "" {
+				q.WriteString(fmt.Sprintf(`"%v".`, v.Database))
+			}
+			q.WriteString(fmt.Sprintf(`"%v".`, v.Schema))
+		}
+		q.WriteString(fmt.Sprintf(`"%v" = "%v", `, v.Name, v.Value))
+	}
+	return strings.TrimSuffix(q.String(), ", ")
+}
+
+
 type externalTable struct {
 	CreatedOn         sql.NullString `db:"created_on"`
 	ExternalTableName sql.NullString `db:"name"`
diff --git a/pkg/snowflake/generic.go b/pkg/snowflake/generic.go
index 86772e5b..83507f26 100644
--- a/pkg/snowflake/generic.go
+++ b/pkg/snowflake/generic.go
@@ -108,6 +108,21 @@ func (b *AlterPropertiesBuilder) SetTags(tags []TagValue) {
 	b.tags = tags
 }
 
+func (ab *AlterPropertiesBuilder) GetTagValueString() string {
+	var q strings.Builder
+	for _, v := range ab.tags {
+		fmt.Println(v)
+		if v.Schema != "" {
+			if v.Database != "" {
+				q.WriteString(fmt.Sprintf(`"%v".`, v.Database))
+			}
+			q.WriteString(fmt.Sprintf(`"%v".`, v.Schema))
+		}
+		q.WriteString(fmt.Sprintf(`"%v" = "%v", `, v.Name, v.Value))
+	}
+	return strings.TrimSuffix(q.String(), ", ")
+}
+
 func (ab *AlterPropertiesBuilder) Statement() string {
 	var sb strings.Builder
 	sb.WriteString(fmt.Sprintf(`ALTER %s "%s" SET`, ab.entityType, ab.name)) // TODO handle error
@@ -134,10 +149,9 @@ func (ab *AlterPropertiesBuilder) Statement() string {
 		sb.WriteString(fmt.Sprintf(" %s=%.2f", strings.ToUpper(k), ab.floatProperties[k]))
 	}
 
-	for _, t := range sortTags(ab.tags) {
-		sb.WriteString(fmt.Sprintf(` TAG "%v"."%v"."%v" = "%v"`, t.Database, t.Schema, t.Name, t.Value))
+	if len(ab.tags) > 0 {
+		sb.WriteString(fmt.Sprintf(` TAG %s`, ab.GetTagValueString()))
 	}
-
 	return sb.String()
 }
 
@@ -195,6 +209,21 @@ func (b *CreateBuilder) SetTags(tags []TagValue) {
 	b.tags = tags
 }
 
+func (b *CreateBuilder) GetTagValueString() string {
+	var q strings.Builder
+	for _, v := range b.tags {
+		fmt.Println(v)
+		if v.Schema != "" {
+			if v.Database != "" {
+				q.WriteString(fmt.Sprintf(`"%v".`, v.Database))
+			}
+			q.WriteString(fmt.Sprintf(`"%v".`, v.Schema))
+		}
+		q.WriteString(fmt.Sprintf(`"%v" = "%v", `, v.Name, v.Value))
+	}
+	return strings.TrimSuffix(q.String(), ", ")
+}
+
 func (b *CreateBuilder) Statement() string {
 	var sb strings.Builder
 	sb.WriteString(fmt.Sprintf(`CREATE %s "%s"`, b.entityType, b.name)) // TODO handle error
@@ -221,13 +250,8 @@ func (b *CreateBuilder) Statement() string {
 		sb.WriteString(fmt.Sprintf(" %s=%.2f", strings.ToUpper(k), b.floatProperties[k]))
 	}
 
-	for i, t := range sortTags(b.tags) {
-		if i == 0 {
-			sb.WriteString(` TAG`)
-		} else if i < len(b.tags)-1 {
-			sb.WriteString(`, `)
-		}
-		sb.WriteString(fmt.Sprintf(`"%v"."%v"."%v" = "%v"`, t.Database, t.Schema, t.Name, t.Value))
+	if len(b.tags) > 0 {
+		sb.WriteString(fmt.Sprintf(` WITH TAG (%s)`, b.GetTagValueString()))
 	}
 
 	return sb.String()


Test patch:
diff --git a/pkg/snowflake/external_table_test.go b/pkg/snowflake/external_table_test.go
index 4e220904..f4d6e02b 100644
--- a/pkg/snowflake/external_table_test.go
+++ b/pkg/snowflake/external_table_test.go
@@ -21,6 +21,13 @@ func TestExternalTableCreate(t *testing.T) {
 	r.Equal(s.Create(), `CREATE EXTERNAL TABLE "test_db"."test_schema"."test_table" ("column1" OBJECT AS expression1, "column2" VARCHAR AS expression2) WITH LOCATION = location REFRESH_ON_CREATE = false AUTO_REFRESH = false PATTERN = 'pattern' FILE_FORMAT = ( file format ) COMMENT = 'Test Comment'`)
 }
 
+func TestExternalTableUpdate(t *testing.T){
+	r := require.New(t)
+	s := ExternalTable("test_table", "test_db", "test_schema")
+	s.WithTags([]TagValue{{Name: "tag1", Value: "value1", Schema: "test_schema", Database: "test_db"}})
+	r.Equal(s.Update(), `ALTER EXTERNAL TABLE "test_db"."test_schema"."test_table" TAG "test_db"."test_schema"."tag1" = "value1"`)
+}
+
 func TestExternalTableDrop(t *testing.T) {
 	r := require.New(t)
 	s := ExternalTable("test_table", "test_db", "test_schema")