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

Allow user lookup by group membership #998

Merged
merged 6 commits into from
May 6, 2022
Merged

Conversation

BrentSouza
Copy link
Contributor

@BrentSouza BrentSouza commented Mar 3, 2022

The goal of this PR is to provide an api convenient way to get a list of users by group membership. Using the provider currently, this is a multi-step process.

data "okta_group" "foo" {
  name              = "foo"
  # this just gets you user ids
  include_users = true
}

# now you have to lookup each individual user
data "okta_user" "foo_user" {
  for_each = data.okta_group.foo.users
  user_id = each.key
}

Which has to hit the api multiple times for each user to get a fully populated user object. It would be preferable to hit the v1/groups/${groupId}/users endpoint to get a list of users in one shot, but there doesn't seem to be a way to do this. To solve this problem, this PR adds a new property group_id to the okta_users data block. You would use it as follows:

data "okta_users" "foo_users" {
  group_id = var.foo_group_id
}

Nice...except we lose the group_memberships that get populated when we use data.okta_user. To rectify, I've included an additional property called include_groups which functions like the include_users property of the data.okta_group resource.

data "okta_users" "foo_users" {
  group_id           = var.foo_group_id
  # now we have each user's group memberships
  include_groups = true
}

Unfortunately, that still requires us to hit the api for each user, but it still results in less api calls than the current state.

I've included some additional tests for these resource changes, but please let me know if you require any further changes to accept this PR.

@monde monde self-requested a review March 7, 2022 19:06
@monde
Copy link
Collaborator

monde commented Mar 7, 2022

Sorry for the delay @BrentSouza I will look into this.

@monde
Copy link
Collaborator

monde commented Mar 7, 2022

Also @BrentSouza, thanks all the code, examples, and tests!

@BrentSouza
Copy link
Contributor Author

@monde not a problem. Happy to contribute. Please let me know if you need any changes. My team here will be planning to use this enhancement soon :-)

@BrentSouza
Copy link
Contributor Author

Hi @monde! Any feedback on this one?

@BrentSouza
Copy link
Contributor Author

Hello there. Not trying to pester, just occasionally poking to see if there's anything else needed to get this pushed through. I do have a need for this functionality, but I can use a fork in the short-term.

@monde monde self-assigned this Mar 22, 2022
@monde
Copy link
Collaborator

monde commented Mar 22, 2022

Hi @BrentSouza thanks for the ping, I'll put some time into this, probably tomorrow.

@BrentSouza
Copy link
Contributor Author

@monde You are a wonderful person and I am sure you have more important things to do. However, please let me grab your attention for a moment to remind you of the wonderful code (I might be biased) in this PR. I hope my reminders are not getting annoying. If there is anything I can do to facilitate, please let me know.

@monde
Copy link
Collaborator

monde commented Apr 6, 2022

@BrentSouza I'll try and look through this PR and get it into a Friday release.

@BrentSouza
Copy link
Contributor Author

Hello friends! Just checking in again to make sure the fire of progress is still burning 🔥

@monde
Copy link
Collaborator

monde commented Apr 22, 2022

Hi @BrentSouza thanks for the nudge and I apologize for letting this slip through the cracks. I will make it a priority next week.

Copy link
Collaborator

@monde monde left a comment

Choose a reason for hiding this comment

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

I ran all the ACCs and they pass for me. Thank you @BrentSouza , code looks good to me.

TF_ACC=1 go test -tags unit -mod=readonly -test.v -run ^TestAccOktaDataSourceUsers_readWithGroupId$ ./okta 2>&1
=== RUN   TestAccOktaDataSourceUsers_readWithGroupId
--- PASS: TestAccOktaDataSourceUsers_readWithGroupId (12.33s)
PASS
ok      github.com/okta/terraform-provider-okta/okta    13.073s

TF_ACC=1 go test -tags unit -mod=readonly -test.v -run ^TestAccOktaDataSourceUsers_readWithGroupIdIncludingGroups$ ./okta 2>&1
=== RUN   TestAccOktaDataSourceUsers_readWithGroupIdIncludingGroups
--- PASS: TestAccOktaDataSourceUsers_readWithGroupIdIncludingGroups (12.30s)
PASS
ok      github.com/okta/terraform-provider-okta/okta    12.632s

TF_ACC=1 go test -tags unit -mod=readonly -test.v -run ^TestAccOktaDataSourceUsers_read$ ./okta 2>&1
=== RUN   TestAccOktaDataSourceUsers_read
--- PASS: TestAccOktaDataSourceUsers_read (13.11s)
PASS
ok      github.com/okta/terraform-provider-okta/okta    13.433s

@monde monde merged commit 41aba64 into okta:master May 6, 2022
@monde monde mentioned this pull request May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants