Add tags attribute support for redshift parameter group

[teraken0509]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8882

Release note for CHANGELOG:

adds tags attribute for "aws_redshift_parameter_group"

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftParameterGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftParameterGroup_ -timeout 120m
=== RUN   TestAccAWSRedshiftParameterGroup_importBasic
=== PAUSE TestAccAWSRedshiftParameterGroup_importBasic
=== RUN   TestAccAWSRedshiftParameterGroup_withParameters
=== PAUSE TestAccAWSRedshiftParameterGroup_withParameters
=== RUN   TestAccAWSRedshiftParameterGroup_withoutParameters
=== PAUSE TestAccAWSRedshiftParameterGroup_withoutParameters
=== RUN   TestAccAWSRedshiftParameterGroup_withTags
=== PAUSE TestAccAWSRedshiftParameterGroup_withTags
=== CONT  TestAccAWSRedshiftParameterGroup_importBasic
=== CONT  TestAccAWSRedshiftParameterGroup_withTags
=== CONT  TestAccAWSRedshiftParameterGroup_withoutParameters
=== CONT  TestAccAWSRedshiftParameterGroup_withParameters
--- PASS: TestAccAWSRedshiftParameterGroup_withoutParameters (24.66s)
--- PASS: TestAccAWSRedshiftParameterGroup_withParameters (25.35s)
--- PASS: TestAccAWSRedshiftParameterGroup_importBasic (28.48s)
--- PASS: TestAccAWSRedshiftParameterGroup_withTags (61.68s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	61.767s

[bflad]

Overall looking good, @kterada0509, thanks for working on this. A few little items and this should be ready for merge!

[bflad]

When using d.Set() with aggregate types (TypeList, TypeSet, TypeMap), we should perform error checking to prevent issues where the code is not properly able to set the Terraform state. e.g.

Suggested change

	d.Set("tags", tagsToMapRedshift(describeResp.ParameterGroups[0].Tags))
	if err := d.Set("tags", tagsToMapRedshift(describeResp.ParameterGroups[0].Tags)); err != nil {
	return fmt.Errorf("error setting tags: %s", err)
	}

[teraken0509]

Thanks I fixed

[bflad]

This seems like it would be super helpful to add as a resource attribute as well! Can you please use this logic to d.Set("arn", arn) in the Read, add the schema definition (with documentation), and then use d.Get("arn").(string) in the setTagsRedshift() call below? Thanks!

[teraken0509]

Thanks, I fixed it and affected resources with this change.
acctest is working on now.

[bflad]

It looks like the formatting here is off. Can you please adjust to 2 spaces and aligned like terraform fmt? Thanks.

[teraken0509]

fixed!

[bflad]

Same here 😄

[teraken0509]

fixed!

[bflad]

Thanks for the update, @kterada0509! 🚀 Looked good except for one subtle issue, which will be fixed during merge.

Output from acceptance testing:

--- PASS: TestAccAWSRedshiftCluster_basic (735.02s)
--- PASS: TestAccAWSRedshiftCluster_changeAvailabilityZone (1185.70s)
--- PASS: TestAccAWSRedshiftCluster_enhancedVpcRoutingEnabled (856.11s)
--- PASS: TestAccAWSRedshiftCluster_forceNewUsername (1156.82s)
--- PASS: TestAccAWSRedshiftCluster_iamRoles (797.91s)
--- PASS: TestAccAWSRedshiftCluster_kmsKey (755.35s)
--- PASS: TestAccAWSRedshiftCluster_loggingEnabled (594.63s)
--- PASS: TestAccAWSRedshiftCluster_publiclyAccessible (722.36s)
--- PASS: TestAccAWSRedshiftCluster_snapshotCopy (775.50s)
--- PASS: TestAccAWSRedshiftCluster_tags (700.00s)
--- PASS: TestAccAWSRedshiftCluster_withFinalSnapshot (992.02s)
--- PASS: TestAccAWSRedshiftEventSubscription_basicUpdate (20.94s)
--- PASS: TestAccAWSRedshiftEventSubscription_categoryUpdate (20.71s)
--- PASS: TestAccAWSRedshiftEventSubscription_tagsUpdate (91.82s)
--- PASS: TestAccAWSRedshiftEventSubscription_withPrefix (12.40s)
--- PASS: TestAccAWSRedshiftEventSubscription_withSourceIds (37.30s)
--- PASS: TestAccAWSRedshiftParameterGroup_importBasic (12.02s)
--- PASS: TestAccAWSRedshiftParameterGroup_withoutParameters (7.98s)
--- PASS: TestAccAWSRedshiftParameterGroup_withParameters (10.94s)
--- PASS: TestAccAWSRedshiftParameterGroup_withTags (24.15s)
--- PASS: TestAccAWSRedshiftSnapshotCopyGrant_Basic (14.56s)
--- PASS: TestAccAWSRedshiftSnapshotCopyGrant_Update (28.97s)
--- PASS: TestAccAWSRedshiftSubnetGroup_basic (25.48s)
--- PASS: TestAccAWSRedshiftSubnetGroup_importBasic (19.91s)
--- PASS: TestAccAWSRedshiftSubnetGroup_tags (20.34s)
--- PASS: TestAccAWSRedshiftSubnetGroup_updateDescription (31.93s)
--- PASS: TestAccAWSRedshiftSubnetGroup_updateSubnetIds (22.68s)

[bflad]

There was a subtle bug introduced here as the handling of the tags argument would trigger an error because it was not protected by an if d.IsNewResource() conditional and the arn attribute was not set yet (which showed as a * resource ARN in the Redshift error):

--- FAIL: TestAccAWSRedshiftParameterGroup_withTags (8.02s)
    testing.go:568: Step 0 error: errors during apply:

        Error: AccessDenied: User: arn:aws:iam::--OMITTED--:user/bflad is not authorized to perform: redshift:CreateTags on resource: *

To fix this, opted to switch resourceAwsRedshiftParameterGroupCreate from returning resourceAwsRedshiftParameterGroupUpdate to instead return resourceAwsRedshiftParameterGroupRead and implement setting parameters during creation:

	if v := d.Get("parameter").(*schema.Set); v.Len() > 0 {
		parameters, err := expandRedshiftParameters(v.List())

		if err != nil {
			return fmt.Errorf("error expanding parameter: %s", err)
		}

		modifyOpts := redshift.ModifyClusterParameterGroupInput{
			ParameterGroupName: aws.String(d.Id()),
			Parameters:         parameters,
		}

		if _, err := conn.ModifyClusterParameterGroup(&modifyOpts); err != nil {
			return fmt.Errorf("error adding Redshift Parameter Group (%s) parameters: %s", d.Id(), err)
		}
	}

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!