-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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
don't force stream destroy on shard_count update #894
Conversation
Hi @nevins-b Thanks for the work here - please could you add an acceptance test to show the new functionality in action? As part of the acceptance test, it would be good to see that the kinesis stream didn't actually get recreated Thanks Paul |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nevins-b
I left 2 comments inline - these need updated or we get a panic as follows:
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSKinesisStream_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSKinesisStream_ -timeout 120m
=== RUN TestAccAWSKinesisStream_basic
panic: interface conversion: interface {} is int, not int64
goroutine 129 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.updateKinesisShardCount(0xc4203de3e8, 0xc42016ae70, 0xc42070b3b8, 0xc4206f22c8)
/Users/stacko/Code/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_kinesis_stream.go:252 +0x554
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsKinesisStreamUpdate(0xc42016ae70, 0x235db00, 0xc4207b4400, 0x22ca860, 0xc4203fc8a8)
/Users/stacko/Code/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_kinesis_stream.go:125 +0xc0
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsKinesisStreamCreate(0xc42016ae70, 0x235db00, 0xc4207b4400, 0x0, 0x0)
/Users/stacko/Code/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_kinesis_stream.go:111 +0x774
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc4202d18c0, 0xc4201d66e0, 0xc4201d2380, 0x235db00, 0xc4207b4400, 0xc4207d3d01, 0x3e, 0x0)
/Users/stacko/Code/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:186 +0x48d
Once i updated these locally, I got the tests to run, but the shardCount test failed:
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSKinesisStream_' 2 ↵ ✹
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSKinesisStream_ -timeout 120m
=== RUN TestAccAWSKinesisStream_basic
--- PASS: TestAccAWSKinesisStream_basic (98.59s)
=== RUN TestAccAWSKinesisStream_importBasic
--- PASS: TestAccAWSKinesisStream_importBasic (100.75s)
=== RUN TestAccAWSKinesisStream_shardCount
--- FAIL: TestAccAWSKinesisStream_shardCount (200.80s)
testing.go:428: Step 1 error: Check failed: Check 2/4 error: Bad Stream Shard Count
expected: 4
got: 6
Please can you take a look?
Thanks
Paul
aws/resource_aws_kinesis_stream.go
Outdated
sn := d.Get("name").(string) | ||
|
||
oraw, nraw := d.GetChange("shard_count") | ||
o := oraw.(int64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't case to int64, it needs to be int
aws/resource_aws_kinesis_stream.go
Outdated
log.Printf("[DEBUG] Change %s Stream ShardCount to %d", sn, n) | ||
_, err := conn.UpdateShardCount(&kinesis.UpdateShardCountInput{ | ||
StreamName: aws.String(sn), | ||
TargetShardCount: aws.Int64(n), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be:
TargetShardCount: aws.Int64(int64(n)),
Looks like it's working now, I forgot that you needed to look at open shards only
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nevins-b
This LGTM now - thanks for all the work here
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSKinesisStream_ -timeout 120m
=== RUN TestAccAWSKinesisStream_basic
--- PASS: TestAccAWSKinesisStream_basic (104.19s)
=== RUN TestAccAWSKinesisStream_importBasic
--- PASS: TestAccAWSKinesisStream_importBasic (101.48s)
=== RUN TestAccAWSKinesisStream_shardCount
--- PASS: TestAccAWSKinesisStream_shardCount (223.59s)
=== RUN TestAccAWSKinesisStream_retentionPeriod
--- PASS: TestAccAWSKinesisStream_retentionPeriod (183.73s)
=== RUN TestAccAWSKinesisStream_shardLevelMetrics
--- PASS: TestAccAWSKinesisStream_shardLevelMetrics (212.04s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 825.061s
Paul
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! |
Addresses #521