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

[datadog_user_role] Creating a new datadog_user_role resource #2311

Merged
merged 24 commits into from
Apr 16, 2024

Conversation

wangwillson1
Copy link
Contributor

@wangwillson1 wangwillson1 commented Mar 11, 2024

As part of ACCESS-2220, we want to create a datadog_user_role resource for user-role assignments via Terraform. These assignments should not overwrite existing ones that aren't managed by Terraform.

@wangwillson1 wangwillson1 force-pushed the willsonwang/access-2220-terraform-resource branch from 330a02b to b320876 Compare March 13, 2024 14:29
@wangwillson1 wangwillson1 force-pushed the willsonwang/access-2220-terraform-resource branch from b320876 to 9b0504a Compare March 13, 2024 16:18
@wangwillson1 wangwillson1 marked this pull request as ready for review March 13, 2024 20:46
@wangwillson1 wangwillson1 requested review from a team as code owners March 13, 2024 20:46
@wangwillson1 wangwillson1 requested a review from a team March 13, 2024 20:46
drichards-87
drichards-87 previously approved these changes Mar 13, 2024
@wangwillson1 wangwillson1 changed the title [ACCESS-2220] datadog_user_role resource [datadog_user_role][ACCESS-2220] datadog_user_role resource Mar 13, 2024
@wangwillson1 wangwillson1 changed the title [datadog_user_role][ACCESS-2220] datadog_user_role resource [datadog_user_role] Creating a new datadog_user_role resource Mar 13, 2024
@wangwillson1 wangwillson1 requested a review from a team as a code owner March 20, 2024 21:26
@wangwillson1 wangwillson1 force-pushed the willsonwang/access-2220-terraform-resource branch from 1c580a2 to 570bb9a Compare March 20, 2024 21:44
@wangwillson1 wangwillson1 marked this pull request as draft April 2, 2024 14:29
Copy link
Member

@srosenthal-dd srosenthal-dd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it worked well!

I am trying to see how this and #2340 interact, doing some more local testing, will comment on the other PR.

@wangwillson1
Copy link
Contributor Author

wangwillson1 commented Apr 2, 2024

I am trying to see how this and #2340 interact, doing some more local testing, will comment on the other PR.

i wonder if i made those changes obsolete... assuming we don't use datadog_user and datadog_user_role together, it seems that the Computed: true attribute make it so that when the roles attribute is omitted, no changes take effect now (which is what we want!)

fwiw this was my test

data "datadog_role" "foo" {
  filter = "Datadog Standard Role"
}

resource "datadog_user" "a" {
	email = "[email protected]"
        roles = [data.datadog_role.foo.id]
}

resource "datadog_user" "b" {
	email = "[email protected]"
        roles = []
}

resource "datadog_user" "c" {
	email = "[email protected]"
}

resource "datadog_user_role" "c" {
	role_id = data.datadog_role.foo.id
	user_id = datadog_user.c.id
}

i could freely assign roles to user c (and it would get supplemented with the standard role), but the others had the datadog_user roles as the source of truth

image

EDIT: just saw your comment on importing

@wangwillson1 wangwillson1 requested a review from skarimo April 2, 2024 14:39
@srosenthal-dd
Copy link
Member

I retested with your latest code, this time not including #2340.

I noted that:

  1. ✅ Importing an existing datadog_user works and doesn't change the roles
  2. ✅ The new datadog_user_role worked well for creations/deletions/imports
  3. ✅ Changing datadog_user to have unset roles always leaves it alone, but setting to empty list removes all roles
  4. ⚠️ One edge case I found, not caused by this PR: if you entirely replace the list of roles for a datadog_user, it'll first remove, then add, leaving a brief period where the user has no roles. If the user is the one whose key you are using for Terraform, you get locked out. Proposed fix for that is [datadog_user] Re-order role updates #2346.
  5. ℹ️ I probably missed some discussion, but I'm curious about the decision to make datadog_role_users data source instead of a more direct data datadog_user_role. I tried using it and it worked OK but I wasn't sure what users would do with it.

srosenthal-dd
srosenthal-dd previously approved these changes Apr 2, 2024
@wangwillson1 wangwillson1 marked this pull request as ready for review April 3, 2024 18:28
pablito-perez
pablito-perez previously approved these changes Apr 15, 2024
…ce, update tests to avoid Datadog Standard Role, as that has really long lookup and will error if any deletes flake
@nkzou nkzou dismissed stale reviews from pablito-perez and srosenthal-dd via c3c62dd April 15, 2024 18:16
@DataDog DataDog deleted a comment from dd-devflow bot Apr 16, 2024
@DataDog DataDog deleted a comment from dd-devflow bot Apr 16, 2024
@DataDog DataDog deleted a comment from dd-devflow bot Apr 16, 2024
@DataDog DataDog deleted a comment from dd-devflow bot Apr 16, 2024
@DataDog DataDog deleted a comment from dd-devflow bot Apr 16, 2024
@DataDog DataDog deleted a comment from dd-devflow bot Apr 16, 2024
@nkzou nkzou merged commit 73881df into master Apr 16, 2024
22 of 24 checks passed
@nkzou nkzou deleted the willsonwang/access-2220-terraform-resource branch April 16, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants