Merge pull request #39734 from hashicorp/td-generate-tagtests-ignore-…

…tags

tagging tests: Adds tests for ignoring tags

.changelog/39734.txt

@@ -0,0 +1,15 @@
+```release-note:bug
+data-source/aws_batch_job_definition: Properly handles ignored tags.
+```
+
+```release-note:bug
+resource/aws_dynamodb_table_replica: Properly handles default and ignored tags.
+```
+
+```release-note:bug
+resource/aws_cognito_user_pool: Properly handles ignored tags.
+```
+
+```release-note:bug
+data-source/aws_cognito_user_pool: Properly handles ignored tags.
+```

.ci/.semgrep-test-constants.yml

@@ -252,6 +252,30 @@ rules:
     options:
       constant_propagation: false
 
+  - id: literal-ProviderValue1Again-string-test-constant
+    languages: [go]
+    message: Use the constant `acctest.CtProviderValue1Again` for the string literal "providervalue1again" in test files
+    paths:
+      include:
+        - "internal/service/**/*_test.go"
+    pattern: '"providervalue1again"'
+    severity: ERROR
+    fix: "acctest.CtProviderValue1Again"
+    options:
+      constant_propagation: false
+
+  - id: literal-ProviderValue1Updated-string-test-constant
+    languages: [go]
+    message: Use the constant `acctest.CtProviderValue1Updated` for the string literal "providervalue1updated" in test files
+    paths:
+      include:
+        - "internal/service/**/*_test.go"
+    pattern: '"providervalue1updated"'
+    severity: ERROR
+    fix: "acctest.CtProviderValue1Updated"
+    options:
+      constant_propagation: false
+
   - id: literal-RName-string-test-constant
     languages: [go]
     message: Use the constant `acctest.CtRName` for the string literal "rName" in test files
@@ -324,6 +348,18 @@ rules:
     options:
       constant_propagation: false
 
+  - id: literal-ResourceValue1Again-string-test-constant
+    languages: [go]
+    message: Use the constant `acctest.CtResourceValue1Again` for the string literal "resourcevalue1again" in test files
+    paths:
+      include:
+        - "internal/service/**/*_test.go"
+    pattern: '"resourcevalue1again"'
+    severity: ERROR
+    fix: "acctest.CtResourceValue1Again"
+    options:
+      constant_propagation: false
+
   - id: literal-ResourceValue1Updated-string-test-constant
     languages: [go]
     message: Use the constant `acctest.CtResourceValue1Updated` for the string literal "resourcevalue1updated" in test files
@@ -348,6 +384,18 @@ rules:
     options:
       constant_propagation: false
 
+  - id: literal-ResourceValue2Updated-string-test-constant
+    languages: [go]
+    message: Use the constant `acctest.CtResourceValue2Updated` for the string literal "resourcevalue2updated" in test files
+    paths:
+      include:
+        - "internal/service/**/*_test.go"
+    pattern: '"resourcevalue2updated"'
+    severity: ERROR
+    fix: "acctest.CtResourceValue2Updated"
+    options:
+      constant_propagation: false
+
   - id: literal-RulePound-string-test-constant
     languages: [go]
     message: Use the constant `acctest.CtRulePound` for the string literal "rule.#" in test files

.ci/semgrep/tags/update.yml

@@ -16,3 +16,11 @@ rules:
           metavariable: "$FUNC"
           regex: "^resource\\w*(Create|Put|Set|Upsert|Enable)$"
     severity: WARNING
+
+  - id: attrtags-in-haschanges
+    languages: [go]
+    message: Do not include `names.AttrTags` in `HasChanges`, use `names.AttrTagsAll`
+    patterns:
+      - pattern: |
+          $D.HasChanges(..., names.AttrTags, ...)
+    severity: ERROR

.teamcity/components/generated/services_all.kt

@@ -13,7 +13,7 @@ val services = mapOf(
     "appconfig" to ServiceSpec("AppConfig"),
     "appfabric" to ServiceSpec("AppFabric", regionOverride = "us-east-1"),
     "appflow" to ServiceSpec("AppFlow"),
-    "appintegrations" to ServiceSpec("AppIntegrations"),
+    "appintegrations" to ServiceSpec("AppIntegrations", parallelismOverride = 10),
     "applicationinsights" to ServiceSpec("CloudWatch Application Insights"),
     "applicationsignals" to ServiceSpec("Application Signals"),
     "appmesh" to ServiceSpec("App Mesh"),

docs/resource-tagging.md

@@ -219,8 +219,12 @@ Most services can use a facility we call _transparent_ (or _implicit_) _tagging_
     }
     ```
 
-The `identifierAttribute` argument to the `@Tags` annotation identifies the attribute in the resource's schema whose value is used in tag listing and updating API calls. Common values are `"arn"` and "`id`".
-Once the annotation has been added to the resource's code, run `make gen` to register the resource for transparent tagging. This will add an entry to the `service_package_gen.go` file located in the service package folder.
+The `identifierAttribute` argument to the `@Tags` annotation identifies the attribute in the resource type's schema whose value is used in tag listing and updating API calls.
+Common values are `"arn"` and `"id"`.
+If the resource type does not need separate `createTags`, `listTags`, or `updateTags` functions, do not specify an `identifierAttribute`.
+
+Once the annotation has been added to the resource's code, run `make gen` to register the resource for transparent tagging.
+This will add an entry to the `service_package_gen.go` file located in the service package folder.
 
 #### Resource Create Operation
 
@@ -559,6 +563,13 @@ Use the annotation `@Testing(checkDestroyNoop=true)`.
 For some resource types, tags cannot be modified without recreating the resource.
 Use the annotation `@Testing(tagsUpdateForceNew=true)`.
 
+Resource types which pass the result of `getTagsIn` directly onto their Update Input may have an error where ignored tags are not correctly excluded from the update.
+Use the annotation `@Testing(tagsUpdateGetTagsIn=true)`.
+
+Some tests read the tag values directly from the AWS API.
+If the resource type does not specify `identifierAttribute` in its `@Tags` annotation, specify a `@Testing(tagsIdentifierAttribute=<attribute name>)` annotation to identify which attribute value should be used by the `listTags` function.
+If a resource type is also needed for the `listTags` function, also specify the `tagsResourceType` annotation.
+
 At least one resource type, the Service Catalog Provisioned Product, does not support removing tags.
 This is likely an error on the AWS side.
 Add the annotation `@Testing(noRemoveTags=true)` as a workaround.

internal/acctest/consts.csv

@@ -19,14 +19,18 @@ private_key_pem,PrivateKeyPEM
 provider_tags,ProviderTags
 providerkey1,ProviderKey1
 providervalue1,ProviderValue1
+providervalue1updated,ProviderValue1Updated
+providervalue1again,ProviderValue1Again
 rName,RName
 resource_owner,ResourceOwner
 resource_tags,ResourceTags
 resourcekey1,ResourceKey1
 resourcekey2,ResourceKey2
 resourcevalue1,ResourceValue1
 resourcevalue1updated,ResourceValue1Updated
+resourcevalue1again,ResourceValue1Again
 resourcevalue2,ResourceValue2
+resourcevalue2updated,ResourceValue2Updated
 rule.#,RulePound
 tags.%,TagsPercent
 tags.key1,TagsKey1

internal/acctest/statecheck/full_tags.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/hashicorp/terraform-plugin-framework/datasource"
 	"github.com/hashicorp/terraform-plugin-framework/resource"
 	"github.com/hashicorp/terraform-plugin-testing/knownvalue"
 	"github.com/hashicorp/terraform-plugin-testing/statecheck"
@@ -19,10 +20,19 @@ import (
 
 var _ statecheck.StateCheck = expectFullTagsCheck{}
 
+type entity string
+
+const (
+	entityResource   entity = "resource"
+	entityDataSource entity = "data source"
+)
+
 type expectFullTagsCheck struct {
 	base           Base
 	knownValue     knownvalue.Check
 	servicePackage conns.ServicePackage
+	tagSpecFinder  tagSpecFinder
+	entity         entity
 }
 
 func (e expectFullTagsCheck) CheckState(ctx context.Context, req statecheck.CheckStateRequest, resp *statecheck.CheckStateResponse) {
@@ -31,36 +41,24 @@ func (e expectFullTagsCheck) CheckState(ctx context.Context, req statecheck.Chec
 		return
 	}
 
-	var tagsSpec *types.ServicePackageResourceTags
 	sp := e.servicePackage
-	for _, r := range sp.FrameworkResources(ctx) {
-		foo, _ := r.Factory(ctx)
-		var metadata resource.MetadataResponse
-		foo.Metadata(ctx, resource.MetadataRequest{}, &metadata)
-		if res.Type == metadata.TypeName {
-			tagsSpec = r.Tags
-			break
-		}
-	}
-	if tagsSpec == nil {
-		for _, r := range sp.SDKResources(ctx) {
-			if res.Type == r.TypeName {
-				tagsSpec = r.Tags
-				break
-			}
-		}
-	}
+
+	tagsSpec := e.tagSpecFinder(ctx, sp, res.Type)
 
 	if tagsSpec == nil {
-		resp.Error = fmt.Errorf("no tagging specification found for resource type %s", res.Type)
+		resp.Error = fmt.Errorf("no tagging specification found for %s type %s", e.entity, res.Type)
 		return
 	}
 
 	identifierAttr := tagsSpec.IdentifierAttribute
+	if identifierAttr == "" {
+		resp.Error = fmt.Errorf("no tag identifier attribute defined for %s type %s", e.entity, res.Type)
+		return
+	}
 
 	identifier, ok := res.AttributeValues[identifierAttr]
 	if !ok {
-		resp.Error = fmt.Errorf("attribute %q not found in resource %s", identifierAttr, e.base.ResourceAddress())
+		resp.Error = fmt.Errorf("attribute %q not found in %s %s", identifierAttr, e.entity, e.base.ResourceAddress())
 		return
 	}
 
@@ -69,8 +67,12 @@ func (e expectFullTagsCheck) CheckState(ctx context.Context, req statecheck.Chec
 	var err error
 	if v, ok := sp.(tftags.ServiceTagLister); ok {
 		err = v.ListTags(ctx, acctest.Provider.Meta(), identifier.(string)) // Sets tags in Context
-	} else if v, ok := sp.(tftags.ResourceTypeTagLister); ok && tagsSpec.ResourceType != "" {
-		err = v.ListTags(ctx, acctest.Provider.Meta(), identifier.(string), tagsSpec.ResourceType) // Sets tags in Context
+	} else if v, ok := sp.(tftags.ResourceTypeTagLister); ok {
+		if tagsSpec.ResourceType == "" {
+			err = fmt.Errorf("ListTags method for service %s requires ResourceType, but none was set", sp.ServicePackageName())
+		} else {
+			err = v.ListTags(ctx, acctest.Provider.Meta(), identifier.(string), tagsSpec.ResourceType) // Sets tags in Context
+		}
 	} else {
 		err = fmt.Errorf("no ListTags method found for service %s", sp.ServicePackageName())
 	}
@@ -93,6 +95,8 @@ func (e expectFullTagsCheck) CheckState(ctx context.Context, req statecheck.Chec
 		return
 	}
 
+	tags = tags.IgnoreSystem(sp.ServicePackageName())
+
 	tagsMap := tfmaps.ApplyToAllValues(tags.Map(), func(s string) any {
 		return s
 	})
@@ -103,10 +107,92 @@ func (e expectFullTagsCheck) CheckState(ctx context.Context, req statecheck.Chec
 	}
 }
 
-func ExpectFullTags(servicePackage conns.ServicePackage, resourceAddress string, knownValue knownvalue.Check) expectFullTagsCheck {
+func ExpectFullResourceTags(servicePackage conns.ServicePackage, resourceAddress string, knownValue knownvalue.Check) expectFullTagsCheck {
+	return expectFullTagsCheck{
+		base:           NewBase(resourceAddress),
+		knownValue:     knownValue,
+		servicePackage: servicePackage,
+		tagSpecFinder:  findResourceTagSpec,
+		entity:         entityResource,
+	}
+}
+
+func ExpectFullResourceTagsSpecTags(servicePackage conns.ServicePackage, resourceAddress string, tagsSpec *types.ServicePackageResourceTags, knownValue knownvalue.Check) expectFullTagsCheck {
 	return expectFullTagsCheck{
 		base:           NewBase(resourceAddress),
 		knownValue:     knownValue,
 		servicePackage: servicePackage,
+		tagSpecFinder:  identityTagSpec(tagsSpec),
+		entity:         entityResource,
+	}
+}
+
+func ExpectFullDataSourceTags(servicePackage conns.ServicePackage, resourceAddress string, knownValue knownvalue.Check) expectFullTagsCheck {
+	return expectFullTagsCheck{
+		base:           NewBase(resourceAddress),
+		knownValue:     knownValue,
+		servicePackage: servicePackage,
+		tagSpecFinder:  findDataSourceTagSpec,
+		entity:         entityDataSource,
+	}
+}
+
+func ExpectFullDataSourceTagsSpecTags(servicePackage conns.ServicePackage, resourceAddress string, tagsSpec *types.ServicePackageResourceTags, knownValue knownvalue.Check) expectFullTagsCheck {
+	return expectFullTagsCheck{
+		base:           NewBase(resourceAddress),
+		knownValue:     knownValue,
+		servicePackage: servicePackage,
+		tagSpecFinder:  identityTagSpec(tagsSpec),
+		entity:         entityDataSource,
+	}
+}
+
+type tagSpecFinder func(context.Context, conns.ServicePackage, string) *types.ServicePackageResourceTags
+
+func findResourceTagSpec(ctx context.Context, sp conns.ServicePackage, typeName string) (tagsSpec *types.ServicePackageResourceTags) {
+	for _, r := range sp.FrameworkResources(ctx) {
+		factory, _ := r.Factory(ctx)
+		var metadata resource.MetadataResponse
+		factory.Metadata(ctx, resource.MetadataRequest{}, &metadata)
+		if metadata.TypeName == typeName {
+			tagsSpec = r.Tags
+			break
+		}
+	}
+	if tagsSpec == nil {
+		for _, r := range sp.SDKResources(ctx) {
+			if r.TypeName == typeName {
+				tagsSpec = r.Tags
+				break
+			}
+		}
+	}
+	return tagsSpec
+}
+
+func findDataSourceTagSpec(ctx context.Context, sp conns.ServicePackage, typeName string) (tagsSpec *types.ServicePackageResourceTags) {
+	for _, r := range sp.FrameworkDataSources(ctx) {
+		factory, _ := r.Factory(ctx)
+		var metadata datasource.MetadataResponse
+		factory.Metadata(ctx, datasource.MetadataRequest{}, &metadata)
+		if metadata.TypeName == typeName {
+			tagsSpec = r.Tags
+			break
+		}
+	}
+	if tagsSpec == nil {
+		for _, r := range sp.SDKDataSources(ctx) {
+			if r.TypeName == typeName {
+				tagsSpec = r.Tags
+				break
+			}
+		}
+	}
+	return tagsSpec
+}
+
+func identityTagSpec(tagsSpec *types.ServicePackageResourceTags) tagSpecFinder {
+	return func(ctx context.Context, sp conns.ServicePackage, typeName string) *types.ServicePackageResourceTags {
+		return tagsSpec
 	}
 }