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

Adds recently supported per_page/pagination teams_list #1706

Merged

Conversation

AkemiDavisson
Copy link
Contributor

@AkemiDavisson AkemiDavisson commented Jun 16, 2022

@jacobbednarz
Copy link
Member

@tjstansell you've mentioned in a couple of issues you've got near 5k team list items. are you able to take this PR for a spin and confirm you get them all back and managed correctly?

@github-actions
Copy link
Contributor

changelog detected ✅

@tjstansell
Copy link
Contributor

@tjstansell you've mentioned in a couple of issues you've got near 5k team list items. are you able to take this PR for a spin and confirm you get them all back and managed correctly?

Seems like there should be a unit test for this.

@cloudflare cloudflare deleted a comment from github-actions bot Jun 22, 2022
@jacobbednarz jacobbednarz added the workflow/pending-cloudflare-response Indicates an issue or PR requires a response from the Cloudflare team. label Jun 22, 2022
@jacobbednarz
Copy link
Member

pending TotalPages to land in the API response.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2022

Marking this pull request as stale due to 14 days of inactivity. This helps our maintainers find and focus on the active pull requests. If this pull request receives no comments in the next 7 days it will automatically be closed. Maintainers can also remove the lifecycle/stale label.
If this pull request was automatically closed and you feel this pull request should be reopened, we encourage creating a new pull request linking back to this one for added context. Thank you!

@fwwieffering
Copy link

@jacobbednarz is this PR still waiting on anything?

@jacobbednarz
Copy link
Member

acceptance tests are green

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareTeamsList" -count 1 -parallel 1 -timeout 120m -parallel 1
?       github.com/cloudflare/terraform-provider-cloudflare     [no test files]
=== RUN   TestAccCloudflareTeamsListBasic
--- PASS: TestAccCloudflareTeamsListBasic (9.49s)
=== RUN   TestAccCloudflareTeamsListReordered
--- PASS: TestAccCloudflareTeamsListReordered (22.99s)
PASS
ok      github.com/cloudflare/terraform-provider-cloudflare/internal/provider   32.775s
?       github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check   [no test files]
?       github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/maintainer-only-file-check        [no test files]
?       github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/tf-log-check      [no test files]
?       github.com/cloudflare/terraform-provider-cloudflare/version     [no test files]

@jacobbednarz
Copy link
Member

@fwwieffering see two comments up from yours 😄

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

Marking this pull request as stale due to 14 days of inactivity. This helps our maintainers find and focus on the active pull requests. If this pull request receives no comments in the next 7 days it will automatically be closed. Maintainers can also remove the lifecycle/stale label.
If this pull request was automatically closed and you feel this pull request should be reopened, we encourage creating a new pull request linking back to this one for added context. Thank you!

@github-actions
Copy link
Contributor

Marking this pull request as stale due to 14 days of inactivity. This helps our maintainers find and focus on the active pull requests. If this pull request receives no comments in the next 7 days it will automatically be closed. Maintainers can also remove the lifecycle/stale label.
If this pull request was automatically closed and you feel this pull request should be reopened, we encourage creating a new pull request linking back to this one for added context. Thank you!

@geota
Copy link

geota commented Oct 4, 2022

@jacobbednarz API now returnst his: https://api.cloudflare.com/#zero-trust-lists-zero-trust-list-items

  "result_info": {
    "page": 1,
    "per_page": 20,
    "count": 1,
    "total_count": 2000
  }

Can you confirm this is not sufficient and you need total_pages as well? E.g. total_count / per_page? That seems redundant, but we can add it.

@jacobbednarz
Copy link
Member

there is a little bit of nuance in the v4 envelope that can be confusing.

  • page is the current page of results
  • per_page is the number of results expected in the current response
  • count is the number of filtered results found (say if you provide param1=blah to the request)
  • total_count is the number of all results without filters
  • total_pages is to return the number of all results without filters

i don't agree with this pattern (including filtered + unfiltered options in the response, as really, we don't actually care) however, that is the intended use of those fields which we're lacking some of here. some teams internally don't follow this and that's fine but we're trying to make sure all the fields at least exist for a consistent payload.

we are also in the process of standardising pagination in cloudflare-go so in the near future, consumers can do something like paginateResults(...) and this is will fetch all the results - see https://github.com/cloudflare/cloudflare-go/blob/dc6c002de52f15566cbcedb5be9a5e758858dabb/pagination.go. this relies on the TotalPages field being present.

@jacobbednarz jacobbednarz removed the workflow/pending-cloudflare-response Indicates an issue or PR requires a response from the Cloudflare team. label Oct 20, 2022
@jacobbednarz
Copy link
Member

i've updated the internals of cloudflare-go to handle the pagination automatically via cloudflare/cloudflare-go#1114 so we no longer need it here. once that is released, this PR will be good to merge.

acceptance tests are all green

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareTeamsList_" -count 1 -parallel 1 -timeout 120m -parallel 1
?       github.com/cloudflare/terraform-provider-cloudflare     [no test files]
=== RUN   TestAccCloudflareTeamsList_Basic
--- PASS: TestAccCloudflareTeamsList_Basic (10.73s)
=== RUN   TestAccCloudflareTeamsList_LottaListItems
--- PASS: TestAccCloudflareTeamsList_LottaListItems (31.84s)
=== RUN   TestAccCloudflareTeamsList_Reordered
--- PASS: TestAccCloudflareTeamsList_Reordered (21.29s)
PASS
ok      github.com/cloudflare/terraform-provider-cloudflare/internal/provider   64.198s

@jacobbednarz jacobbednarz added the workflow/pending-upstream-library Indicates an issue or PR requires changes from an upstream library. label Oct 20, 2022
@jacobbednarz jacobbednarz merged commit 3709167 into cloudflare:master Oct 31, 2022
@github-actions github-actions bot added this to the v3.27.0 milestone Oct 31, 2022
github-actions bot pushed a commit that referenced this pull request Oct 31, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

This functionality has been released in v3.27.0 of the Terraform Cloudflare 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow/pending-upstream-library Indicates an issue or PR requires changes from an upstream library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudflare_teams_list with more than 50 items does not work
5 participants