r/aws_msk_cluster - new storage_info arg #24767

GlennChia · 2022-05-13T04:48:59Z

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

Output from acceptance testing:

$  make testacc TESTARGS='-run=TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolume\|TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo\|TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize' PKG=kafka
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/kafka/... -v -count 1 -parallel 20  -run=TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolume\|TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo\|TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize -timeout 180m
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo (2134.04s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize (2138.68s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo (5049.87s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/kafka      5049.916s
...

deprecate ebs_volume_size

considers refactor from deprecated ebs_volume_size to storage_info

accept api err of no updates to ebs volume

deprecate ebs_volume_size and replace with storage_info

AdamTylerLynch · 2022-05-16T16:47:52Z

make testacc TESTS=TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo PKG=kafka 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/kafka/... -v -count 1 -parallel 20 -run='TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo'  -timeout 180m
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo (1591.54s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/kafka	1596.532s
make testacc TESTS=TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo PKG=kafka 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/kafka/... -v -count 1 -parallel 20 -run='TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo'  -timeout 180m
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo (4886.87s)
PASS

AdamTylerLynch · 2022-05-16T16:48:01Z

LGTM 🚀

gdavison

Thanks for the PR, @GlennChia. It's looking good. I have a few suggestions for updates

website/docs/r/msk_cluster.html.markdown

gdavison · 2022-05-16T20:58:41Z

internal/service/kafka/cluster_test.go

+		CheckDestroy:      testAccCheckClusterDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccClusterBrokerNodeGroupInfoStorageInfoConfig2(rName, original_volume_size, "kafka.m5.4xlarge"),


testAccClusterBrokerNodeGroupInfoStorageInfoConfig1, testAccClusterBrokerNodeGroupInfoStorageInfoConfig2, and testAccClusterBrokerNodeGroupInfoStorageInfoConfig3 should have more descriptive names identifying which parameters are set

Resolved in 4cc2c44

gdavison · 2022-05-16T21:16:25Z

internal/service/kafka/cluster_test.go

+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"current_version",
+				},
+			},


This import isn't needed, since other tests cover importing a configuration using the deprecated broker_node_group_info. ebs_volume_size

Resolved in 3158ad8

gdavison · 2022-05-16T21:17:17Z

internal/service/kafka/cluster_test.go

+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"current_version",
+				},
+			},


This import isn't needed, since other tests cover importing a configuration using broker_node_group_info.storage_info.ebs_storage_info.volume_size

Resolved in 3158ad8

gdavison · 2022-05-16T21:43:21Z

internal/service/kafka/cluster.go

+		}
+	}
+
+	if d.HasChange("broker_node_group_info.0.storage_info") {


The way these conditionals are ordered could lead to two UpdateBrokerStorageWithContext requests to the API. The top-level conditional could instead be if d.HasChanges("broker_node_group_info.0.ebs_volume_size", "broker_node_group_info.0.storage_info"). From there you could build the kafka.UpdateBrokerStorageInput based on what parameters are set

Resolved in 917a661

gdavison · 2022-05-16T22:02:30Z

internal/service/kafka/cluster.go

+		input := &kafka.UpdateBrokerStorageInput{
+			ClusterArn:     aws.String(d.Id()),
+			CurrentVersion: aws.String(d.Get("current_version").(string)),
+		}
+		if v, ok := d.GetOk("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.provisioned_throughput"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {
+			input.TargetBrokerEBSVolumeInfo = []*kafka.BrokerEBSVolumeInfo{
+				{
+					KafkaBrokerNodeId:     aws.String("All"),
+					ProvisionedThroughput: expandProvisionedThroughput(v.([]interface{})[0].(map[string]interface{})),
+					VolumeSizeGB:          aws.Int64(int64(d.Get("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.volume_size").(int))),
+				},
+			}
+
+		} else {
+			input.TargetBrokerEBSVolumeInfo = []*kafka.BrokerEBSVolumeInfo{
+				{
+					KafkaBrokerNodeId: aws.String("All"),
+					VolumeSizeGB:      aws.Int64(int64(d.Get("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.volume_size").(int))),
+				},
+			}
+		}


This could be simplified to something like the following

Suggested change

input := &kafka.UpdateBrokerStorageInput{

ClusterArn: aws.String(d.Id()),

CurrentVersion: aws.String(d.Get("current_version").(string)),

}

if v, ok := d.GetOk("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.provisioned_throughput"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {

input.TargetBrokerEBSVolumeInfo = []*kafka.BrokerEBSVolumeInfo{

{

KafkaBrokerNodeId: aws.String("All"),

ProvisionedThroughput: expandProvisionedThroughput(v.([]interface{})[0].(map[string]interface{})),

VolumeSizeGB: aws.Int64(int64(d.Get("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.volume_size").(int))),

},

}

} else {

input.TargetBrokerEBSVolumeInfo = []*kafka.BrokerEBSVolumeInfo{

{

KafkaBrokerNodeId: aws.String("All"),

VolumeSizeGB: aws.Int64(int64(d.Get("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.volume_size").(int))),

},

}

}

input := &kafka.UpdateBrokerStorageInput{

ClusterArn: aws.String(d.Id()),

CurrentVersion: aws.String(d.Get("current_version").(string)),

}

ebsVolumeInfo := &kafka.BrokerEBSVolumeInfo{

KafkaBrokerNodeId: aws.String("All"),

VolumeSizeGB: aws.Int64(int64(d.Get("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.volume_size").(int))),

}

if v, ok := d.GetOk("broker_node_group_info.0.storage_info.0.ebs_storage_info.0.provisioned_throughput"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {

ebsVolumeInfo.ProvisionedThroughput: expandProvisionedThroughput(v.([]interface{})[0].(map[string]interface{}))

}

input.TargetBrokerEBSVolumeInfo = []*kafka.BrokerEBSVolumeInfo{ebsVolumeInfo}

Resolved in 9766872

internal/service/kafka/cluster_test.go

Co-authored-by: Graham Davison <g.m.davison@computer.org>

GlennChia · 2022-05-17T07:15:54Z

Thanks for the review @gdavison. In addition, I also removed the import test in 6e7218a after upgrading the instance type. This is because TestAccKafkaCluster_BrokerNodeGroupInfo_instanceType also does that import check. Re-ran the test cases (just for the BrokerNodeGroupInfo ones) gives the following

$ make testacc TESTARGS='-run=TestAccKafkaCluster_BrokerNodeGroupInfo' PKG=kafka
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/kafka/... -v -count 1 -parallel 20  -run=TestAccKafkaCluster_BrokerNodeGroupInfo -timeout 180m
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_instanceType
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_instanceType
=== RUN   TestAccKafkaCluster_BrokerNodeGroupInfo_publicAccessSaslIam
=== PAUSE TestAccKafkaCluster_BrokerNodeGroupInfo_publicAccessSaslIam
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_instanceType
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_publicAccessSaslIam
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo
=== CONT  TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo (2226.55s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize (2230.19s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo (4659.47s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_publicAccessSaslIam (6049.81s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_instanceType (6664.00s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/kafka      6664.050s

gdavison

Looks good, @GlennChia! 🚀

--- PASS: TestAccKafkaCluster_enhancedMonitoring (1517.86s)
--- PASS: TestAccKafkaCluster_EncryptionInfo_encryptionAtRestKMSKeyARN (1522.11s)
--- PASS: TestAccKafkaCluster_EncryptionInfoEncryptionInTransit_inCluster (1523.78s)
--- PASS: TestAccKafkaCluster_EncryptionInfoEncryptionInTransit_clientBroker (1524.23s)
--- PASS: TestAccKafkaCluster_tags (1525.65s)
--- PASS: TestAccKafkaClusterDataSource_basic (1581.76s)
--- PASS: TestAccKafkaCluster_ClientAuthenticationTLS_certificateAuthorityARNs (1608.87s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_storageInfo (1713.09s)
--- PASS: TestAccKafkaCluster_loggingInfo (1758.40s)
--- PASS: TestAccKafkaCluster_openMonitoring (1786.87s)
--- PASS: TestAccKafkaCluster_Info_revision (1947.46s)
--- PASS: TestAccKafkaCluster_numberOfBrokerNodes (2520.16s)
--- PASS: TestAccKafkaCluster_kafkaVersionDowngrade (2993.91s)
--- PASS: TestAccKafkaCluster_basic (1478.53s)
--- PASS: TestAccKafkaCluster_ClientAuthenticationSASL_scram (3024.41s)
--- PASS: TestAccKafkaCluster_ClientAuthenticationSASL_iam (3170.76s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_ebsVolumeSize (1765.18s)
--- PASS: TestAccKafkaCluster_ClientAuthenticationTLS_initiallyNoAuthentication (3449.99s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_modifyEbsVolumeSizeToStorageInfo (4556.84s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_publicAccessSaslIam (5109.58s)
--- PASS: TestAccKafkaCluster_kafkaVersionUpgradeWithInfo (5676.89s)
--- PASS: TestAccKafkaCluster_BrokerNodeGroupInfo_instanceType (6346.58s)
--- PASS: TestAccKafkaCluster_kafkaVersionUpgrade (5588.27s)

github-actions · 2022-05-20T05:06:53Z

This functionality has been released in v4.15.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. Thank you!

github-actions · 2022-06-26T02:39:56Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

GlennChia added 8 commits May 13, 2022 03:42

feat(kafka): add storage_info arg

c5f751e

deprecate ebs_volume_size

feat(kafka): customdiff fix ImportState test drift

baeb31f

feat(kafka): add expand storage_info funcs

7f5f1cc

feat(kafka): add flatten storage_info funcs

0fdcbbc

feat(kafka): add update for storage_info

2968247

considers refactor from deprecated ebs_volume_size to storage_info

feat(kafka): ebs_volume_size refact wo del storage

f0c3dba

accept api err of no updates to ebs volume

test(kafka): storage_info update volume_size

994d334

test(kafka): storage_info update volume_size

b32a8bb

GlennChia added 3 commits May 13, 2022 05:09

docs(kafka): rsrc add storage_info desc, example

7120ba1

docs(kafka): rsrc deprecate ebs_volume_size

1595c08

ci(kafka): aws_msk_cluster changelog - refact arg

daedf01

deprecate ebs_volume_size and replace with storage_info

GlennChia force-pushed the f-aws_msk_cluster-storage-info-arg branch from c0068d0 to daedf01 Compare May 13, 2022 05:18

GlennChia marked this pull request as ready for review May 13, 2022 06:51

justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels May 13, 2022

gdavison self-assigned this May 16, 2022

gdavison requested changes May 16, 2022

View reviewed changes

GlennChia and others added 4 commits May 17, 2022 08:28

docs(kafka): use dynamic ref to sec group id

d4c8e4d

Co-authored-by: Graham Davison <g.m.davison@computer.org>

docs(kafka): use dynamic ref to client_subnet ids

3bff530

docs(kafka): ref storage_info in deprecated arg

0f8a107

refactor(kafka): simplify ProvThroughput arg check

9766872

Co-authored-by: Graham Davison <g.m.davison@computer.org>

GlennChia force-pushed the f-aws_msk_cluster-storage-info-arg branch from 353d2b0 to 9766872 Compare May 17, 2022 04:50

refactor(kafka): reduce updates for storage_info

917a661

GlennChia added 6 commits May 17, 2022 04:52

test(kafka): rename config to indicate deprecated

781ad78

test(kafka): rename storage info test configs

4cc2c44

test(kafka): deprecated ebs_volume_size still set

2b8396f

test(kafka): remove extra import checks

3158ad8

test(kafka): refactor to PlanOnly since no changes

ed14c7c

test(kafka): rm import test after update inst type

6e7218a

AdamTylerLynch requested a review from gdavison May 18, 2022 12:35

gdavison approved these changes May 18, 2022

View reviewed changes

gdavison merged commit 2fbf08c into hashicorp:main May 18, 2022

github-actions bot added this to the v4.15.0 milestone May 18, 2022

GlennChia deleted the f-aws_msk_cluster-storage-info-arg branch May 19, 2022 06:57

GlennChia mentioned this pull request May 26, 2022

Add support for aws_msk_cluster multi update #25061

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2022

justinretzolk added the partner Contribution from a partner. label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_msk_cluster - new storage_info arg #24767

r/aws_msk_cluster - new storage_info arg #24767

GlennChia commented May 13, 2022 •

edited

Loading

AdamTylerLynch commented May 16, 2022

AdamTylerLynch commented May 16, 2022

gdavison left a comment

gdavison May 16, 2022

GlennChia May 17, 2022 •

edited

Loading

gdavison May 16, 2022

GlennChia May 17, 2022

gdavison May 16, 2022

GlennChia May 17, 2022

gdavison May 16, 2022

GlennChia May 17, 2022

gdavison May 16, 2022

GlennChia May 17, 2022

GlennChia commented May 17, 2022 •

edited

Loading

gdavison left a comment

github-actions bot commented May 20, 2022

github-actions bot commented Jun 26, 2022

r/aws_msk_cluster - new storage_info arg #24767

r/aws_msk_cluster - new storage_info arg #24767

Conversation

GlennChia commented May 13, 2022 • edited Loading

Community Note

AdamTylerLynch commented May 16, 2022

AdamTylerLynch commented May 16, 2022

gdavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlennChia May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlennChia commented May 17, 2022 • edited Loading

gdavison left a comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2022

github-actions bot commented Jun 26, 2022

GlennChia commented May 13, 2022 •

edited

Loading

GlennChia May 17, 2022 •

edited

Loading

GlennChia commented May 17, 2022 •

edited

Loading