Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jan 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22db3c1
Add tag listing and updating for Kinesis (streams) to keyvaluetags.
Dec 18, 2019
cb2683b
r/aws_kinesis_stream: Refactor tagging logic to keyvaluetags package.
Dec 18, 2019
a2eea21
d/aws_kinesis_stream: Refactor tagging logic to keyvaluetags package.
Dec 18, 2019
b39ce11
Restore call to `d.SetId()` in the Kinesis Streams Read method.
Dec 18, 2019
916e617
Document use of 'ServiceTagInputIdentifierRequiresSlice' customization.
Dec 19, 2019
0544f30
Add 'ServiceTagInputRequiresTransformation' customization and impleme…
Dec 19, 2019
796a9f9
r/aws_kinesis_stream: Check tag values in acceptance test.
ewbankkit Dec 20, 2019
7dfbd4e
Add 'ServiceTagFunctionBatchSize'.
ewbankkit Dec 22, 2019
7418bca
'TagInputRequiresTransformation' -> 'TagInputCustomValue'.
ewbankkit Dec 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions aws/data_source_aws_kinesis_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package aws

import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/kinesis"
"fmt"

"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
)

func dataSourceAwsKinesisStream() *schema.Resource {
Expand Down Expand Up @@ -57,10 +58,7 @@ func dataSourceAwsKinesisStream() *schema.Resource {
Set: schema.HashString,
},

"tags": {
Type: schema.TypeMap,
Computed: true,
},
"tags": tagsSchemaComputed(),
},
}
}
Expand All @@ -83,13 +81,15 @@ func dataSourceAwsKinesisStreamRead(d *schema.ResourceData, meta interface{}) er
d.Set("retention_period", state.retentionPeriod)
d.Set("shard_level_metrics", state.shardLevelMetrics)

tags, err := conn.ListTagsForStream(&kinesis.ListTagsForStreamInput{
StreamName: aws.String(sn),
})
tags, err := keyvaluetags.KinesisListTags(conn, sn)

if err != nil {
return err
return fmt.Errorf("error listing tags for Kinesis Stream (%s): %s", sn, err)
}

if err := d.Set("tags", tags.IgnoreAws().Map()); err != nil {
return fmt.Errorf("error setting tags: %s", err)
}
d.Set("tags", tagsToMapKinesis(tags.Tags))

return nil
}
5 changes: 5 additions & 0 deletions aws/internal/keyvaluetags/generators/listtags/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ var serviceNames = []string{
"iotanalytics",
"iotevents",
"kafka",
"kinesis",
"kinesisanalytics",
"kinesisanalyticsv2",
"kms",
Expand Down Expand Up @@ -225,6 +226,8 @@ func ServiceListTagsFunction(serviceName string) string {
return "ListTagsForDeliveryStream"
case "glue":
return "GetTags"
case "kinesis":
return "ListTagsForStream"
case "kms":
return "ListResourceTags"
case "lambda":
Expand Down Expand Up @@ -289,6 +292,8 @@ func ServiceListTagsInputIdentifierField(serviceName string) string {
return "DeliveryStreamName"
case "fsx":
return "ResourceARN"
case "kinesis":
return "StreamName"
case "kinesisanalytics":
return "ResourceARN"
case "kinesisanalyticsv2":
Expand Down
36 changes: 36 additions & 0 deletions aws/internal/keyvaluetags/generators/updatetags/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,27 @@ case "datapipeline":
return "AddTags"
```

#### ServiceTagInputCustomValue

Given the following compilation errors:

```text
aws/internal/keyvaluetags/update_tags_gen.go:1994:4: cannot use updatedTags.IgnoreAws().KinesisTags() (type []*kinesis.Tag) as type map[string]*string in field value
```

or

```text
aws/internal/keyvaluetags/update_tags_gen.go:2534:4: cannot use updatedTags.IgnoreAws().PinpointTags() (type map[string]*string) as type *pinpoint.TagsModel in field value
```

The value of the tags for tagging must be transformed. Add an entry within the `ServiceTagInputCustomValue()` function of the generator to return the custom value. In the above case:

```go
case "kinesis":
return "aws.StringMap(chunk.IgnoreAws().Map())"
```

#### ServiceTagInputIdentifierField

Given the following compilation error:
Expand All @@ -118,6 +139,21 @@ case "athena":
return "ResourceARN"
```

#### ServiceTagInputIdentifierRequiresSlice

Given the following compilation error:

```text
aws/internal/keyvaluetags/update_tags_gen.go:1296:4: cannot use aws.String(identifier) (type *string) as type []*string in field value
```

The value to identify the resource for tagging must be passed in a string slice. Add an entry within the `ServiceTagInputIdentifierRequiresSlice()` function of the generator to ensure that the value is passed as expected. In the above case

```go
case "ec2":
return "yes"
```

#### ServiceTagInputTagsField

Given the following compilation error:
Expand Down
89 changes: 89 additions & 0 deletions aws/internal/keyvaluetags/generators/updatetags/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ var serviceNames = []string{
"iotanalytics",
"iotevents",
"kafka",
"kinesis",
"kinesisanalytics",
"kinesisanalyticsv2",
"kms",
Expand Down Expand Up @@ -120,6 +121,8 @@ func main() {
templateFuncMap := template.FuncMap{
"ClientType": keyvaluetags.ServiceClientType,
"TagFunction": ServiceTagFunction,
"TagFunctionBatchSize": ServiceTagFunctionBatchSize,
"TagInputCustomValue": ServiceTagInputCustomValue,
"TagInputIdentifierField": ServiceTagInputIdentifierField,
"TagInputIdentifierRequiresSlice": ServiceTagInputIdentifierRequiresSlice,
"TagInputResourceTypeField": ServiceTagInputResourceTypeField,
Expand Down Expand Up @@ -188,6 +191,33 @@ func {{ . | Title }}UpdateTags(conn {{ . | ClientType }}, identifier string{{ if
newTags := New(newTagsMap)

if removedTags := oldTags.Removed(newTags); len(removedTags) > 0 {
{{- 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 }}
Comment on lines +194 to +220
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR submitted: #11502

Copy link
Contributor Author

@ewbankkit ewbankkit Jan 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 }}

input := &{{ . | TagPackage }}.{{ . | UntagFunction }}Input{
{{- if . | TagInputIdentifierRequiresSlice }}
{{ . | TagInputIdentifierField }}: aws.StringSlice([]string{identifier}),
Expand All @@ -209,9 +239,37 @@ func {{ . | Title }}UpdateTags(conn {{ . | ClientType }}, identifier string{{ if
if err != nil {
return fmt.Errorf("error untagging resource (%s): %w", identifier, err)
}
{{- end }}
}

if updatedTags := oldTags.Updated(newTags); len(updatedTags) > 0 {
{{- if . | TagFunctionBatchSize }}
chunks := updatedTags.Chunks({{ . | TagFunctionBatchSize }})

for _, chunk := range chunks {
input := &{{ . | TagPackage }}.{{ . | TagFunction }}Input{
{{- if . | TagInputIdentifierRequiresSlice }}
{{ . | TagInputIdentifierField }}: aws.StringSlice([]string{identifier}),
{{- else }}
{{ . | TagInputIdentifierField }}: aws.String(identifier),
{{- end }}
{{- if . | TagInputResourceTypeField }}
{{ . | TagInputResourceTypeField }}: aws.String(resourceType),
{{- end }}
{{- if . | TagInputCustomValue }}
{{ . | TagInputTagsField }}: {{ . | TagInputCustomValue }},
{{- else }}
{{ . | TagInputTagsField }}: chunk.IgnoreAws().{{ . | Title }}Tags(),
{{- end }}
}

_, err := conn.{{ . | TagFunction }}(input)

if err != nil {
return fmt.Errorf("error tagging resource (%s): %w", identifier, err)
}
}
{{- else }}
input := &{{ . | TagPackage }}.{{ . | TagFunction }}Input{
{{- if . | TagInputIdentifierRequiresSlice }}
{{ . | TagInputIdentifierField }}: aws.StringSlice([]string{identifier}),
Expand All @@ -221,14 +279,19 @@ func {{ . | Title }}UpdateTags(conn {{ . | ClientType }}, identifier string{{ if
{{- if . | TagInputResourceTypeField }}
{{ . | TagInputResourceTypeField }}: aws.String(resourceType),
{{- end }}
{{- if . | TagInputCustomValue }}
{{ . | TagInputTagsField }}: {{ . | TagInputCustomValue }},
{{- else }}
{{ . | TagInputTagsField }}: updatedTags.IgnoreAws().{{ . | Title }}Tags(),
{{- end }}
}

_, err := conn.{{ . | TagFunction }}(input)

if err != nil {
return fmt.Errorf("error tagging resource (%s): %w", identifier, err)
}
{{- end }}
}

return nil
Expand Down Expand Up @@ -269,6 +332,8 @@ func ServiceTagFunction(serviceName string) string {
return "AddTags"
case "firehose":
return "TagDeliveryStream"
case "kinesis":
return "AddTagsToStream"
case "medialive":
return "CreateTags"
case "mq":
Expand Down Expand Up @@ -298,6 +363,16 @@ func ServiceTagFunction(serviceName string) string {
}
}

// ServiceTagFunctionBatchSize determines the batch size (if any) for tagging and untagging.
func ServiceTagFunctionBatchSize(serviceName string) string {
switch serviceName {
case "kinesis":
return "10"
default:
return ""
}
}

// ServiceTagInputIdentifierField determines the service tag identifier field.
func ServiceTagInputIdentifierField(serviceName string) string {
switch serviceName {
Expand Down Expand Up @@ -343,6 +418,8 @@ func ServiceTagInputIdentifierField(serviceName string) string {
return "DeliveryStreamName"
case "fsx":
return "ResourceARN"
case "kinesis":
return "StreamName"
case "kinesisanalytics":
return "ResourceARN"
case "kinesisanalyticsv2":
Expand Down Expand Up @@ -422,6 +499,16 @@ func ServiceTagInputTagsField(serviceName string) string {
}
}

// ServiceTagInputCustomValue determines any custom value for the service tagging tags field.
func ServiceTagInputCustomValue(serviceName string) string {
switch serviceName {
case "kinesis":
return "aws.StringMap(chunk.IgnoreAws().Map())"
default:
return ""
}
}

// ServiceTagInputResourceTypeField determines the service tagging resource type field.
func ServiceTagInputResourceTypeField(serviceName string) string {
switch serviceName {
Expand Down Expand Up @@ -467,6 +554,8 @@ func ServiceUntagFunction(serviceName string) string {
return "RemoveTags"
case "firehose":
return "UntagDeliveryStream"
case "kinesis":
return "RemoveTagsFromStream"
case "medialive":
return "DeleteTags"
case "mq":
Expand Down
20 changes: 20 additions & 0 deletions aws/internal/keyvaluetags/key_value_tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,26 @@ func (tags KeyValueTags) Updated(newTags KeyValueTags) KeyValueTags {
return result
}

// Chunks returns a slice of KeyValueTags, each of the specified size.
func (tags KeyValueTags) Chunks(size int) []KeyValueTags {
result := []KeyValueTags{}

i := 0
var chunk KeyValueTags
for k, v := range tags {
if i%size == 0 {
chunk = make(KeyValueTags)
result = append(result, chunk)
}

chunk[k] = v

i++
}

return result
}

// New creates KeyValueTags from common Terraform Provider SDK types.
// Supports map[string]string, map[string]*string, map[string]interface{}, and []interface{}.
// When passed []interface{}, all elements are treated as keys and assigned nil values.
Expand Down
Loading