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

Jump to bottom

resource/aws_api_gateway_usag_plan: Fixed setting of rate_limit #2076

Merged
merged 1 commit into from
Oct 27, 2017
Merged

resource/aws_api_gateway_usag_plan: Fixed setting of rate_limit #2076

merged 1 commit into from
Oct 27, 2017

Conversation

Ninir
Copy link
Contributor

@Ninir Ninir commented Oct 26, 2017 •
edited
Loading

Fixes #2057

This fixes the fact to correctly send the value on creation, due to the casting made: as the schema type is of type int and we try to check if the value cast as a float64 is ok, the check was always ignored on creation.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayUsagePlan_'                               
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayUsagePlan_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayUsagePlan_importBasic
--- PASS: TestAccAWSAPIGatewayUsagePlan_importBasic (18.08s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_basic
--- PASS: TestAccAWSAPIGatewayUsagePlan_basic (98.03s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_description
--- PASS: TestAccAWSAPIGatewayUsagePlan_description (83.69s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_productCode
--- PASS: TestAccAWSAPIGatewayUsagePlan_productCode (78.23s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_throttling
--- PASS: TestAccAWSAPIGatewayUsagePlan_throttling (59.58s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_quota
--- PASS: TestAccAWSAPIGatewayUsagePlan_quota (73.80s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_apiStages
--- PASS: TestAccAWSAPIGatewayUsagePlan_apiStages (87.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	499.266s

radeksimko
radeksimko reviewed Oct 27, 2017
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Do you mind adding an acceptance test (or modifying an existing one) with this field confirming the bug is fixed and remains that way? 🙂

aws/resource_aws_api_gateway_usage_plan.go Outdated
@@ -355,7 +355,7 @@ func resourceAwsApiGatewayUsagePlanUpdate(d *schema.ResourceData, meta interface
operations = append(operations, &apigateway.PatchOperation{
Op: aws.String("replace"),
Path: aws.String("/throttle/rateLimit"),
Value: aws.String(strconv.Itoa(d["rate_limit"].(int))),
Value: aws.String(strconv.FormatFloat(d["rate_limit"].(float64), 'E', -1, 64)),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a nitpick, but is there any value in exposing the exponent? i.e. in using E? Is the API actually going to parse it correctly and do something with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably no value, preferred the use of the f format instead

aws/resource_aws_api_gateway_usage_plan.go Outdated
@@ -369,7 +369,7 @@ func resourceAwsApiGatewayUsagePlanUpdate(d *schema.ResourceData, meta interface
operations = append(operations, &apigateway.PatchOperation{
Op: aws.String("add"),
Path: aws.String("/throttle/rateLimit"),
Value: aws.String(strconv.Itoa(d["rate_limit"].(int))),
Value: aws.String(strconv.FormatFloat(d["rate_limit"].(float64), 'E', -1, 64)),
Copy link
Member

Choose a reason for hiding this comment

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

Same question here ^

@radeksimko radeksimko added bug Addresses a defect in current functionality. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 27, 2017
@Ninir
Copy link
Contributor Author

Ninir commented Oct 27, 2017

Here we go!

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayUsagePlan_throttlingInitialRateLimit'      
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayUsagePlan_throttlingInitialRateLimit -timeout 120m
=== RUN   TestAccAWSAPIGatewayUsagePlan_throttlingInitialRateLimit
--- PASS: TestAccAWSAPIGatewayUsagePlan_throttlingInitialRateLimit (18.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	18.739s

radeksimko
radeksimko approved these changes Oct 27, 2017
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 27, 2017
@Ninir Ninir merged commit b66f6b6 into hashicorp:master Oct 27, 2017
@Ninir Ninir deleted the b-usageplan-ratelimit branch October 27, 2017 10:56
@ghost
Copy link

ghost commented Apr 10, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws_api_gateway_usage_plan rate_limit does not "take" the first time
2 participants
@Ninir @radeksimko