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

Add new resource github_team_members to allow authoritative team management #975

Merged
merged 7 commits into from
Feb 3, 2022

Conversation

stawik-mesa
Copy link
Contributor

@stawik-mesa stawik-mesa commented Nov 10, 2021

Relates to #395

Add new resource to allow authoritative team memberships.

Usage:

resource "github_team_members" "test_team_memberships" {
  team_id  = github_team.test_team.id
  members {
    username = "octocat"
    role     = "member"
  }

  members {
    username = "octocat2"
    role     = "maintainer"
  }
}

@stawik-mesa stawik-mesa changed the title Add new resource github_team_memberships to allow authorative team management Add new resource github_team_memberships to allow authoritative team management Nov 10, 2021
@majormoses
Copy link
Contributor

I like the idea, I am not sure if the name should be so similar that someone might accidentally typo the wrong one. Maybe something more explicit would be better as this deviates from the 1:1 relationship we have with the api to resource now.

Some suggestions, feel free to mix/match as I could not say I found a name I loved:

  • github_team_membership_removes_undefined
  • github_team_members_remove_unmanaged
  • github_group_only_these_members
  • github_group_remove_unmanaged_users
  • github_group_terraform_managed_only

@stawik-mesa
Copy link
Contributor Author

Do you really think it's necessary to such a long name?

I would prefer github_team_members (differs from github_team_membership).

I also added a detailed description in the resource documentation.

@jcudit jcudit added this to the v4.19.0 milestone Nov 22, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

I think this resource would make the overall use case much easier to configure, but dilutes the project slightly with yet another way to accomplish the task.

As for naming, I wish github_team_membership was already available or github_team could have taken the members block. I'm open to not block on the naming if we can get clearer documentation around when to use this resource versus the others.

Finally, the tests need to be reworked slightly to match the existing pattern. See resource_github_team_test for a relevant example, particularly the anonymous / individual / organization casing.

website/docs/r/team_members.html.markdown Outdated Show resolved Hide resolved
website/docs/r/team_members.html.markdown Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Udit <jcudit@github.com>
@stawik-mesa stawik-mesa requested a review from jcudit December 1, 2021 17:05
@stawik-mesa
Copy link
Contributor Author

@jcudit Thanks for the comments. I updates the documentation and reworked the test case.

@stawik-mesa stawik-mesa changed the title Add new resource github_team_memberships to allow authoritative team management Add new resource github_team_members to allow authoritative team management Dec 6, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Gave this a spin locally but found it difficult to test given the whole invite-another-collaborator flow. Looks good on another read through though and feel we can ship and iterate if needed.

🙇🏾 for the docs updates, hoping they reduce confusion over time.

github/resource_github_team_members_test.go Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Udit <jcudit@github.com>
@stawik-mesa
Copy link
Contributor Author

@jcudit Do you already have a timeline for the merge? Thanks for the reviews and approval ;)

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

@jcudit and I have read through this together and we'll get it released shortly!

@kfcampbell kfcampbell merged commit 0586394 into integrations:main Feb 3, 2022
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
…gement (integrations#975)

* Add new resource github_team_memberships to allow authorative team management

* Fix typo in test

* Add docs and rename resource to github_team_members

* Apply suggestions from code review

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Rework tests

* Update documentation

* Apply suggestions from code review

Co-authored-by: Jeremy Udit <jcudit@github.com>

Co-authored-by: Jeremy Udit <jcudit@github.com>
kazaker pushed a commit to auto1-oss/terraform-provider-github that referenced this pull request Dec 28, 2022
…gement (integrations#975)

* Add new resource github_team_memberships to allow authorative team management

* Fix typo in test

* Add docs and rename resource to github_team_members

* Apply suggestions from code review

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Rework tests

* Update documentation

* Apply suggestions from code review

Co-authored-by: Jeremy Udit <jcudit@github.com>

Co-authored-by: Jeremy Udit <jcudit@github.com>
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.

4 participants