Refactor Kinesis Streams resource and data source to use keyvaluetags package #11352

ewbankkit · 2019-12-18T20:17:14Z

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #10688.

Release note for CHANGELOG:

NONE

Output from acceptance testing:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisStreamDataSource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 1 -run=TestAccAWSKinesisStreamDataSource -timeout 120m
=== RUN   TestAccAWSKinesisStreamDataSource
=== PAUSE TestAccAWSKinesisStreamDataSource
=== CONT  TestAccAWSKinesisStreamDataSource
--- PASS: TestAccAWSKinesisStreamDataSource (172.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	172.498s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisStream_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 1 -run=TestAccAWSKinesisStream_basic -timeout 120m
=== RUN   TestAccAWSKinesisStream_basic
=== PAUSE TestAccAWSKinesisStream_basic
=== CONT  TestAccAWSKinesisStream_basic
--- PASS: TestAccAWSKinesisStream_basic (79.57s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	79.671s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisStream_Tags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 1 -run=TestAccAWSKinesisStream_Tags -timeout 120m
=== RUN   TestAccAWSKinesisStream_Tags
=== PAUSE TestAccAWSKinesisStream_Tags
=== CONT  TestAccAWSKinesisStream_Tags
--- PASS: TestAccAWSKinesisStream_Tags (106.87s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	106.976s

ewbankkit · 2019-12-18T20:19:25Z

aws/internal/keyvaluetags/generators/updatetags/main.go

@@ -422,6 +432,16 @@ func ServiceTagInputTagsField(serviceName string) string {
 	}
 }

+// ServiceTagInputResourceTypeField determines the service tagging tags field requires a map[string]*string.


Yet another permutation for the Kinesis Streams service.
Listing tags returns a []*kinesis.Tag but the tags input to the AddTags API requires a map[string]*string.
Capture that here via another specialization.

Refactoring the import functionality to set resource ID there is too much surgery for this refactor.

ewbankkit · 2019-12-18T21:04:52Z

Remove WIP.
Ready for review.

…nt for Kinesis.

ewbankkit · 2019-12-19T18:49:42Z

Given that #11368 has a similar problem with a transformation of the tags passed to the tagging function i replaced the ServiceTagInputRequiresStringMap customization with the more general ServiceTagInputRequiresTransformation customization.
This requires the addition of a hand-coded transformation function to be added to a new file service_customizations.go.
@bflad, I'm open to other ideas.

ewbankkit · 2019-12-19T18:50:03Z

Removed WIP (again).

bflad · 2019-12-19T22:05:43Z

Phew! We are getting into the weeds now with these tagging implementations. Thanks for starting this one and nice work shimming this in here so far. A few notes/open questions -- would love to hear your opinions here!

The kinesis service is one that requires batched tagging 🙁 (only 10 tags at a time per API call), so we would need to support batching in the generator template. There's actually other services that have this sort of limitation as well, although I'm forgetting exactly which off the top of my head (sorry I know this isn't too helpful), and we don't have any open bug reports I can quickly find at the moment.

I think for this type of handling we can introduce a generator function that just contains the number of tags per batch, e.g.

func ServiceTagFunctionBatchSize(serviceName string) string {
  switch serviceName {
  case "kinesis":
    return "10"
  default:
    return ""
  }
}

And the template can if/else the batching logic based off that. The logic I originally had in tagsKinesis.go was a little Go code verbose, so totally understandable if it was switched to something more compact like the batching in #10435 with slice indices.

For the TagInputRequiresTransformation bits, wow that API difference is annoying. While the extra {SERVICE}TagInput() functions would offer the most flexibility, it might be nice to simplify this to not need abstraction layer either via:

Making the customization value itself the input value in the template from the customization function, e.g. return "aws.StringMap(updatedTags.IgnoreAws().Map())" and

			{{- if . | TagInputCustomValue }}
			{{ . | TagInputTagsField }}:         {{ . | TagInputCustomValue }},
			{{- else }}

This has the problem of needing to make sure any changes to the updatedTags.IgnoreAws() bits occur in both the template and the new customization function though.

Making the template function be based on something like "tag input requires map" for dropping the real input value directly since we already support UntagInputRequiresTagType:

			{{- if . | TagInputRequiresMap }}
			{{ . | TagInputTagsField }}:         aws.StringMap(updatedTags.IgnoreAws().Map()),
			{{- else }}

At first blush, this seemed like a quick win, but I see the problem you mention with Pinpoint being that it needs to be &pinpoint.TagsModel{Tags: updatedTags.IgnoreAws().PinpointTags()},. That wouldn't translate well to just being handled in the template though except something not so pretty like return "&pinpoint.TagsModel{Tags:" and:

			{{- if . | TagInputRequiresMap }}
			{{ . | TagInputTagsField }}:         aws.StringMap(updatedTags.IgnoreAws().Map()),
			{{- else if . | TagInputRequiresWrappingType }}
			{{ . | TagInputTagsField }}:         {{ . | TagInputRequiresWrappingType }} updatedTags.IgnoreAws().{{ . | Title }}Tags()},
			{{- else }}}}

