Auto pagination org users #26
Merged
Conversation
This file contains hidden or 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
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
pabloinigo
approved these changes
Sep 1, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
163-166: Be explicit about how to run tfplugindocs.Suggest adding the exact command, leveraging tools.go.
Add after the line:
Make sure to run `tfplugindocs` if resources are updated. +For a local run: +`go run -tags tools github.com/hashicorp/terraform-plugin-docs/cmd/tfplugindocs generate`internal/provider/data_source_openai_organization_users.go (2)
127-130: Consider making batch size tunable.A fixed batchSize=100 works, but a schema or env override (with sane max) can help testing and large orgs.
168-170: Use a stable content-derived ID instead of count-only.Count-only IDs collide when filters/content change but the total stays constant.
Apply:
- // Set a unique ID for the data source - d.SetId(fmt.Sprintf("organization-users-all-%d", len(allUsers))) + // Set a stable ID derived from inputs and results + id, err := computeUsersDataSourceID(allUsers, emails) + if err != nil { + return diag.FromErr(fmt.Errorf("error computing data source ID: %w", err)) + } + d.SetId(id)Add this helper (and imports) elsewhere in the file:
// imports to add: import ( "crypto/sha256" "encoding/hex" "sort" "strings" ) // computeUsersDataSourceID builds a stable ID from filters and returned user IDs. func computeUsersDataSourceID(users []map[string]interface{}, emails []string) (string, error) { ids := make([]string, 0, len(users)) for _, u := range users { if id, ok := u["id"].(string); ok { ids = append(ids, id) } } sort.Strings(ids) sort.Strings(emails) var b strings.Builder b.WriteString("count=") b.WriteString(fmt.Sprint(len(users))) b.WriteString(";emails=") b.WriteString(strings.Join(emails, ",")) b.WriteString(";ids=") b.WriteString(strings.Join(ids, ",")) sum := sha256.Sum256([]byte(b.String())) // Shorten for readability return "organization-users-" + hex.EncodeToString(sum[:8]), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
README.md(2 hunks)docs/data-sources/organization_users.md(0 hunks)internal/provider/data_source_openai_organization_users.go(1 hunks)internal/provider/resource_openai_model_response.go(1 hunks)internal/provider/resource_openai_organization_user.go(0 hunks)tools/tools.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/provider/resource_openai_organization_user.go
- docs/data-sources/organization_users.md
🔇 Additional comments (3)
internal/provider/resource_openai_model_response.go (1)
256-260: No-op whitespace change — LGTM.No functional impact.
tools/tools.go (1)
1-8: Pinning tfplugindocs via tools build tag — LGTM.Standard pattern; won’t affect normal builds.
README.md (1)
84-85: install-dev target defined in Makefile
Comment on lines
+159
to
164
| // Check if there are more pages | ||
| hasMore = resp.HasMore | ||
| if hasMore && resp.LastID != "" { | ||
| after = resp.LastID | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against infinite pagination if the API doesn’t advance.
If HasMore=true but LastID is empty or unchanged, this can loop forever. Fail fast.
Apply:
- // Check if there are more pages
- hasMore = resp.HasMore
- if hasMore && resp.LastID != "" {
- after = resp.LastID
- }
+ // Check if there are more pages and ensure progress
+ prevAfter := after
+ hasMore = resp.HasMore
+ if hasMore {
+ if resp.LastID == "" || resp.LastID == prevAfter {
+ return diag.Errorf("pagination did not advance at page %d (last_id=%q)", pageCount, resp.LastID)
+ }
+ after = resp.LastID
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Check if there are more pages | |
| hasMore = resp.HasMore | |
| if hasMore && resp.LastID != "" { | |
| after = resp.LastID | |
| } | |
| } | |
| // Check if there are more pages and ensure progress | |
| prevAfter := after | |
| hasMore = resp.HasMore | |
| if hasMore { | |
| if resp.LastID == "" || resp.LastID == prevAfter { | |
| return diag.Errorf("pagination did not advance at page %d (last_id=%q)", pageCount, resp.LastID) | |
| } | |
| after = resp.LastID | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/provider/data_source_openai_organization_users.go around lines
159-164, the pagination loop sets hasMore = resp.HasMore and updates after =
resp.LastID without guarding for an unchanged or empty LastID, which can cause
an infinite loop; modify the loop to detect when resp.HasMore is true but
resp.LastID is empty or equal to the previous after value (track previousLastID
before update) and then fail fast or break with a clear error (or set
hasMore=false) to prevent infinite pagination; ensure you return or propagate an
informative error so callers know pagination did not advance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Previous implementation followed API a bit too closely, to the point of being pointless a bit. Now the provider will go through all users till no more are found.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Summary by CodeRabbit
Refactor
Documentation
Chores