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

github_team creates team with no members that is unmanageable #130

Open
ghost opened this issue Aug 11, 2018 · 10 comments
Open

github_team creates team with no members that is unmanageable #130

ghost opened this issue Aug 11, 2018 · 10 comments
Labels
r/team Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Milestone

Comments

@ghost
Copy link

ghost commented Aug 11, 2018

This issue was originally opened by @steven-edgar as hashicorp/terraform#18655. It was migrated here as a result of the provider split. The original body of the issue is below.


Terraform Version

Terraform v0.11.7

Terraform Configuration Files

resource "github_team" "my-team" { 
name = "my-team" 
description = "This is my team" 
privacy = "closed" 
}

resource "github_team" "my-subteam" { 
name = "my-subteam" 
description = "This is my subteam" 
privacy = "closed" 
parent_team_id = "${github_team.my-team.id}" 
}

resource "github_team_membership" "my-team_github-user-1" { 
team_id = "${github_team.my-subteam.id}" 
username = "github-user-1" 
role = "maintainer" 
}

Debug Output

Crash Output

Expected Behavior

Created a github team with the creators account as maintainer

Actual Behavior

Created a github team with no members, which was therefor unmaintainable. We've had to contact Github support to have that resolved.

Steps to Reproduce

Github team "my-team" already existed and had been imported into the terraform state. We then added my-subteam and ran:
terraform init
terraform apply

Additional Context

References

@radeksimko
Copy link
Contributor

Hi there @steven-edgar
thanks for opening this issue. It looks like you're bumping into the exact same issue as is described in https://github.com/terraform-providers/terraform-provider-github/pull/104

Can you please take time to read that short thread and respond there (with scopes of the token used), so we get a chance to reproduce this behaviour and decide what next steps to take?

@radeksimko radeksimko added Type: Bug Something isn't working as documented Awaiting response labels Aug 13, 2018
@carinadigital
Copy link

carinadigital commented Mar 6, 2019

Hi @radeksimko

I have some proposals for fixing this. It feels wrong to add this discussion to #104 (where I'm actively participating) as there could be multiple solution PRs and the discussion should remain somewhat central. First to restate the problem:

When creating a team via the github UI, the user creating the team is automatically added as a member to the team with maintainer role. This is same whether the user is an organisation admin or regular organisation member creating a team.

When terraform creates a github_team resource it it created without any members, and any team membership is controlled github_team_membership resource. This isn't a problem for token from a organisation admin user as they have access to all teams. It is a problem for regular organisation member's token when creating a team because their user isn't a member with maintainer role of the team when it tries to add itself as a member.

You may say just use the organisation admin user's token. This isn't always possible especially in larger organisations either due to delegated responsibilities or security concerns. You can see from the participation in #104 that there are multiple people affected. For example, I am in a github organisation with around 1500 repositories, where I have delegated responsibility for about 100 of them.

I'm not in favour of the current PR in #104 as we will run into a conflict of managing team membership roles through both github_team:maintainers and github_team_membership
Trying to manage the team membership role through github_team:maintainers will be problematic.
The github api for creation of new teams has a maintainers parameter to add to maintainers to the new team at create time. However it's not symmetric with getTeam functions which deals with team members only and not maintainers.

My suggestion is to to mirror the behaviour of github and add a create_default_maintainer boolean to the resource which would add the user associated with the token as a default maintainer to the team.

 resource "github_team" "foo" {
          name = "myfooteam"
          description = "foo"
          privacy = "secret"
          create_default_maintainer = true
  }

This could be done by these changes to the schema below and

  • resourceGithubTeamRead() will always set create_default_maintainer to false
  • a diff suppression function.
"create_default_maintainer": {
	Type:     schema.TypeBool,
	Optional: true,
	Default:  false,
},

This would ;

  • only add a maintainer on creation of a team, and not updates
  • not appear in diff/plans
  • Import correctly
  • Leave the team membership and role to be managed by github_team_membership

The only problem I could see is that we would want to ensure the idempotent behaviour of github_team_membership resource when adding team membership to a team where the user is already a member with maintainer role

I appreciate that I may be misusing the schema by adding a required field that stores an option, so some guidance from @radeksimko if this seems like a workable solution would be great. Alternatively does anyone have any better ideas for how to solve this?

@majormoses
Copy link
Contributor

It's technically manageable by org maintainers, I personally do not want anyone as team maintainers at my org as they should only be able to make team changes in github but I realize not every setup is the same and can see why this would be desired. I support this behind a feature flag with the old defaults to maintain backwards compatibility.

@carinadigital
Copy link

I support this behind a feature flag with the old defaults to maintain backwards compatibility.

create_default_maintainer defaulting to false is the same as the previous behaviour.

@kpfleming
Copy link
Contributor

I've just begun using this provider, and ran into this issue today for the first time. I am an org admin, so I don't need to be included in teams I create (and I usually remove myself as I don't want to receive notifications sent to the team). Before I read this issue and the two associated PRs, I assumed we could just completely suppress the automatic behavior from GitHub because there wouldn't be any case where the 'creating user' needs to be a member of the team.

Now I see that's not true, and there are completely reasonable situations where that is required. Unfortunately this conflicts with the entire concept of Terraform management of the team; it will have a member which Terraform does not have reflected in the state, and if the person managing the Terraform configuration adds a github_team_membership resource for the same user then the result will be illogical (especially if that resource is later deleted, which would remove the person from the team).

In my mind I wondered if it would be possible for a provider to tell Terraform that additional resources were automatically created as a side-effect of a resource creation operation, but that would only get the maintainer added to the state; they wouldn't be added to the HCL files, and the next plan/apply cycle would remove them!

I can't think of a good way to resolve this, because even attempting to generate a warning to the user that their HCL files will result in a team they cannot manage would require making two passes over the configuration, and Terraform doesn't do that. The best I can think of is to have the top-level github object find out whether the token in use has admin:org scope, and if it does not, then any github_team resources which have create_default_maintainer set to false would generate an error.

@kpfleming
Copy link
Contributor

Taking that one step further, the top-level provider could also obtain the username of the user who owns the token, and then if a github_team_membership resource is seen for that same user with permissions less than maintainer, an error would be generated.

@the-maldridge
Copy link

Not quite a year later I'm coming across this issue. It would be ideal to be able to suppress the automatic maintainer add, as I now have a user that got added to every team I created with terraform. Since this is a service principal acting at organization level it should be transparent without needing to be a maintainer in every group. There are valid cases where this can lock up, but I would argue that those are cases that the github API needs to solve, not terraform. I would happily 👍 a PR that adds a flag to create teams that don't have the creating user as a member.

@jcudit jcudit added this to the v4.0.2 milestone Nov 17, 2020
@jcudit jcudit added the r/team label Dec 8, 2020
@jcudit jcudit removed this from the v4.1.1 milestone Dec 8, 2020
@jcudit jcudit added this to the v4.4.0 milestone Jan 16, 2021
jcudit pushed a commit that referenced this issue Jan 18, 2021
This adds a possible fix to the following issues by providing users with an option to remove the automatic addition of a default maintainer to a team during creation.

/cc #527
/cc #104
/cc #130
jcudit pushed a commit that referenced this issue Feb 5, 2021
This adds a possible fix to the following issues by providing users with an option to remove the automatic addition of a default maintainer to a team during creation.

/cc #527
/cc #104
/cc #130
jcudit pushed a commit that referenced this issue Feb 5, 2021
* Add `create_default_maintainer` option to `github_team`

This adds a possible fix to the following issues by providing users with an option to remove the automatic addition of a default maintainer to a team during creation.

/cc #527
/cc #104
/cc #130
k24dizzle added a commit to lyft/terraform-provider-github that referenced this issue Feb 22, 2021
* v4.1.0

* Fix unable to resolve node id for branch_protection (integrations#610)

* Don't check node id for length

* Check if node id is valid base 64

Co-authored-by: Willem Gillis <[email protected]>

* temporarily disable PR acceptance testing

these jobs all fail and are confusing to contributors when launched from 
a PR raised by a fork.  there are ways to get around this, but will 
defer until the repository is transferred.  disabling for now.

* remove `ForceNew` on `template*` as they are concerns only at creation time (integrations#609)

There are a number of resources that have been marked as `ForceNew: true` out of a desire in "correctness" by those that do not actually understand how these resources are used in the real world and damage that can be done. No one wants to blow up a repository to change something like this, if they need to there is a mechanism built into terraform called [taint](https://www.terraform.io/docs/commands/taint.html). While there are some things that make sense for using `ForceNew` a repository for source control on properties that the API will ignore outside of creation is not one of them.

Signed-off-by: Ben Abrams <[email protected]>

* Add diff suppression function to the branch protection resource (integrations#614)

Adding a diff suppression function to the branch protection resource to
ignore the strict status check field if no contexts have been specified.

This resolves the issue with the GraphQL API returning a strict status
check value of "true" by default, regardless of contexts being set or
not.

* Add Apps to actor types in branch protection (integrations#615)

Adding Github Apps to actor types in the branch protection resource.

NOTE: Apps as an actor type is only available in push restrictions.

* Added `allowsDeletions`and `allowsForcePushes`settings (integrations#623)

* Added `allowsDeletions`and `allowsForcePushes`settings https://developer.github.com/v4/changelog/2020-11-13-schema-changes/ (#1)

* complete documentation

* update module github.com/shurcooL/githubv4 with `go get github.com/shurcooL/githubv4`

* vendor latest githubv4

* add test for deletions and force pushes

Co-authored-by: Jeremy Udit <[email protected]>

* Fix syntax error

* Conditionally Run GHES Test Suite

* Run gofmt (integrations#645)

Signed-off-by: Stephen Hoekstra <[email protected]>

* Allow dependabot to check github actions (integrations#643)

* Typo: s/visiblity/visibility (integrations#629)

Small typo in the docs.

* github_repository_webhook: describe content_type options (integrations#510)

* change private to visibility (integrations#635)

* Use commit SHA to lookup commit info in github_repository_file resource (integrations#644)

* Fix references to "master"

Signed-off-by: Stephen Hoekstra <[email protected]>

* Use commit SHA to lookup commit info

Currently the provider loops through commits in a repo until it finds the most recent commit containing the managed file. This is inefficient and could lead to you being rate limited if managing a few files that were updated a long time ago.

This commit fixes that by storing the commit SHA when updating the file and using that SHA to lookup the commit info instead of looping through all commits.

Signed-off-by: Stephen Hoekstra <[email protected]>

* add release automation for terraform registry

/cc https://www.terraform.io/docs/registry/providers/publishing.html

* Add v4.2.0 Release (integrations#641)

* add v4.1.1 release items

* correct semver version

* Document Additional Breaking Change For v3.0.0

* add `github_repository_file` bugfix

* move to correct release

* add goreleaser configuration to enable release automation

* resource/repository: add support for enabling github pages (integrations#490)

* add support for enabling github pages

* update resource comments

* add additional comments in expand methods

* add formatting fixes

Co-authored-by: Jeremy Udit <[email protected]>

* Add `github_branch_protection_v3` Resource (integrations#642)

* Add `branch_protection_v3` Resource

- add new resource to `website/github.erb`
- add new resource to `website/docs/r/<resource>.html.markdown`
- add new resource to `github/provider.go`
- add tests for resource in `github/resource_<resource>_test.go`
- implement new resource in `github/resource_<resource>.go`

* fixup! gofmt fixes

* add changelog entries for v4.3.0 release (integrations#658)

* Remove github.com/hashicorp/terraform from dependencies (integrations#628)

* remove github.com/hashicorp/terraform from dependencies

* go mod tidy

* refactor: execute fmt

* Allow dependabot to check go dependencies (integrations#653)

* Fix link to Milestones page (integrations#663)

* github_branch_default: send only fields that changed. Fixes integrations#625 integrations#620. (integrations#666)

* github_branch_default: send only fields that changed. Fixes integrations#625 integrations#620.

* fix failing test and update docs

Co-authored-by: Jeremy Udit <[email protected]>

* Fix error handling (integrations#668)

Do not silently proceed further on receiving an error response.

Signed-off-by: rustyclock <[email protected]>

* Update CHANGELOG.md

* Remove Obsolete Test

My understanding is our use of Terraform Registry makes this failing test unnecessary.

* Update GitHub organization references (integrations#672)

Following the project transfer from `terraform-providers` organization to `integrations`.

* Add Example For `github_team_repository` (integrations#676)

* Add Example For `github_team_repository`

also documents a limitation with `for_each` in this scenario

* fixup! add newline

* changelog: manually fix links to issues (integrations#673)

They were pointing to the old archived repo, which doesn't have newer issues.

* Handle base64 decodable repo names (integrations#684)

* github/config: Fix detection of individual, non-org accounts (integrations#685)

- Previously, whenever an individual user tried to interact with their
  repos, the provider would return an error, rendering v4.3.1 unusable
  _for individuals_:

  ```
  ➜ terraform plan
  Error: GET https://api.github.com/orgs/issyl0: 404 Not Found []
  on /Users/issyl0/repos/terraform/github.tf line 1, in provider "github":
  ```

- `ConfigureOwner` works such that if the `owner.name` is not blank (ie,
  the user had specified `owner = <username>` in their Terraform file),
  the code progresses to check if the `owner.name` is an org.
  Importantly, that check (prior to this change) returned an error if
  `owner.name` was not an org. That final error meant it was impossible
  to run if not using an organisation account.

- Reproduction steps: https://gist.github.com/issyl0/cd61e4cb59de2c2e1e8f45e3cf7c12f5

* add changelog entry for v4.3.2

* Add `create_default_maintainer` Option To `github_team` (integrations#661)

* Add `create_default_maintainer` option to `github_team`

This adds a possible fix to the following issues by providing users with an option to remove the automatic addition of a default maintainer to a team during creation.

/cc integrations#527
/cc integrations#104
/cc integrations#130

* Refresh CONTRIBUTING Documentation (integrations#682)

* refresh contributing docs

* add quick instructions

* add updates for newer versions of terraform

* Add Diff Suppression Option To `repository_collaborator` (integrations#683)

* add diff suppression option to `repository_collaborator`

* fixup! remove comment

* remove hardcoded username from test

* Update CHANGELOG for v4.4.0 release

* Add repo context to error message when branch is not found (integrations#691)

* Add repo context to error message when branch is not found

* fix failing `TestAccGithubRepositoryFile` test

access committer data through `Commit` struct

Co-authored-by: Jeremy Udit <[email protected]>

* fix based on linter (integrations#694)

* add v4.4.1 release notes

* Modify github_team_repository to accept slug as a valid team_id as well (integrations#693)

* First attempt

* Add comment

* Attempt to modify unit tests

* Edit docs to reflect change

* Make sure team_id is set appropriately

* fixing lint

* add passing tests for `github_team_repository`

Co-authored-by: Jeremy Udit <[email protected]>

* add v4.5.0 release notes

* temporarily ignore `darwin/arm64` to unblock releases

/cc integrations#695

* update release notes for v4.5.0

Co-authored-by: tf-release-bot <[email protected]>
Co-authored-by: Polygens <[email protected]>
Co-authored-by: Willem Gillis <[email protected]>
Co-authored-by: Jeremy Udit <[email protected]>
Co-authored-by: Ben Abrams <[email protected]>
Co-authored-by: Patrick Marabeas <[email protected]>
Co-authored-by: Francois BAYART <[email protected]>
Co-authored-by: Stephen Hoekstra <[email protected]>
Co-authored-by: John Losito <[email protected]>
Co-authored-by: Bret <[email protected]>
Co-authored-by: Jakub Holy <[email protected]>
Co-authored-by: Ichinose Shogo <[email protected]>
Co-authored-by: angie pinilla <[email protected]>
Co-authored-by: Shu Kutsuzawa <[email protected]>
Co-authored-by: Oleksandr Dievri <[email protected]>
Co-authored-by: Dee Kryvenko <[email protected]>
Co-authored-by: Ravi <[email protected]>
Co-authored-by: Alexis Gauthiez <[email protected]>
Co-authored-by: Christian Höltje <[email protected]>
Co-authored-by: Issy Long <[email protected]>
Co-authored-by: Michael Barany <[email protected]>
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this issue Jul 26, 2022
…#661)

* Add `create_default_maintainer` option to `github_team`

This adds a possible fix to the following issues by providing users with an option to remove the automatic addition of a default maintainer to a team during creation.

/cc integrations#527
/cc integrations#104
/cc integrations#130
@github-actions
Copy link

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Dec 12, 2022
@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone Priority: Normal labels Dec 12, 2022
@github-actions github-actions bot removed the Status: Stale Used by stalebot to clean house label Dec 13, 2022
Copy link

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Apr 25, 2024
@the-maldridge
Copy link

Still a bug.

@github-actions github-actions bot removed the Status: Stale Used by stalebot to clean house label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r/team Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants