aws: add pagination support to the findRecord func for route53 #3900
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bflad
added
service/route53
|
Hi @benburkert 👋 Is there any reason to not implement the AWS SDK paginator? https://docs.aws.amazon.com/sdk-for-go/api/service/route53/#Route53.ListResourceRecordSetsPages |
bflad
added
the
waiting-response
The ListResourceRecordSets API call may return ALIAS records that point to the record specified by the "name" parameter. These records may appear first because the results are lexicographically ordered. When there are more than 100 of these records they can cause the target record to be part of the truncated set. Check for a truncated indicator in the ListResourceRecordSetsOutput. If the response is truncated, repeat the API call with an updated starting name, type, and identifier. If the response is not truncated, return an r53NoRecordsFound.
|
hi @bflad, I was unaware of the |
bflad
removed
the
waiting-response
bflad
reviewed
Mar 29, 2018
aws/resource_aws_route53_record.go
Outdated
| } | ||
| var record *route53.ResourceRecordSet | ||
| err = conn.ListResourceRecordSetsPages(lopts, func(resp *route53.ListResourceRecordSetsOutput, lastPage bool) bool { | ||
| for _, record = range resp.ResourceRecordSets { |
There was a problem hiding this comment.
For clarity, we should utilize a separate variable here instead of the record we want to return, e.g.
for _, recordSet := range respResourceRecordSets {Then we can set record to the correct value when its correctly found and not introduce the awkward record = nil each loop.
aws/resource_aws_route53_record.go
Outdated
| return record, nil | ||
|
|
||
| record = nil | ||
| return true |
There was a problem hiding this comment.
Shouldn't this be return !lastPage so we keep looping until the last page?
There was a problem hiding this comment.
Doesn't seem to make a difference, but I've gone ahead and changed it. 👍
bflad
approved these changes
Apr 2, 2018
There was a problem hiding this comment.
LGTM, thanks! 🚀
24 tests passed (all tests)
=== RUN TestAccAWSRoute53Record_weighted_basic
--- PASS: TestAccAWSRoute53Record_weighted_basic (163.11s)
=== RUN TestAccAWSRoute53Record_basic
--- PASS: TestAccAWSRoute53Record_basic (182.25s)
=== RUN TestAccAWSRoute53Record_spfSupport
--- PASS: TestAccAWSRoute53Record_spfSupport (186.18s)
=== RUN TestAccAWSRoute53Record_s3_alias
--- PASS: TestAccAWSRoute53Record_s3_alias (190.88s)
=== RUN TestAccAWSRoute53Record_generatesSuffix
--- PASS: TestAccAWSRoute53Record_generatesSuffix (191.76s)
=== RUN TestAccAWSRoute53Record_caaSupport
--- PASS: TestAccAWSRoute53Record_caaSupport (193.40s)
=== RUN TestAccAWSRoute53Record_importUnderscored
--- PASS: TestAccAWSRoute53Record_importUnderscored (193.48s)
=== RUN TestAccAWSRoute53Record_aliasUppercase
--- PASS: TestAccAWSRoute53Record_aliasUppercase (195.51s)
=== RUN TestAccAWSRoute53Record_geolocation_basic
--- PASS: TestAccAWSRoute53Record_geolocation_basic (218.63s)
=== RUN TestAccAWSRoute53Record_latency_basic
--- PASS: TestAccAWSRoute53Record_latency_basic (222.98s)
=== RUN TestAccAWSRoute53Record_failover
--- PASS: TestAccAWSRoute53Record_failover (227.96s)
=== RUN TestAccAWSRoute53Record_txtSupport
--- PASS: TestAccAWSRoute53Record_txtSupport (229.74s)
=== RUN TestAccAWSRoute53Record_alias
--- PASS: TestAccAWSRoute53Record_alias (230.42s)
=== RUN TestAccAWSRoute53Record_TypeChange
--- PASS: TestAccAWSRoute53Record_TypeChange (242.84s)
=== RUN TestAccAWSRoute53Record_basic_fqdn
--- PASS: TestAccAWSRoute53Record_basic_fqdn (244.98s)
=== RUN TestAccAWSRoute53Record_SetIdentiferChange
--- PASS: TestAccAWSRoute53Record_SetIdentiferChange (247.43s)
=== RUN TestAccAWSRoute53Record_importBasic
--- PASS: TestAccAWSRoute53Record_importBasic (250.40s)
=== RUN TestAccAWSRoute53Record_AliasChange
--- PASS: TestAccAWSRoute53Record_AliasChange (280.04s)
=== RUN TestAccAWSRoute53Record_wildcard
--- PASS: TestAccAWSRoute53Record_wildcard (287.57s)
=== RUN TestAccAWSRoute53Record_empty
--- PASS: TestAccAWSRoute53Record_empty (153.92s)
=== RUN TestAccAWSRoute53Record_longTXTrecord
--- PASS: TestAccAWSRoute53Record_longTXTrecord (151.72s)
=== RUN TestAccAWSRoute53Record_multivalue_answer_basic
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (172.75s)
=== RUN TestAccAWSRoute53Record_weighted_alias
--- PASS: TestAccAWSRoute53Record_weighted_alias (387.20s)
=== RUN TestAccAWSRoute53Record_allowOverwrite
--- PASS: TestAccAWSRoute53Record_allowOverwrite (224.86s)
bflad
added a commit
that referenced
this pull request
Apr 2, 2018
|
This has been released in version 1.14.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ListResourceRecordSetsAPI call may returnALIASrecords that point to the record specified by the "name" parameter. These records may appear first because the results are lexicographically ordered. When there are more than 100 of these records they can cause the target record to be part of the truncated set.Check for a truncated indicator in the
ListResourceRecordSetsOutput. If the response is truncated, repeat the API call with an updated starting name, type, and identifier. If the response is not truncated, return anr53NoRecordsFound.