Mark unlicensed users as disabled. #38
Conversation
WalkthroughThe pull request introduces several modifications across multiple files. A new method, Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
pkg/connector/user.go (1)
109-109: Externalize status message for maintainabilityThe status message
"No Bitbucket license"is hardcoded, which might make future updates or internationalization efforts more challenging. Consider defining this message as a constant or retrieving it from a configuration.Apply this diff to define a constant:
const ( // ... existing constants + noLicenseMessage = "No Bitbucket license" ) // ... opts = append(opts, rs.WithDetailedStatus(v2.UserTrait_Status_STATUS_DISABLED, noLicenseMessage))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- pkg/client/client.go (2 hunks)
- pkg/client/pagination.go (0 hunks)
- pkg/connector/group.go (1 hunks)
- pkg/connector/org.go (1 hunks)
- pkg/connector/project.go (1 hunks)
- pkg/connector/repo.go (1 hunks)
- pkg/connector/user.go (8 hunks)
💤 Files with no reviewable changes (1)
- pkg/client/pagination.go
🧰 Additional context used
🔇 Additional comments (5)
pkg/connector/org.go (1)
138-138: LGTM. Please verify the new parameter's purpose and add a comment.The addition of the
nilparameter to theuserResourcefunction call aligns with the PR objective of marking unlicensed users as disabled. However, to ensure clarity and maintainability:
- Please verify that the
nilvalue is correctly handled in theuserResourcefunction to represent a non-disabled user.- Consider adding a comment explaining the purpose of this new parameter, e.g.:
// nil represents that the user is not disabledTo ensure consistency across the codebase, please run the following command to check for any other occurrences of
userResourcecalls that might need updating:✅ Verification successful
Verified ✅
The addition of the
nilparameter in theuserResourcefunction call is consistent with the function's signature and Go best practices. Passingnilfor theoptsslice safely represents the absence of additional options.No further action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for userResource function calls rg 'userResource\s*\(' --type goLength of output: 571
Script:
#!/bin/bash # Extract the userResource function signatures ast-grep --lang go --pattern 'func userResource($_, $_, $_, $_) $_' .Length of output: 3001
pkg/connector/repo.go (1)
163-163: Clarify the purpose of the new parameter inuserResourceThe addition of a
nilargument to theuserResourcefunction call appears to be related to the PR objective of marking unlicensed users as disabled. However, the purpose and impact of this new parameter are not immediately clear from this change alone.Could you please provide more context on:
- What does this new parameter represent in the
userResourcefunction?- How does passing
nilhere contribute to marking unlicensed users as disabled?Additionally, consider if there's a more explicit way to handle this functionality. For example, if this parameter is meant to represent a license status, it might be clearer to use a boolean flag or a dedicated type instead of
nil.To better understand the impact of this change, let's examine the
userResourcefunction definition:This will help us verify the new parameter's purpose and ensure it's being used correctly.
✅ Verification successful
**The addition of the
nilargument in theuserResourcefunction call correctly matches the updated function signature and aligns with the PR objectives. **🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the userResource function definition ast-grep --lang go --pattern 'func userResource($_, $_, $_, $_) { $$$ }'Length of output: 76
Script:
#!/bin/bash # Search for all userResource function definitions regardless of the number of parameters ast-grep --lang go --pattern 'func userResource($${}) { $$$ }' # Alternatively, use ripgrep to find all occurrences of userResource function definitions rg 'func userResource' --context 2Length of output: 413
pkg/client/client.go (3)
231-231: LGTM: Method signature and placement are appropriate.The
GetGroupUsersmethod signature follows the conventions used in other methods of theDataCenterClientstruct. It correctly includes a context parameter and returns appropriate types ([]User, error).
234-260: Pagination logic is well-implemented.The method effectively handles pagination by:
- Using a
forloop to fetch all pages.- Updating the
startvariable withNextPageStartfrom the API response.- Breaking the loop when
IsLastPageis true.- Appending users from each page to the
usersslice.The use of the
ITEMSPERPAGEconstant for the page size is good for maintainability.
231-263: Overall, the new GetGroupUsers method is a valuable addition.The implementation is solid and follows the patterns established in other methods of the
DataCenterClientstruct. While there are opportunities for optimization and improved error handling, the current implementation should work well for most use cases.Key strengths:
- Correct pagination handling
- Proper use of context in the method signature
- Consistent error checking
Areas for potential improvement:
- More robust error handling
- Optimizations for very large datasets
- Respecting context cancellation within the loop
These suggestions can be addressed in future iterations to make the method even more robust and efficient.
| @@ -103,7 +103,7 @@ func (g *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken | |||
| ID: usrCppy.ID, | |||
| Slug: usrCppy.Slug, | |||
| Type: usrCppy.Type, | |||
| }, resource.Id) | |||
| }, resource.Id, nil) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of userResource function parameters
The userResource function is being called with varying numbers of parameters across different files, which may lead to inconsistencies and potential bugs.
Current parameter counts:
- 5 parameters:
pkg/connector/user.go
- 4 parameters:
pkg/connector/group.gopkg/connector/repo.go
- 3 parameters:
pkg/connector/project.gopkg/connector/org.go
Please ensure that all userResource function calls conform to the updated function signature to maintain consistency and prevent potential issues.
🔗 Analysis chain
Approve change, but clarification needed on new parameter
The addition of the nil parameter to the userResource function call appears to be part of a broader update to the function signature. This change is likely related to the PR objective of marking unlicensed users as disabled.
However, I have a few suggestions:
-
Could you please clarify the purpose of this new parameter? Understanding its role would help in reviewing the change more effectively.
-
Consider adding a comment to explain why
nilis used here. This would improve code readability and maintainability. -
It's important to ensure that this change is consistent across all calls to
userResourcein the codebase.
To verify the consistency of this change, please run the following script:
This script will help identify any inconsistencies in the userResource function calls across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in userResource function calls
# Search for all userResource function calls
echo "Searching for userResource function calls:"
rg --type go "userResource\(" -A 3
# Count the number of parameters in each userResource call
echo -e "\nCounting parameters in userResource calls:"
rg --type go "userResource\(" | sed -E 's/.*userResource\((.*)\).*/\1/' | awk -F',' '{print NF}'
Length of output: 1915
| @@ -158,7 +158,7 @@ func (p *projectBuilder) Grants(ctx context.Context, resource *v2.Resource, pTok | |||
| ID: usrCppy.ID, | |||
| Slug: usrCppy.Slug, | |||
| Type: usrCppy.Type, | |||
| }, resource.Id) | |||
| }, resource.Id, nil) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent userResource Function Calls Detected
Several userResource function calls across the codebase are missing the newly added nil parameter. This inconsistency may lead to unexpected behavior or runtime errors.
Affected Files:
pkg/connector/project.gopkg/connector/org.gopkg/connector/group.go
🔗 Analysis chain
Approve change, but verify consistency across codebase.
The addition of the nil parameter to the userResource function call aligns with the PR objective of handling unlicensed users. This change appears to be correct in this context.
However, to ensure consistency and prevent potential issues, it's important to verify that all other calls to userResource across the codebase have been updated similarly.
To verify the consistency of this change, we can run the following script:
Would you like me to run this verification script?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent calls to userResource function
# Search for userResource function calls
echo "Searching for userResource function calls:"
rg --type go 'userResource\s*\(' -A 3
# Note: Manually review the output to ensure all calls have been updated consistently
Length of output: 1708
pkg/client/client.go
Outdated
| func (d *DataCenterClient) GetGroupUsers(ctx context.Context, group string) ([]User, error) { | ||
| var users []User | ||
| var err error | ||
| start := 0 | ||
|
|
||
| for { | ||
| queryParams := map[string]string{ | ||
| "start": strconv.Itoa(start), | ||
| "limit": strconv.Itoa(ITEMSPERPAGE), | ||
| "group": group, | ||
| } | ||
|
|
||
| uri, err := d.MakeURL(ctx, allUsersEndpoint, queryParams) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var userData UsersAPIData | ||
| resp, err := d.Do(ctx, http.MethodGet, uri, nil, &userData) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
| users = append(users, userData.Users...) | ||
|
|
||
| start = userData.NextPageStart | ||
| if userData.IsLastPage { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return users, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizations for large datasets and context cancellation.
While the current implementation works well for most cases, consider the following improvements:
-
For very large groups, fetching all users at once might be memory-intensive. Consider implementing a way to stream results or fetch users in batches.
-
Add a check for context cancellation within the loop to allow for early termination of the operation.
-
Implement a rate limiting or backoff mechanism to avoid overwhelming the API with requests.
Here's a sketch of how you might implement these improvements:
func (d *DataCenterClient) GetGroupUsers(ctx context.Context, group string) ([]User, error) {
var users []User
start := 0
for {
select {
case <-ctx.Done():
return users, ctx.Err()
default:
// Existing code here...
// Add a small delay between requests
time.Sleep(100 * time.Millisecond)
}
}
}Consider also adding a parameter to allow callers to specify a maximum number of users to fetch, which can help manage memory usage for very large groups.
pkg/client/client.go
Outdated
| var users []User | ||
| var err error | ||
| start := 0 | ||
|
|
||
| for { | ||
| queryParams := map[string]string{ | ||
| "start": strconv.Itoa(start), | ||
| "limit": strconv.Itoa(ITEMSPERPAGE), | ||
| "group": group, | ||
| } | ||
|
|
||
| uri, err := d.MakeURL(ctx, allUsersEndpoint, queryParams) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var userData UsersAPIData | ||
| resp, err := d.Do(ctx, http.MethodGet, uri, nil, &userData) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
| users = append(users, userData.Users...) | ||
|
|
||
| start = userData.NextPageStart | ||
| if userData.IsLastPage { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return users, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling.
While the current error handling is generally good, there's room for improvement:
- The method returns the last error encountered in the loop, which might not be the most relevant if an earlier iteration failed.
- Consider using a separate error variable for the loop and returning it immediately if an error occurs.
Here's a suggested improvement:
func (d *DataCenterClient) GetGroupUsers(ctx context.Context, group string) ([]User, error) {
var users []User
- var err error
start := 0
for {
queryParams := map[string]string{
"start": strconv.Itoa(start),
"limit": strconv.Itoa(ITEMSPERPAGE),
"group": group,
}
uri, err := d.MakeURL(ctx, allUsersEndpoint, queryParams)
if err != nil {
return nil, err
}
var userData UsersAPIData
resp, err := d.Do(ctx, http.MethodGet, uri, nil, &userData)
if err != nil {
return nil, err
}
defer resp.Body.Close()
users = append(users, userData.Users...)
start = userData.NextPageStart
if userData.IsLastPage {
break
}
}
- return users, err
+ return users, nil
}This change ensures that any error encountered during the loop is immediately returned, providing more accurate error reporting.
📝 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.
| var users []User | |
| var err error | |
| start := 0 | |
| for { | |
| queryParams := map[string]string{ | |
| "start": strconv.Itoa(start), | |
| "limit": strconv.Itoa(ITEMSPERPAGE), | |
| "group": group, | |
| } | |
| uri, err := d.MakeURL(ctx, allUsersEndpoint, queryParams) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var userData UsersAPIData | |
| resp, err := d.Do(ctx, http.MethodGet, uri, nil, &userData) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer resp.Body.Close() | |
| users = append(users, userData.Users...) | |
| start = userData.NextPageStart | |
| if userData.IsLastPage { | |
| break | |
| } | |
| } | |
| return users, err | |
| } | |
| func (d *DataCenterClient) GetGroupUsers(ctx context.Context, group string) ([]User, error) { | |
| var users []User | |
| start := 0 | |
| for { | |
| queryParams := map[string]string{ | |
| "start": strconv.Itoa(start), | |
| "limit": strconv.Itoa(ITEMSPERPAGE), | |
| "group": group, | |
| } | |
| uri, err := d.MakeURL(ctx, allUsersEndpoint, queryParams) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var userData UsersAPIData | |
| resp, err := d.Do(ctx, http.MethodGet, uri, nil, &userData) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer resp.Body.Close() | |
| users = append(users, userData.Users...) | |
| start = userData.NextPageStart | |
| if userData.IsLastPage { | |
| break | |
| } | |
| } | |
| return users, nil | |
| } |
| for _, group := range u.userGroupFilter { | ||
| users, err := u.client.GetGroupUsers(ctx, group) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
| u.groupUsers = append(u.groupUsers, users...) | ||
| } |
There was a problem hiding this comment.
Handle potential duplicates when aggregating group users
When appending users from each group to u.groupUsers, there is a possibility of duplicate entries if a user belongs to multiple groups in userGroupFilter. This could lead to unnecessary processing and memory usage.
If you continue using a slice for groupUsers, consider deduplicating the users after aggregation.
| @@ -10,8 +11,7 @@ | |||
| rs "github.com/conductorone/baton-sdk/pkg/types/resource" | |||
| ) | |||
|
|
|||
| func userResource(_ context.Context, user *client.User, parentResourceID *v2.ResourceId) (*v2.Resource, error) { | |||
| var userStatus v2.UserTrait_Status_Status = v2.UserTrait_Status_STATUS_ENABLED | |||
| func userResource(_ context.Context, user *client.User, parentResourceID *v2.ResourceId, opts []rs.UserTraitOption) (*v2.Resource, error) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Update All userResource Calls to Include the New opts Parameter
Several instances of userResource are missing the required opts parameter or are passing nil. Please update these calls to include the new parameter to ensure consistency and prevent potential compile-time errors.
pkg/connector/repo.gopkg/connector/project.gopkg/connector/group.gopkg/connector/org.go
🔗 Analysis chain
Ensure all calls to userResource are updated to match the new signature
The userResource function signature has been modified to include an additional parameter opts []rs.UserTraitOption. Please verify that all invocations of userResource within the codebase have been updated to pass this new argument to prevent any potential compile-time errors.
You can run the following script to identify all usages of userResource and check if they have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to userResource and display the lines for verification.
rg --type go 'userResource\(' -A 2
Length of output: 1332
| if pToken == nil || pToken.Token == "" { | ||
| for _, group := range u.userGroupFilter { | ||
| users, err := u.client.GetGroupUsers(ctx, group) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
| u.groupUsers = append(u.groupUsers, users...) | ||
| } |
There was a problem hiding this comment.
Populate groupUsers consistently across pagination
Currently, u.groupUsers is populated only when pToken is nil or empty. In scenarios where pagination occurs (i.e., pToken is not nil), u.groupUsers may not be populated, which could lead to incorrect behavior when checking if a user belongs to a group.
Consider refactoring to ensure that u.groupUsers is populated regardless of the pagination state to maintain consistent behavior across all pages.
Apply this diff to adjust the logic:
if pToken == nil || pToken.Token == "" {
for _, group := range u.userGroupFilter {
users, err := u.client.GetGroupUsers(ctx, group)
if err != nil {
return nil, "", nil, err
}
u.groupUsers = append(u.groupUsers, users...)
}
+} else {
+ // Continue appending group users for subsequent pages
+ for _, group := range u.userGroupFilter {
+ users, err := u.client.GetGroupUsers(ctx, group)
+ if err != nil {
+ return nil, "", nil, err
+ }
+ u.groupUsers = append(u.groupUsers, users...)
+ }
}📝 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.
| if pToken == nil || pToken.Token == "" { | |
| for _, group := range u.userGroupFilter { | |
| users, err := u.client.GetGroupUsers(ctx, group) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| u.groupUsers = append(u.groupUsers, users...) | |
| } | |
| if pToken == nil || pToken.Token == "" { | |
| for _, group := range u.userGroupFilter { | |
| users, err := u.client.GetGroupUsers(ctx, group) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| u.groupUsers = append(u.groupUsers, users...) | |
| } | |
| } else { | |
| // Continue appending group users for subsequent pages | |
| for _, group := range u.userGroupFilter { | |
| users, err := u.client.GetGroupUsers(ctx, group) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| u.groupUsers = append(u.groupUsers, users...) | |
| } | |
| } |
pkg/connector/user.go
Outdated
| opts := []rs.UserTraitOption{} | ||
| if len(u.userGroupFilter) > 0 && !slices.ContainsFunc(u.groupUsers, func(u client.User) bool { | ||
| return u.ID == usr.ID | ||
| }) { | ||
| opts = append(opts, rs.WithDetailedStatus(v2.UserTrait_Status_STATUS_DISABLED, "No Bitbucket license")) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize user lookup by using a map for faster performance
Using slices.ContainsFunc to check if a user exists in u.groupUsers can lead to performance issues with large datasets due to linear search time. To enhance performance, consider using a map to store groupUsers, enabling constant-time lookups.
Apply this diff to implement the optimization:
type userBuilder struct {
resourceType *v2.ResourceType
client *client.DataCenterClient
userGroupFilter []string
- groupUsers []client.User
+ groupUserIDs map[string]struct{}
}
func (u *userBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, pToken *pagination.Token) ([]*v2.Resource, string, annotations.Annotations, error) {
var rv []*v2.Resource
if pToken == nil || pToken.Token == "" {
+ u.groupUserIDs = make(map[string]struct{})
for _, group := range u.userGroupFilter {
users, err := u.client.GetGroupUsers(ctx, group)
if err != nil {
return nil, "", nil, err
}
- u.groupUsers = append(u.groupUsers, users...)
+ for _, user := range users {
+ u.groupUserIDs[user.ID] = struct{}{}
+ }
}
}
users, nextPageToken, err := u.client.GetUsers(ctx, pToken)
if err != nil {
return nil, "", nil, err
}
for _, usr := range users {
usrCopy := usr
opts := []rs.UserTraitOption{}
- if len(u.userGroupFilter) > 0 && !slices.ContainsFunc(u.groupUsers, func(u client.User) bool {
- return u.ID == usr.ID
- }) {
+ if len(u.userGroupFilter) > 0 {
+ if _, exists := u.groupUserIDs[usr.ID]; !exists {
opts = append(opts, rs.WithDetailedStatus(v2.UserTrait_Status_STATUS_DISABLED, "No Bitbucket license"))
+ }
}
ur, err := userResource(ctx, &usrCopy, parentResourceID, opts)
if err != nil {
return nil, "", nil, err
}
rv = append(rv, ur)
}This change replaces the slice with a map, significantly improving the performance of user existence checks.
Committable suggestion was skipped due to low confidence.
pkg/client/client.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
this defer isn't safe inside the for loop?
There was a problem hiding this comment.
Whoops. I close the response body every time now.
I'm guessing I didn't run into any bugs testing because baton-SDK's http wrapper replaces the response body with a noop reader.
Previously we didn't sync unlicensed users. Now we mark them as disabled.
ggreer
force-pushed
the
ggreer/disable-unlicensed-users
branch
from
8c5993f to
84e2d23
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/connector/user.go (2)
73-76: LGTM: Updates touserBuilderstructThe renaming of
userGroupstouserGroupFilterand the addition ofgroupUsersfield improve the clarity and functionality of the struct.Consider using a map instead of a slice for
groupUsersto improve lookup performance, especially if the number of users can be large. For example:groupUsers map[string]client.UserThis would allow for constant-time lookups instead of linear-time searches.
Line range hint
88-112: Improve consistency and performance inListmethodWhile the changes add the desired functionality, there are a few areas for improvement:
Consistency across pagination:
ThegroupUsersis only populated whenpTokenis nil or empty. This may lead to inconsistent behavior when paginating through results.Performance of user lookup:
The current implementation uses a linear search withslices.ContainsFunc, which can be inefficient for large datasets.To address these issues:
Consider populating
groupUsersregardless of thepTokenvalue to ensure consistency across all pages.As suggested in the TODO comment, use a map instead of a slice for
groupUsersto improve lookup performance. This change would also address the concern raised in theuserBuilderstruct review.Here's a proposed refactor:
type userBuilder struct { // ... other fields ... groupUsers map[string]struct{} } func (u *userBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, pToken *pagination.Token) ([]*v2.Resource, string, annotations.Annotations, error) { var rv []*v2.Resource if u.groupUsers == nil { u.groupUsers = make(map[string]struct{}) for _, group := range u.userGroupFilter { users, err := u.client.GetGroupUsers(ctx, group) if err != nil { return nil, "", nil, err } for _, user := range users { u.groupUsers[user.ID] = struct{}{} } } } users, nextPageToken, err := u.client.GetUsers(ctx, pToken) if err != nil { return nil, "", nil, err } for _, usr := range users { usrCopy := usr opts := []rs.UserTraitOption{} if len(u.userGroupFilter) > 0 { if _, exists := u.groupUsers[usr.ID]; !exists { opts = append(opts, rs.WithDetailedStatus(v2.UserTrait_Status_STATUS_DISABLED, "No Bitbucket license")) } } ur, err := userResource(ctx, &usrCopy, parentResourceID, opts) if err != nil { return nil, "", nil, err } rv = append(rv, ur) } return rv, nextPageToken, nil, nil }This refactoring addresses both the consistency and performance issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- pkg/client/client.go (2 hunks)
- pkg/client/pagination.go (0 hunks)
- pkg/connector/group.go (1 hunks)
- pkg/connector/org.go (1 hunks)
- pkg/connector/project.go (1 hunks)
- pkg/connector/repo.go (1 hunks)
- pkg/connector/user.go (8 hunks)
💤 Files with no reviewable changes (1)
- pkg/client/pagination.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/client/client.go
- pkg/connector/group.go
- pkg/connector/org.go
- pkg/connector/project.go
- pkg/connector/repo.go
🧰 Additional context used
🔇 Additional comments (4)
pkg/connector/user.go (4)
5-5: LGTM: Import of "slices" packageThe addition of the "slices" package import is appropriate for the new functionality in the
Listmethod.
14-14: LGTM: UpdateduserResourcefunction signatureThe addition of the
opts []rs.UserTraitOptionparameter enhances the flexibility of user trait configuration.As mentioned in a previous review, please ensure that all calls to
userResourceacross the codebase have been updated to include this new parameter.
50-51: LGTM: Incorporation ofoptsintouserTraitsThe addition of
optstouserTraitscorrectly implements the new functionality provided by the updated function signature.
136-138: LGTM: Update tonewUserBuilderfunctionThe renaming of the
userGroupsparameter touserGroupFilteris consistent with the changes made to theuserBuilderstruct and accurately reflects its new purpose.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/client/client.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
pkg/client/client.go (1)
231-266: LGTM: The newGetGroupUsersmethod is correctly implementedThe implementation of the
GetGroupUsersmethod appears correct and follows the established patterns for handling paginated API requests. The method appropriately handles pagination, error checking, and data aggregation.
| for { | ||
| queryParams := map[string]string{ | ||
| "start": strconv.Itoa(start), | ||
| "limit": strconv.Itoa(ITEMSPERPAGE), |
There was a problem hiding this comment.
Undefined Constant ITEMSPERPAGE
The constant ITEMSPERPAGE used in the query parameters for pagination is undefined in this file. Please ensure that ITEMSPERPAGE is defined or imported appropriately before using it.
Previously we didn't sync unlicensed users. Now we mark them as disabled.
Summary by CodeRabbit
New Features
Enhancements
Functionality Updates
Pagination Improvements