Merge pull request #37066 from hashicorp/td-generate-tagtests-servicecatalog

Service Catalog Provisioned Product tagging issues

Author: gdavison

.changelog/37066.txt

@@ -0,0 +1,7 @@
+```release-note:bug
+resource/aws_servicecatalog_provisioned_product: Fixes error where tag values are not applied to products when tag values don't change.
+```
+
+```release-note:bug
+resource/aws_servicecatalog_portfolio: Fixes error where deletion fails if resource was deleted out of band.
+```

internal/acctest/s3.go

@@ -0,0 +1,37 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package acctest
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/terraform"
+	"github.com/hashicorp/terraform-provider-aws/internal/conns"
+	tfs3 "github.com/hashicorp/terraform-provider-aws/internal/service/s3"
+)
+
+func S3BucketHasTag(ctx context.Context, bucketName, key, value string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		conn := Provider.Meta().(*conns.AWSClient).S3Client(ctx)
+
+		tags, err := tfs3.BucketListTags(ctx, conn, bucketName)
+		if err != nil {
+			return err
+		}
+
+		for k, v := range tags {
+			if k == key {
+				if v.ValueString() == value {
+					return nil
+				} else {
+					return fmt.Errorf("expected tag %q value to be %s, got %s", key, value, v.ValueString())
+				}
+			}
+		}
+
+		return fmt.Errorf("expected tag %q not found", key)
+	}
+}

internal/generate/tagstests/file.tmpl

@@ -51,6 +51,9 @@ import (
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
 	"github.com/hashicorp/terraform-provider-aws/internal/acctest"
 	"github.com/hashicorp/terraform-provider-aws/names"
+	{{ range .GoImports -}}
+	{{ if .Alias }}{{ .Alias }} {{ end }}"{{ .Path }}"
+	{{ end }}
 )
 
 {{ if .Serialize }}
@@ -173,6 +176,9 @@ func {{ template "testname" . }}_tags_AddOnUpdate(t *testing.T) {
 }
 
 func {{ template "testname" . }}_tags_EmptyTag_OnCreate(t *testing.T) {
+{{- if .SkipEmptyTags }}
+	t.Skip("Resource {{ .Name }} does not support empty tags")
+{{ end }}
 	{{- template "Init" . }}
 
 	resource.{{ if .Serialize }}Test{{ else }}ParallelTest{{ end }}(t, resource.TestCase{
@@ -200,6 +206,9 @@ func {{ template "testname" . }}_tags_EmptyTag_OnCreate(t *testing.T) {
 }
 
 func {{ template "testname" . }}_tags_EmptyTag_OnUpdate_Add(t *testing.T) {
+{{- if .SkipEmptyTags }}
+	t.Skip("Resource {{ .Name }} does not support empty tags")
+{{ end }}
 	{{- template "Init" . }}
 
 	resource.{{ if .Serialize }}Test{{ else }}ParallelTest{{ end }}(t, resource.TestCase{
@@ -237,6 +246,9 @@ func {{ template "testname" . }}_tags_EmptyTag_OnUpdate_Add(t *testing.T) {
 }
 
 func {{ template "testname" . }}_tags_EmptyTag_OnUpdate_Replace(t *testing.T) {
+{{- if .SkipEmptyTags }}
+	t.Skip("Resource {{ .Name }} does not support empty tags")
+{{ end }}
 	{{- template "Init" . }}
 
 	resource.{{ if .Serialize }}Test{{ else }}ParallelTest{{ end }}(t, resource.TestCase{
@@ -500,6 +512,9 @@ func {{ template "testname" . }}_tags_DefaultTags_updateToResourceOnly(t *testin
 }
 
 func {{ template "testname" . }}_tags_DefaultTags_emptyResourceTag(t *testing.T) {
+{{- if .SkipEmptyTags }}
+	t.Skip("Resource {{ .Name }} does not support empty tags")
+{{ end }}
 	{{- template "Init" . }}
 
 	resource.{{ if .Serialize }}Test{{ else }}ParallelTest{{ end }}(t, resource.TestCase{

internal/generate/tagstests/main.go

@@ -111,6 +111,13 @@ type ResourceDatum struct {
 	Implementation    implementation
 	Serialize         bool
 	PreCheck          bool
+	SkipEmptyTags     bool // TODO: Remove when we have a strategy for resources that have a minimum tag value length of 1
+	GoImports         []goImport
+}
+
+type goImport struct {
+	Path  string
+	Alias string
 }
 
 //go:embed file.tmpl
@@ -221,7 +228,28 @@ func (v *visitor) processFuncDecl(funcDecl *ast.FuncDecl) {
 					d.ExistsTypeName = typeName
 				}
 				if attr, ok := args.Keyword["generator"]; ok {
-					d.Generator = attr
+					parts := strings.Split(attr, ";")
+					switch len(parts) {
+					case 1:
+						d.Generator = parts[0]
+
+					case 2:
+						d.Generator = parts[1]
+						d.GoImports = append(d.GoImports, goImport{
+							Path: parts[0],
+						})
+
+					case 3:
+						d.Generator = parts[2]
+						d.GoImports = append(d.GoImports, goImport{
+							Path:  parts[0],
+							Alias: parts[1],
+						})
+
+					default:
+						v.errs = append(v.errs, fmt.Errorf("invalid generator value: %q at %s.", attr, fmt.Sprintf("%s.%s", v.packageName, v.functionName)))
+						continue
+					}
 				}
 				if attr, ok := args.Keyword["importIgnore"]; ok {
 					d.ImportIgnore = strings.Split(attr, ";")
@@ -254,6 +282,14 @@ func (v *visitor) processFuncDecl(funcDecl *ast.FuncDecl) {
 						skip = true
 					}
 				}
+				if attr, ok := args.Keyword["skipEmptyTags"]; ok {
+					if b, err := strconv.ParseBool(attr); err != nil {
+						v.errs = append(v.errs, fmt.Errorf("invalid skipEmptyTags value: %q at %s. Should be boolean value.", attr, fmt.Sprintf("%s.%s", v.packageName, v.functionName)))
+						continue
+					} else {
+						d.SkipEmptyTags = b
+					}
+				}
 			}
 		}
 	}

internal/service/s3/exports.go

@@ -7,4 +7,6 @@ package s3
 var (
 	ResourceBucket = resourceBucket
 	ResourceObject = resourceObject
+
+	BucketListTags = bucketListTags
 )

internal/service/s3/exports_test.go

@@ -28,7 +28,6 @@ var (
 	ResourceDirectoryBucket                         = newDirectoryBucketResource
 	ResourceObjectCopy                              = resourceObjectCopy
 
-	BucketListTags                        = bucketListTags
 	BucketUpdateTags                      = bucketUpdateTags
 	BucketRegionalDomainName              = bucketRegionalDomainName
 	BucketWebsiteEndpointAndDomain        = bucketWebsiteEndpointAndDomain

internal/service/servicecatalog/generate.go

@@ -3,6 +3,7 @@
 
 //go:generate go run ../../generate/tags/main.go -ServiceTagsSlice
 //go:generate go run ../../generate/servicepackage/main.go
+//go:generate go run ../../generate/tagstests/main.go
 // ONLY generate directives and package declaration! Do not add anything else to this file.
 
 package servicecatalog

internal/service/servicecatalog/portfolio.go

@@ -26,6 +26,7 @@ import (
 
 // @SDKResource("aws_servicecatalog_portfolio", name="Portfolio")
 // @Tags
+// @Testing(existsType="github.com/aws/aws-sdk-go/service/servicecatalog.DescribePortfolioOutput", generator="github.com/hashicorp/terraform-plugin-testing/helper/acctest;sdkacctest;sdkacctest.RandString(5)", skipEmptyTags=true)
 func ResourcePortfolio() *schema.Resource {
 	return &schema.Resource{
 		CreateWithoutTimeout: resourcePortfolioCreate,
@@ -185,6 +186,10 @@ func resourcePortfolioDelete(ctx context.Context, d *schema.ResourceData, meta i
 		Id: aws.String(d.Id()),
 	})
 
+	if tfawserr.ErrCodeEquals(err, servicecatalog.ErrCodeResourceNotFoundException) {
+		return diags
+	}
+
 	if err != nil {
 		return sdkdiag.AppendErrorf(diags, "deleting Service Catalog Portfolio (%s): %s", d.Id(), err)
 	}

internal/service/servicecatalog/portfolio_data_source_test.go

@@ -22,7 +22,7 @@ func TestAccServiceCatalogPortfolioDataSource_basic(t *testing.T) {
 		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
 		ErrorCheck:               acctest.ErrorCheck(t, names.ServiceCatalogServiceID),
 		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
-		CheckDestroy:             testAccCheckServiceCatlaogPortfolioDestroy(ctx),
+		CheckDestroy:             testAccCheckPortfolioDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccPortfolioDataSourceConfig_basic(rName),