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

resource/github_team: Add maintainers #104

Closed
wants to merge 1 commit into from

Conversation

amitizle
Copy link

@amitizle amitizle commented Jul 2, 2018

In case not using Admin account/token, the newly created team resource is created with 0 members, not allowing the service account / user account to proceed and add members, sub-teams or repos.

cc @caquino

@radeksimko radeksimko added the Type: Feature New feature or request label Aug 2, 2018
@radeksimko radeksimko changed the title Add maintainers to resource_github_team.go resource/github_team: Add maintainers Aug 2, 2018
@radeksimko
Copy link
Contributor

Hi @amitizle
thank you for your PR and sorry for a small delay in reviewing it.

Before we move on, do you mind explaining the use case a bit further? Specifically I'm curious what permissions does the token/user have (and relationship to the org) that you're allowed to invite new maintainers during creation of the team, but not afterwards?

Do I understand correctly that you cannot achieve this via github_team_membership resource?

@amitizle
Copy link
Author

amitizle commented Aug 9, 2018

Hey @radeksimko, no worries and thanks for the response.
Yes what you mentioned is more or less the use case.
If within an org I'm not an admin (global admin), I still can create a team.
If I'm creating the team via the Github web app it automatically adds me to the team as a maintainer, making is possible for me (as the maintainer) to invite more members.
Using the provider, it creates the team with no members and then me as the Terraform operator (either personal or a service account that is not a global admin) cannot add members.
So the plan phase succeeds, but the apply phase fails due to me not being able to add new members (neither myself) to the new team.

@radeksimko
Copy link
Contributor

Do you mind listing all the scopes that you have set for your token?

Is it possible that as a user clicking in the UI, you have more permissions than the token (used by Terraform) has?

If that is not the case, then I'd be tempted to call this a bug and I'd probably raise it with GitHub support.

@amitizle
Copy link
Author

amitizle commented Aug 9, 2018

I'll try to reproduce soon, with a new token and will post in here.

@steven-edgar
Copy link

I can confirm this bug report. This was creating a subteam within an organisation, with an API key with the following scopes

repo (all)
admin:org (all)
delete_repo

Account that created the API key is able to create subteams and add members without issue. The same terraform code, using the same API key, is able to add members to other teams without issue.

@radeksimko
Copy link
Contributor

Thanks @steven-edgar for providing these details, that's helpful. I'll try to reproduce.

@radeksimko
Copy link
Contributor

I spent some time trying to reproduce this. Here's the full list of steps I took:


Create a new personal token with the following scopes (as instructed above by @steven-edgar ) under my personal account (@radeksimko):

screen shot 2018-08-15 at 10 32 16


Just for completeness I am an Owner of the org - but I don't think it really matters in the context of any token.

screen shot 2018-08-15 at 10 33 20


Run GITHUB_TOKEN=... terraform apply (where ... is the token from previous step) with the following config:

provider "github" {
  organization = "terraformtesting"
}

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 = "hashibot" 
  role = "maintainer" 
}

$ terraform apply
github_team.my-team: Creating...
  description: "" => "This is my team"
  name:        "" => "my-team"
  privacy:     "" => "closed"
github_team.my-team: Creation complete after 1s (ID: 2872296)
github_team.my-subteam: Creating...
  description:    "" => "This is my subteam"
  name:           "" => "my-subteam"
  parent_team_id: "" => "2872296"
  privacy:        "" => "closed"
github_team.my-subteam: Creation complete after 0s (ID: 2872297)
github_team_membership.my-team_github-user-1: Creating...
  role:     "" => "maintainer"
  team_id:  "" => "2872297"
  username: "" => "hashibot"
github_team_membership.my-team_github-user-1: Creation complete after 1s (ID: 2872297:hashibot)

Apply complete! Resources: 3 added, 0 changed, 0 destroyed.

I was therefore unable to reproduce this. Can please you provide further details? e.g. about the hierarchy of your teams in question?

Thanks.

@steven-edgar
Copy link

The team hierarchy is simply as described before, "my-team" doesn't reside under any other team. There are lots of other teams at the org level, but I'm not sure that's relevant. There were no other subteams under "my-team" at the time I created "my-subteam" the first time and encountered the zero members issue, I did create via TF a "my-subteam2" under "my-team", with the same results. I then manually created a subteam under "my-team", using the same account via the website, and used exactly the same terraform and API key as before, with the name of the subteam changed to match the manually created subteam, applied that and it applied the members correctly to the subteam.

The only other thing I can think of that might be relevant is the org requires SSO. The API key is enabled for SSO login, obviously, otherwise things wouldn't have got as far as they have.

Not sure what other details might be helpful for you.

@damacus
Copy link

damacus commented Nov 20, 2018

Hey!

is there anything I can do to get this PR moving? I believe it's going to fix a bug I'm seeing when we add a team using a non-Organisation maintainer.

@ghost ghost removed the Awaiting response label Nov 20, 2018
@carinadigital
Copy link

carinadigital commented Nov 20, 2018

@radeksimko I think being the org owner, and not an org member is why you didn't see the bug.

Just for completeness I am an Owner of the org - but I don't think it really matters in the context of any token.

I work in a large github organisation. We manage a subset of teams and repos and have encountered the problem above. I'll try to get you a full set of reproducible steps.

It might be difficult to write a failing test for this as it depends on the permissions of the user the token relates to.

@karlgrund
Copy link

karlgrund commented Feb 27, 2019

We are experiencing the same issue as described above. In a larger organization, you don't want to give a bot owner permissions to the organization. As it is now that bot can create a team, but it's not able to add anyone or itself to that team.

- module.team-test
  Updating source "modules/team"
module.team-test.github_team.main: Creating...
  description:    "" => "Team Test"
  etag:           "" => "<computed>"
  name:           "" => "team-test"
  parent_team_id: "" => "xxxxx"
  privacy:        "" => "closed"
  slug:           "" => "<computed>"
module.team-test.github_team.main: Creation complete after 1s
module.team-test.github_team_membership.members: Creating...
  etag:     "" => "<computed>"
  role:     "" => "member"
  team_id:  "" => "xxxx"
  username: "" => "karlgrund"

Error: Error applying plan:

1 error(s) occurred:

* module.team-test.github_team_membership.members: 1 error(s) occurred:

* github_team_membership.members: PUT https://api.github.com/teams/xxxx/memberships/karlgrund: 403 You must be an organization owner or team maintainer to add a team membership. []

This is the module that I'm using to create the output above.

resource "github_team" "main" {
  name           = "${var.team_name}"
  description    = "${var.team_description}"
  parent_team_id = "${var.parent_id}"
  privacy        = "${var.privacy}"
}

resource "github_team_membership" "members" {
  count = "${length(var.members)}"

  team_id  = "${github_team.main.id}"
  username = "${var.members[count.index]}"
  role     = "member"
}

We are using version 1.3.0 of the GitHub provider and this solution would unblock us.

@karlgrund
Copy link

I would add the label bug on this PR since you can only use this feature if you're an organization owner/admin.
@amitizle would you be able to have a look at the conflicts?

* github_team_membership.members: PUT https://api.github.com/teams/xxxx/memberships/karlgrund: 403 You must be an organization owner or team maintainer to add a team membership. []

@carinadigital
Copy link

I've resolved the conflicts with @amitizle PR at https://github.com/carinadigital/terraform-provider-github/tree/add_maintainer

I'm currently ensuring all the functionality and tests work.

@amitizle
Copy link
Author

amitizle commented Mar 5, 2019

Hey friends, sorry for missing all of that! I was moving country, having a new baby and starting a new job :D

Can I help with this thing?

@carinadigital
Copy link

carinadigital commented Mar 5, 2019

The current functionality won't work as intended.

There is no readMaintainers() like function on teams (https://godoc.org/github.com/google/go-github/github#Team). The current changes to the schema would introduce two places were we would be managing the same resource (github_team_membership and github_team maintainers attribute).

I'm in favour of taking a different direction with this. One option is to make a change to maintainer, so that it's only used at create time, and then not part of the terraform plans.

However, my preferred option is to mirror the functionality provided by GitHub UI where the user creating the team is automatically added as a maintainer. This solves our problem when we create teams when our terraform credentials are not the organisational owner credentials.

Something like

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

Once I have enough progress, I'll create a new PR and reference it here.

@carinadigital
Copy link

I've sought some guidance at https://github.com/terraform-providers/terraform-provider-github/issues/130 before starting, as the solution might be a little hacky.

@fmasuhr
Copy link

fmasuhr commented Mar 12, 2020

We are maintaining GitHub teams and memberships with an Github access token of an owner. In this case the user is automatically added as team member via the Github API as described here https://developer.github.com/v3/teams/#create-team.
But I think this is actually not a good practice for terraform as memberships are maintained additionally via the github_team_membership. The API is producing side effects which are not represented in the statefile.
The add_default_maintainer attribute proposed by @carinadigital could be a good way to solve this. In the case of add_default_maintainer = false the automatically added user membership should then be dropped again from the team. This will make sure there are no memberships created without adding them to the state.

jcudit pushed a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 <willem.gillis@imec.be>

* 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 <me@benabrams.it>

* 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 <jcudit@github.com>

* Fix syntax error

* Conditionally Run GHES Test Suite

* Run gofmt (integrations#645)

Signed-off-by: Stephen Hoekstra <shoekstra@schubergphilis.com>

* 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 <shoekstra@schubergphilis.com>

* 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 <shoekstra@schubergphilis.com>

* 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 <jcudit@github.com>

* 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 <jcudit@github.com>

* Fix error handling (integrations#668)

Do not silently proceed further on receiving an error response.

Signed-off-by: rustyclock <rustyclock@protonmail.com>

* 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 <jcudit@github.com>

* 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 <jcudit@github.com>

* 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 <terraform@hashicorp.com>
Co-authored-by: Polygens <willem.gillis@gmail.com>
Co-authored-by: Willem Gillis <willem.gillis@imec.be>
Co-authored-by: Jeremy Udit <jcudit@github.com>
Co-authored-by: Ben Abrams <me@benabrams.it>
Co-authored-by: Patrick Marabeas <patrick@marabeas.io>
Co-authored-by: Francois BAYART <francois.bayart@kensu.io>
Co-authored-by: Stephen Hoekstra <shoekstra@schubergphilis.com>
Co-authored-by: John Losito <lositojohnj@gmail.com>
Co-authored-by: Bret <166301+bcomnes@users.noreply.github.com>
Co-authored-by: Jakub Holy <jakubholy@jakubholy.net>
Co-authored-by: Ichinose Shogo <shogo82148@gmail.com>
Co-authored-by: angie pinilla <angelinepinilla@gmail.com>
Co-authored-by: Shu Kutsuzawa <cappyzawa@yahoo.ne.jp>
Co-authored-by: Oleksandr Dievri <o.dievri@gmail.com>
Co-authored-by: Dee Kryvenko <109895+dee-kryvenko@users.noreply.github.com>
Co-authored-by: Ravi <1299606+rustycl0ck@users.noreply.github.com>
Co-authored-by: Alexis Gauthiez <alexis.gauthiez@gmail.com>
Co-authored-by: Christian Höltje <docwhat@gerf.org>
Co-authored-by: Issy Long <me@issyl0.co.uk>
Co-authored-by: Michael Barany <1434605+mbarany@users.noreply.github.com>
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request 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
@nickfloyd
Copy link
Contributor

👋 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!

@nickfloyd nickfloyd added the Status: Stale Used by stalebot to clean house label Nov 30, 2022
@github-actions github-actions bot closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Used by stalebot to clean house Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants