-
Notifications
You must be signed in to change notification settings - Fork 205
chore: Update mongodbatlas_cloud_user_team_assignment
resource to use new query param user_id
#3574
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
Conversation
This reverts commit f8d2d0a.
…m-provider-mongodbatlas into CLOUDP-320243-dev-2.0.0
…m-provider-mongodbatlas into CLOUDP-320243-dev-2.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Updates the mongodbatlas_cloud_user_team_assignment
resource to utilize the new user_id
query parameter for filtering in the MongoDB Cloud API endpoint. This change improves the efficiency of user lookups when fetching team assignments.
- Refactored the Read operation to use query parameters instead of fetching all users and filtering client-side
- Updated test utilities to use the new query parameter approach
- Removed a trivial test case that checked for missing required attributes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
internal/service/clouduserteamassignment/resource.go | Extracted common fetch logic into fetchTeamUser function using query parameters |
internal/service/clouduserteamassignment/data_source.go | Updated to use the new fetchTeamUser function instead of custom filtering logic |
internal/service/clouduserteamassignment/resource_test.go | Updated checkDestroy to use query parameters and removed trivial test case |
} | ||
|
||
userResp = &(userListResp.GetResults())[0] | ||
userResp, found, err := fetchTeamUser(ctx, connV2, orgID, teamID, &userID, &username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check for nulls here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous instruction uses ValueString()
which returns a known string value. If String is null or unknown, it return an empty string "". Therefore, these values won’t be null.
userListUserID, _, err := conn.MongoDBCloudUsersApi.ListTeamUsersWithParams(ctx, userIDParams).Execute() | ||
if userListUserID.HasResults() { | ||
return fmt.Errorf("cloud user team assignment for user (%s) in team (%s) still exists %s", userID, teamID, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this and the code below be written only once like you did in fetchTeamUser
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, the username
part is not necessary since user_id
is required and always available. I’m removing it.
OrgId: orgID, | ||
TeamId: teamID, | ||
} | ||
} else if username != nil && *username != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have missed this, but just curious: can you explain why we try to do also the call with username
and we don't just do it only with userID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because, during the discussion of the posible migration paths, we decided to continue supporting both import keys: org_id/team_id/user_id
and org_id/team_id/username
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -73,73 +71,61 @@ func (r *rs) Create(ctx context.Context, req resource.CreateRequest, resp *resou | |||
resp.Diagnostics.Append(resp.State.Set(ctx, newUserTeamAssignmentModel)...) | |||
} | |||
|
|||
func fetchTeamUser(ctx context.Context, connV2 *admin.APIClient, orgID, teamID string, userID, username *string) (*admin.OrgUserResponse, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: any reason for having the bool found
result? Can see it is redundant as same information is available with *admin.OrgUserResponse
either being defined or nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I refactored that function 5bc1e3d
…se new query param `user_id` (#3574) * Updated SDK * Revert "Updated SDK" This reverts commit f8d2d0a. * Added userId as param to filter * Changed to sequential test * Fix * Refactored Read method in resource and datasource * Fixed CheckDestroy * Removed redundant `found`parameter * Fixed checkDestroy --------- Co-authored-by: Cristina Sánchez Sánchez <cristina.sanchez@mongodb.com>
Description
Read
operation inmongodbatlas_cloud_user_team_assignment
resource to use new queryParamuser_id
to filter results in the "Return All MongoDB Cloud Users Assigned to One Team" endpoint.Additionally,
checkDestroy
inresource_test
to useuser_id
filter param.TestAccCloudUserTeamAssignmentDS_error
which checked if a required attribute was missing.Link to any related issue(s): CLOUDP-334100
Type of change:
Required Checklist:
Further comments
Fixed issue with test
TestAccCloudUserTeamAssignment_basic
. The import check was failing when running this test in parallel due to a diff in the number of teams assigned to the user between step 1 and 2. This issue only occurred in parallel execution with other tests and was not reproducible locally. Changed the test to run sequentially.This replaces PR #3572 , which targeted the wrong branch.
All feedback from that PR has been addressed here.