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.

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

dns: update ListDNSRecords default per_page attribute to 100 records #1171

Merged

Conversation

arturhoo
Copy link
Contributor

@arturhoo arturhoo commented Jan 9, 2023

Bump the PerPage value used when auto-pagination is performed on ListDNSRecords to 100, matching that of the API: https://api.cloudflare.com/#dns-records-for-a-zone-list-dns-records

Description

As outlined in #1169, when no pagination options are passed in to ListDNSRecords, auto- pagination is performed. However, the current per_page value doesn't match that of the official API (50 vs 100). This commit makes both values match.

Has your change been tested?

A new test was implemented that more explicitly tests the auto pagination behaviour.

Screenshots (if appropriate):

N/A

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

None of the above. I'd classify this as an enhancement, per the discussion on #1169

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

@arturhoo arturhoo force-pushed the list-dns-auto-pagination-size branch from f1a4010 to f67ea98 Compare January 9, 2023 10:02
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

changelog detected ✅

@arturhoo arturhoo marked this pull request as ready for review January 9, 2023 10:04
Artur Rodrigues added 2 commits January 9, 2023 22:21
As outlined in github.com/cloudflare/pull/1169,
when no pagination options are passed in to ListDNSRecords,
autopagination is performed. However, the current PerPage
value doesn't match that of the official API. This commit
makes both values match. A new test is also implemented that
more explicitly tests the auto pagination behaviour.

Signed-off-by: Artur Rodrigues <[email protected]>
Signed-off-by: Artur Rodrigues <[email protected]>
@arturhoo arturhoo force-pushed the list-dns-auto-pagination-size branch from f67ea98 to ff35c3c Compare January 9, 2023 22:23
dns.go Show resolved Hide resolved
dns_test.go Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #1171 (99374ea) into master (6153c1e) will decrease coverage by 0.06%.
The diff coverage is 46.94%.

@@            Coverage Diff             @@
##           master    #1171      +/-   ##
==========================================
- Coverage   49.40%   49.33%   -0.07%     
==========================================
  Files         127      128       +1     
  Lines       12290    12416     +126     
==========================================
+ Hits         6072     6126      +54     
- Misses       4840     4886      +46     
- Partials     1378     1404      +26     
Impacted Files Coverage Δ
access_organization.go 64.70% <ø> (ø)
cloudflare_experimental.go 0.00% <0.00%> (ø)
utils.go 72.72% <ø> (ø)
cloudflare.go 68.37% <14.28%> (-0.34%) ⬇️
mtls_certificates.go 26.59% <26.59%> (ø)
origin_ca.go 57.26% <90.90%> (-2.09%) ⬇️
dns.go 68.69% <92.45%> (+4.97%) ⬆️
email_routing_destination.go 66.66% <100.00%> (+0.41%) ⬆️
email_routing_rules.go 65.64% <100.00%> (+0.26%) ⬆️
filter.go 43.38% <100.00%> (+0.41%) ⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dns.go Outdated Show resolved Hide resolved
.changelog/1171.txt Outdated Show resolved Hide resolved
dns.go Outdated Show resolved Hide resolved
@favonia
Copy link
Contributor

favonia commented Jan 10, 2023

@jacobbednarz @arturhoo I wonder how you feel about #1163? It you agree with my proposal, it seems we can address it in this PR as well. More specifically, we can add more assertions at the end of the new TestListDNSRecordsPagination.

@arturhoo
Copy link
Contributor Author

Thanks for taking the time to review this PR @favonia

@jacobbednarz @arturhoo I wonder how you feel about #1163? It you agree with my proposal, it seems we can address it in this PR as well. More specifically, we can add more assertions at the end of the new TestListDNSRecordsPagination.

I don't have a strong opinion. As I outlined in #1169, I'd be more in favor of an explicit auto-pagination param inside ListDNSRecordsParams (or another struct that is embedded in multiple other param structs with a better name than ResultInfo).

In any case, it seems this PR here is a good step forward despite the direction we end up going?

@favonia
Copy link
Contributor

favonia commented Jan 10, 2023

In any case, it seems this PR here is a good step forward despite the direction we end up going?

It is, but it would have saved me from creating another PR. 😆

@arturhoo
Copy link
Contributor Author

Feel free to assign that one to me if it ends up being accepted 😁

fwiw, ResultInfo being returned when auto-pagination is enabled also caused some initial confusion in kubernetes-sigs/external-dns#3288 (comment)

@arturhoo arturhoo changed the title Match ListDNSRecords per_page value to the API's dns: update ListDNSRecords default per_page attribute to 100 records Jan 10, 2023
@jacobbednarz jacobbednarz merged commit 0f63f35 into cloudflare:master Jan 10, 2023
@github-actions github-actions bot added this to the v0.59.0 milestone Jan 10, 2023
@arturhoo arturhoo deleted the list-dns-auto-pagination-size branch January 10, 2023 09:31
github-actions bot pushed a commit that referenced this pull request Jan 10, 2023
@jacobbednarz
Copy link
Member

@jacobbednarz @arturhoo I wonder how you feel about #1163? It you agree with my proposal, it seems we can address it in this PR as well

let's not muddy the waters here and keep them separate. they are different concerns and need separate conversations. I'm not rushing to change that return value as we have some work coming up to revamp the internals where we can cover that off.

with a better name than ResultInfo

this has been a pet peeve of mine for a while and is addressed in the experimental client. read it as PaginationOptions for now 😉

ivan-section-io pushed a commit to section/cloudflare-go that referenced this pull request Jan 12, 2023
@github-actions
Copy link
Contributor

This functionality has been released in v0.59.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants