-
Notifications
You must be signed in to change notification settings - Fork 205
feat: Add new resource mongodbatlas_cloud_user_project_assignment #3568
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
base: CLOUDP-320243-dev-2.0.0
Are you sure you want to change the base?
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
APIx bot: a message has been sent to Docs Slack channel |
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
This PR adds a new Terraform resource mongodbatlas_cloud_user_project_assignment
for managing user assignments to MongoDB Atlas projects with specific roles.
Key changes:
- Implements a new resource with full CRUD operations and import functionality
- Adds comprehensive test coverage including basic functionality and migration scenarios
- Integrates the resource into the provider registration and CI/CD workflows
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
internal/service/clouduserprojectassignment/resource.go | Core resource implementation with CRUD operations and import logic |
internal/service/clouduserprojectassignment/resource_schema.go | Generated schema definition and TFModel struct |
internal/service/clouduserprojectassignment/model.go | Data transformation functions between Terraform and Atlas SDK models |
internal/service/clouduserprojectassignment/resource_test.go | Basic acceptance tests with create, update, and import scenarios |
internal/service/clouduserprojectassignment/resource_migration_test.go | Migration tests for transitioning from project invitation to user assignment |
internal/service/clouduserprojectassignment/model_test.go | Unit tests for model transformation functions |
internal/service/clouduserprojectassignment/main_test.go | Test setup and teardown |
internal/service/clouduserprojectassignment/tfplugingen/generator_config.yml | Configuration for code generation |
internal/provider/provider.go | Registration of the new resource in the provider |
.github/workflows/acceptance-tests-runner.yml | CI configuration updates for the new service |
.changelog/3568.txt | Release note for the new resource |
resp.Diagnostics.Append(resp.State.Set(ctx, newCloudUserProjectAssignmentModel)...) | ||
} | ||
|
||
func fetchProjectUser(ctx context.Context, connV2 *admin.APIClient, projectID, userID, username string) (*admin.GroupUserResponse, 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.
fetchProjectUser
function is introduced here and reused in #3569 to extract common functionality between the resource and the data source.
return addRequests, removeRequests, nil | ||
} | ||
|
||
func diffRoles(oldRoles, newRoles []string) (toAdd, toRemove []string) { |
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.
not urgent, can be done in a follow-up ticket - but this method should have its unit test, or you can have a test for NewAtlasUpdateReq
that checks it always generates the right request based on diff
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.
Added a test for NewAtlasUpdateReq
} | ||
|
||
for _, removeReq := range removeRequests { | ||
_, _, err := connV2.MongoDBCloudUsersApi.RemoveProjectRole(ctx, projectID, userID, removeReq).Execute() |
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.
not blocking - if this API fails, the update will be partially applied (only the addprojectRole). This should be documented at minimum, but also we should check if we're consistent in similar situations
cc @maastha
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.
by documented, do you mean add a comment here or mention something in the public documentation?
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 think we should publicly document it? because I can see a question coming, like: "the update failed but then I logged in and saw some users were added ..."
|
||
resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("project_id"), projectID)...) | ||
|
||
emailRegex := regexp.MustCompile(`^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$`) |
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.
not blocking - we don't have any util method that does that already?
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.
There's a method for this but in the acceptance tests package. Since this same regex is duplicated across 2 of the other new resources, we could consider extracting it into a shared utility function in a follow-up ticket, WDYT?
plan *clouduserprojectassignment.TFModel | ||
expected *admin.GroupUserRequest | ||
}{ | ||
"Complete model": { |
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.
also add test cases for empty/nil values (for all unit tests)
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.
Done 59950ba
return &addProjectUserReq, nil | ||
} | ||
|
||
func NewAtlasUpdateReq(ctx context.Context, plan, state *TFModel) (addRequests, removeRequests []*admin.AddOrRemoveGroupRole, diags diag.Diagnostics) { |
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'm thinking if instead of getting roles from the state here, would it be better to compare against what is returned by the API instead? That ways any partially applied calls can be handled correctly WDYT?
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.
Do you mean making another API call before calling this method in the Update?
resource.ParallelTest(t, *basicTestCase(t)) | ||
} | ||
|
||
func basicTestCase(t *testing.T) *resource.TestCase { |
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 also have a test case to import an existing (ACTIVE) project user? is that possible?
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.
@csanx are there any docs updates for this feature? If not, LGTM
I see https://github.com/mongodb/terraform-provider-mongodbatlas/pull/3578/files so I'll review that and approve this |
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, leaving some doubts which seem more API related
Validators: []validator.String{ | ||
stringvalidator.RegexMatches(regexp.MustCompile("^([a-f0-9]{24})$"), ""), |
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.
Would remove the regex validation just for consistency with other resources
importID := req.ID | ||
ok, parts := conversion.ImportSplit(req.ID, 2) | ||
if !ok { | ||
resp.Diagnostics.AddError("invalid import ID format", "expected 'project_id/user_id' or 'project_id/username', got: "+importID) |
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: Why do we support import with user_id
as well? Does it help with a particular migration journey? Asking as this does add some complexity to our read operation.
}) | ||
} | ||
|
||
func originalConfigFirst(username, projectName, orgID string, roles []string) string { |
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.
func originalConfigFirst(username, projectName, orgID string, roles []string) string { | |
func legacyProjectInvitationConfig(username, projectName, orgID string, roles []string) string { |
Would make more explicit this is using the deprecated resource
resource "mongodbatlas_project_invitation" "mig_test" { | ||
project_id = mongodbatlas_project.mig_test.id | ||
username = local.username | ||
roles = local.roles | ||
} | ||
resource "mongodbatlas_cloud_user_project_assignment" "user_mig_test" { | ||
project_id = mongodbatlas_project.mig_test.id | ||
username = local.username | ||
roles = local.roles | ||
} |
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 user_project_assignment
be created with project_invitation
already existing? I suppose this is how the API behaves but wanted to confirm.
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.
Yes, after running this, the state shows:
{
"mode": "managed",
"type": "mongodbatlas_project_invitation",
"name": "mig_test",
...
},
{
"mode": "managed",
"type": "mongodbatlas_cloud_user_project_assignment",
"name": "user_mig_test",
...
},
We can also see in Cloud Dev that the same user is invited twice.
func removeProjectInvitationConfigThird(username, projectName, orgID string, roles []string) string { | ||
rolesStr := `"` + strings.Join(roles, `", "`) + `"` | ||
return fmt.Sprintf(` | ||
locals { | ||
username = %[1]q | ||
roles = [%[2]s] | ||
} | ||
resource "mongodbatlas_project" "mig_test" { | ||
name = %[3]q | ||
org_id = %[4]q | ||
} | ||
resource "mongodbatlas_cloud_user_project_assignment" "user_mig_test" { | ||
project_id = mongodbatlas_project.mig_test.id | ||
username = local.username | ||
roles = local.roles | ||
} |
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.
When project_invitation
is deleted it has not impact on cloud_user_project_assignment
?
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.
No, they act as two independent invitations. Deleting one just removes that specific invitation.
@jwilliams-mongo Yes, my upcoming PRs will add documentation for all new resources and data sources. The one you mention is related to another resource actually |
Description
Add new resource:
mongodbatlas_cloud_user_project_assignment
Link to any related issue(s): CLOUDP-333176
Type of change:
Required Checklist:
Further comments