Gross. This was a problem that would needed to be solved for CloudFront as well.

Generate the {SERVICE}TagInput() and {SERVICE}UntagInput() functions for all services and update the generator template to just use those functions as values instead of the if/else conditions in the template

This would make the logic potentially easier to understand since it'd be the same for all services instead of having one-off customization functions either in the generator main.go or up a level.

I'm not necessarily opposed to these one-off TagInput customization functions since they would allow the ultimate customizability, just food for thought about later folks who need to read and understand the generator. 😄 Open to ideas.

ewbankkit · 2019-12-23T13:13:19Z

Batch size:

I implemented the ServiceTagFunctionBatchSize customization function and associated template changes as suggested.

Input transformation:

Overall the suggestion of generating {SERVICE}TagInput() and {SERVICE}UntagInput() functions for all services seems to be most generic (and we can collapse the ServiceUntagInputRequiresTagType() logic into this at the same time) but the implementation seemed to be too invasive - The code that generates the default {SERVICE}TagInput() functions needs to know about TagType and Map and Slice services from the listtags generator.
I ended up implementing the first option (TagInputCustomValue) which is the simplest but should be flexible enough to cover the use cases.

Re-ran acceptance tests:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSKinesisStream_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKinesisStream_ -timeout 120m
=== RUN   TestAccAWSKinesisStream_basic
=== PAUSE TestAccAWSKinesisStream_basic
=== RUN   TestAccAWSKinesisStream_createMultipleConcurrentStreams
=== PAUSE TestAccAWSKinesisStream_createMultipleConcurrentStreams
=== RUN   TestAccAWSKinesisStream_encryptionWithoutKmsKeyThrowsError
=== PAUSE TestAccAWSKinesisStream_encryptionWithoutKmsKeyThrowsError
=== RUN   TestAccAWSKinesisStream_encryption
=== PAUSE TestAccAWSKinesisStream_encryption
=== RUN   TestAccAWSKinesisStream_shardCount
=== PAUSE TestAccAWSKinesisStream_shardCount
=== RUN   TestAccAWSKinesisStream_retentionPeriod
=== PAUSE TestAccAWSKinesisStream_retentionPeriod
=== RUN   TestAccAWSKinesisStream_shardLevelMetrics
=== PAUSE TestAccAWSKinesisStream_shardLevelMetrics
=== RUN   TestAccAWSKinesisStream_enforceConsumerDeletion
=== PAUSE TestAccAWSKinesisStream_enforceConsumerDeletion
=== RUN   TestAccAWSKinesisStream_Tags
=== PAUSE TestAccAWSKinesisStream_Tags
=== CONT  TestAccAWSKinesisStream_basic
=== CONT  TestAccAWSKinesisStream_retentionPeriod
=== CONT  TestAccAWSKinesisStream_Tags
=== CONT  TestAccAWSKinesisStream_enforceConsumerDeletion
=== CONT  TestAccAWSKinesisStream_shardLevelMetrics
=== CONT  TestAccAWSKinesisStream_encryptionWithoutKmsKeyThrowsError
=== CONT  TestAccAWSKinesisStream_createMultipleConcurrentStreams
=== CONT  TestAccAWSKinesisStream_shardCount
=== CONT  TestAccAWSKinesisStream_encryption
--- PASS: TestAccAWSKinesisStream_encryptionWithoutKmsKeyThrowsError (56.87s)
--- PASS: TestAccAWSKinesisStream_basic (78.59s)
--- PASS: TestAccAWSKinesisStream_enforceConsumerDeletion (79.44s)
--- PASS: TestAccAWSKinesisStream_retentionPeriod (140.28s)
--- PASS: TestAccAWSKinesisStream_shardLevelMetrics (152.38s)
--- PASS: TestAccAWSKinesisStream_shardCount (157.12s)
--- PASS: TestAccAWSKinesisStream_createMultipleConcurrentStreams (159.59s)
--- PASS: TestAccAWSKinesisStream_Tags (164.11s)
--- PASS: TestAccAWSKinesisStream_encryption (202.96s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	203.034s

bflad

Looks great and nice implementation, @ewbankkit 🚀

Output from acceptance testing:

--- PASS: TestAccAWSKinesisStream_basic (60.09s)
--- PASS: TestAccAWSKinesisStream_createMultipleConcurrentStreams (175.57s)
--- PASS: TestAccAWSKinesisStream_encryption (166.80s)
--- PASS: TestAccAWSKinesisStream_encryptionWithoutKmsKeyThrowsError (46.62s)
--- PASS: TestAccAWSKinesisStream_enforceConsumerDeletion (60.54s)
--- PASS: TestAccAWSKinesisStream_retentionPeriod (89.48s)
--- PASS: TestAccAWSKinesisStream_shardCount (124.02s)
--- PASS: TestAccAWSKinesisStream_shardLevelMetrics (110.65s)
--- PASS: TestAccAWSKinesisStream_Tags (121.00s)
--- PASS: TestAccAWSKinesisStreamDataSource (151.77s)

bflad · 2020-01-03T20:03:15Z

aws/internal/keyvaluetags/generators/updatetags/main.go

+		{{- if . | TagFunctionBatchSize }}
+		chunks := removedTags.Chunks({{ . | TagFunctionBatchSize }})
+
+		for _, chunk := range chunks {
+			input := &{{ . | TagPackage }}.{{ . | UntagFunction }}Input{
+				{{- if . | TagInputIdentifierRequiresSlice }}
+				{{ . | TagInputIdentifierField }}:   aws.StringSlice([]string{identifier}),
+				{{- else }}
+				{{ . | TagInputIdentifierField }}:   aws.String(identifier),
+				{{- end }}
+				{{- if . | TagInputResourceTypeField }}
+				{{ . | TagInputResourceTypeField }}: aws.String(resourceType),
+				{{- end }}
+				{{- if . | UntagInputRequiresTagType }}
+				{{ . | UntagInputTagsField }}:       chunk.IgnoreAws().{{ . | Title }}Tags(),
+				{{- else }}
+				{{ . | UntagInputTagsField }}:       aws.StringSlice(chunk.Keys()),
+				{{- end }}
+			}
+
+			_, err := conn.{{ . | UntagFunction }}(input)
+
+			if err != nil {
+				return fmt.Errorf("error untagging resource (%s): %w", identifier, err)
+			}
+		}
+		{{- else }}


Potential optimization: Given this nice KeyValueTags.Chunks() implementation (💯), we might be able to skip redeclaring the majority of the copy-paste template content by re-scoping the removedTags and updatedTags variables in the loop:

{{- if . | TagFunctionBatchSize }} for _, removedTags := range removedTags.Chunks({{ . | TagFunctionBatchSize }}) { {{- end }}

The template generator should automatically fix up the formatting whether it should be nested for the looping or not. This doesn't seem worth holding up this merge though. 😄

We could also support always calling for _, removedTags := range removedTags.Chunks(-1) for other services and forego the template conditional, but only adding the looping logic when batching is enabled seems more right.

PR submitted: #11502

@bflad Nice,
I didn't know about the formatting fix up with the template generator and had thought about something like:

{{- if . | TagFunctionBatchSize }} chunks := removedTags.Chunks({{ . | TagFunctionBatchSize }}) {{- else }} chunks := []KeyValueTags{removedTags} {{- end }}

…g support Reference: #11352 (comment) Updated via: ``` make gen ``` Output from acceptance testing: ``` --- PASS: TestAccAWSKinesisStream_Tags (219.62s) ```

ghost · 2020-01-10T17:00:25Z

This has been released in version 2.44.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

…g support Reference: #11352 (comment) Updated via: ``` make gen ``` Output from acceptance testing: ``` --- PASS: TestAccAWSKinesisStream_Tags (219.62s) ```

…g support (#11502) Reference: #11352 (comment) Updated via: ``` make gen ``` Output from acceptance testing: ``` --- PASS: TestAccAWSKinesisStream_Tags (219.62s) ```

ghost · 2020-03-27T17:52:01Z

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!

Kit Ewbank added 3 commits December 18, 2019 15:08

Add tag listing and updating for Kinesis (streams) to keyvaluetags.

22db3c1

r/aws_kinesis_stream: Refactor tagging logic to keyvaluetags package.

cb2683b

d/aws_kinesis_stream: Refactor tagging logic to keyvaluetags package.

a2eea21

ewbankkit requested a review from a team December 18, 2019 20:17

ewbankkit commented Dec 18, 2019

View reviewed changes

Restore call to d.SetId() in the Kinesis Streams Read method.

b39ce11

Refactoring the import functionality to set resource ID there is too much surgery for this refactor.

ewbankkit changed the title ~~[WIP] Refactor Kinesis Streams resource and data source to use keyvaluetags package~~ Refactor Kinesis Streams resource and data source to use keyvaluetags package Dec 18, 2019

ewbankkit changed the title ~~Refactor Kinesis Streams resource and data source to use keyvaluetags package~~ [WIP] Refactor Kinesis Streams resource and data source to use keyvaluetags package Dec 19, 2019

Kit Ewbank added 2 commits December 19, 2019 10:58

Document use of 'ServiceTagInputIdentifierRequiresSlice' customization.

916e617

Add 'ServiceTagInputRequiresTransformation' customization and impleme…

0544f30

…nt for Kinesis.

ewbankkit changed the title ~~[WIP] Refactor Kinesis Streams resource and data source to use keyvaluetags package~~ Refactor Kinesis Streams resource and data source to use keyvaluetags package Dec 19, 2019

bflad added technical-debt Addresses areas of the codebase that need refactoring or redesign. and removed needs-triage Waiting for first response or review from a maintainer. labels Dec 19, 2019

ewbankkit mentioned this pull request Dec 19, 2019

Refactor Pinpoint application resource to use keyvaluetags package #11368

Merged

bflad self-assigned this Dec 19, 2019

ewbankkit force-pushed the kinesis-keyvaluetags branch from b9ed22f to 68259eb Compare December 22, 2019 20:49

ewbankkit added 3 commits December 22, 2019 16:05

r/aws_kinesis_stream: Check tag values in acceptance test.

796a9f9

Add 'ServiceTagFunctionBatchSize'.

7dfbd4e

'TagInputRequiresTransformation' -> 'TagInputCustomValue'.

7418bca

ewbankkit force-pushed the kinesis-keyvaluetags branch from 68259eb to 7418bca Compare December 23, 2019 12:56

bflad approved these changes Jan 3, 2020

View reviewed changes

bflad merged commit d69cf7e into hashicorp:master Jan 3, 2020

bflad added this to the v2.44.0 milestone Jan 3, 2020

bflad mentioned this pull request Jan 6, 2020

internal/keyvaluetags: Simplify updatetags generator template batching support #11502

Merged

ewbankkit deleted the kinesis-keyvaluetags branch January 7, 2020 12:08

ewbankkit mentioned this pull request Jan 13, 2020

Refactor CloudFront Distribution resource to use keyvaluetags package #11575

Merged

ghost locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Kinesis Streams resource and data source to use keyvaluetags package #11352

Refactor Kinesis Streams resource and data source to use keyvaluetags package #11352

ewbankkit commented Dec 18, 2019 •

edited

Loading

ewbankkit Dec 18, 2019

ewbankkit commented Dec 18, 2019 •

edited

Loading

ewbankkit commented Dec 19, 2019

ewbankkit commented Dec 19, 2019

bflad commented Dec 19, 2019 •

edited

Loading

ewbankkit commented Dec 23, 2019 •

edited

Loading

bflad left a comment

bflad Jan 3, 2020

bflad Jan 6, 2020

ewbankkit Jan 7, 2020 •

edited

Loading

ghost commented Jan 10, 2020

ghost commented Mar 27, 2020

Refactor Kinesis Streams resource and data source to use keyvaluetags package #11352

Refactor Kinesis Streams resource and data source to use keyvaluetags package #11352

Conversation

ewbankkit commented Dec 18, 2019 • edited Loading

Community Note

ewbankkit Dec 18, 2019

Choose a reason for hiding this comment

ewbankkit commented Dec 18, 2019 • edited Loading

ewbankkit commented Dec 19, 2019

ewbankkit commented Dec 19, 2019

bflad commented Dec 19, 2019 • edited Loading

ewbankkit commented Dec 23, 2019 • edited Loading

Batch size:

Input transformation:

bflad left a comment

Choose a reason for hiding this comment

bflad Jan 3, 2020

Choose a reason for hiding this comment

bflad Jan 6, 2020

Choose a reason for hiding this comment

ewbankkit Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

ghost commented Jan 10, 2020

ghost commented Mar 27, 2020

ewbankkit commented Dec 18, 2019 •

edited

Loading

ewbankkit commented Dec 18, 2019 •

edited

Loading

bflad commented Dec 19, 2019 •

edited

Loading

ewbankkit commented Dec 23, 2019 •

edited

Loading

ewbankkit Jan 7, 2020 •

edited

Loading