Modify github_team to accept slug as a valid parent_team_id

[n0rad]

Inspired by #693, This PR add support for declaring parent_team_id as a string on github_team.

This is especially useful when declaring teams in a loop and so cannot reference the other team object .

[jcudit]

The change looks accurate, but I have concerns about how existing state would migrate to a different data type. This is potentially disruptive for existing callers of parent_team_id as they may need to rewrite their HCL to align to this update.

Would a new string field like parent_team_slug satisfy your use case? Or is parent_team_id required?

[n0rad]

thx,
I don't know terraform code enough to know how field type change is supported, but I can do some tests to see how it goes.

Sure it can be another field, as soon as we can set parent as name it will match our case.
However it will require more code and doc on which field to work with

[n0rad]

Ok so I did some manual tests by creating teams with parent on v4.10.1 and switches to my PR version.
Changing the parent from a number to a string is working smoothly with obvisouly a plan to apply but it's switching with no error.
I changed to another team to be sure it's correclty going forward and it's ok.

[jcudit]

Ran into the following when validating this:

=== RUN   TestAccGithubTeamHierarchical/creates_a_hierarchy_of_teams_using_parent_slug/with_an_organization_account
    testing.go:654: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: github_team.child
          create_default_maintainer: "false" => "false"
          description:               "Terraform acc test child team" => "Terraform acc test child team"
          etag:                      "W/\"2bc6282187ab6b555f96d8e78a18cfcfc7775b092c7d7bf04bc831fdd310f76d\"" => "W/\"2bc6282187ab6b555f96d8e78a18cfcfc7775b092c7d7bf04bc831fdd310f76d\""
          id:                        "4892306" => "4892306"
          ldap_dn:                   "" => ""
          members_count:             "0" => "0"
          name:                      "tf-acc-child-zggde-2" => "tf-acc-child-zggde-2"
          node_id:                   "MDQ6VGVhbTQ4OTIzMDY=" => "MDQ6VGVhbTQ4OTIzMDY="
          parent_team_id:            "4892305" => "tf-acc-parent-zggde-2"
          privacy:                   "closed" => "closed"
          slug:                      "tf-acc-child-zggde-2" => "tf-acc-child-zggde-2"
        
        
        
        STATE:
        
        github_team.child:
          ID = 4892306
          provider = provider.github
          create_default_maintainer = false
          description = Terraform acc test child team
          etag = W/"2bc6282187ab6b555f96d8e78a18cfcfc7775b092c7d7bf04bc831fdd310f76d"
          ldap_dn = 
          members_count = 0
          name = tf-acc-child-zggde-2
          node_id = MDQ6VGVhbTQ4OTIzMDY=
          parent_team_id = 4892305
          privacy = closed
          slug = tf-acc-child-zggde-2
        
          Dependencies:
            github_team.parent
        github_team.parent:
          ID = 4892305
          provider = provider.github
          create_default_maintainer = false
          description = Terraform acc test parent team
          etag = W/"ab36a497bce6797e82636307ff56b3bcdb56d3d572a8abd46d9f7f7481217683"
          ldap_dn = 
          members_count = 0
          name = tf-acc-parent-zggde-2
          node_id = MDQ6VGVhbTQ4OTIzMDU=
          parent_team_id = 
          privacy = closed
          slug = tf-acc-parent-zggde-2

See 7672edb for a test that can be cherry-picked in. 🤔 there seems to be something up with how we set the value of parent_team_id as on subsequent runs, the provider is trying to update it to the slug value instead of the numeric ID:

      parent_team_id:            "4892305" => "tf-acc-parent-zggde-2"

[cytopia]

@usmonster @jcudit do you have a schedule to work on this? This change simplifies team creation with TF hugely.

@n0rad thanks for the PR 👍 , we're currently playing around with your changes and have released it (only linux-amd64 and darwin-amd64 though)

• https://github.com/Flaconi/terraform-provider-github/releases/tag/v4.19.0
• https://registry.terraform.io/providers/Flaconi/github/latest

[cytopia]

@n0rad I found an issue with this current slug name approach (instead of using ID's).

When changing the team name of a parent (and also adjusting their parent name at the same time) and then trying to apply everything, it usually leads to a 404 error. This is due to the fact that the child team wants to update their parent team and fetches this via the slug, however if the parent name has not changed yet, it will result in a 404 not found and everything will fail.

A workaround that I am currently using (and it seems to be working fine) is to loop multiple times (and also wait in between) around fetching the parent id when failed. In the meantime (during sleep and retry) the parent team name eventually gets updated.

I am pretty new to golang and have created a PoC PR on the provider that we're using at flaconi. I am pretty sure that there is a better more elegant solution to it. You can find my code changes here: Flaconi#3 I would suggest to have something like this or an alternative solution also implemented into this PR to mitigate this issue

(CC @jcudit @usmonster )

How to reproduce:

1. Apply
2. Change the Engineering team name in both, the team and as a parent in the childs
3. Apply again

Root module

terraform.tfvars

teams = [
  # ------------------------------------------------------------
  # DevOps
  # ------------------------------------------------------------
  {
    ident       = "devops"
    name        = "DevOps"
    description = "The DevOps Team"
    privacy     = "closed"
    parent_name = null
    members = []
  },
  # ------------------------------------------------------------
  # Engineering
  # ------------------------------------------------------------
  {
    ident       = "engineering"
    name        = "Engineering"
    description = "The Engineering Team"
    privacy     = "closed"
    parent_name = null
    members = []
  },
  {
    ident       = "eng-sub-1"
    name        = "Sub-1"
    description = "Team Sub-1"
    privacy     = "closed"
    parent_name = "Engineering"
    members     = []
  },
  {
    ident       = "eng-sub-2"
    name        = "Sub-2"
    description = "Team Sub-2"
    privacy     = "closed"
    parent_name = "Engineering"
    members     = []
  },
  {
    ident       = "eng-sub-3"
    name        = "Sub-3"
    description = "Team Sub-3"
    privacy     = "closed"
    parent_name = "Engineering"
    members     = []
  },
]

variables.tf

variable "token" {
  description = "Github token to use when adding membership"
  type        = string
}

variable "owner" {
  description = "Github organization name"
  type        = string
}

variable "teams" {
  description = "GitHub teams to manage."
  type = list(object({
    ident       = string
    name        = string
    description = string
    privacy     = string
    parent_name = string
    members     = list(string)
  }))
}

main.tf

locals {
  teams = { for index, team in var.teams : team.ident => team }
}

module "team" {
  for_each   = local.teams

  source = "./modules/team"

  name        = each.value.name
  description = each.value.description
  privacy     = each.value.privacy
  parent_name = each.value.parent_name
  members     = each.value.members
}

team module

team/variables.tf

variable "name" {
  description = "GitHub team name"
  type        = string
}

variable "description" {
  description = "GitHub team description"
  type        = string
  default     = ""
}

variable "privacy" {
  description = "GitHub team privacy (closed / secret)"
  type        = string
  default     = "closed"
}

variable "parent_name" {
  description = "GitHub team parent team name"
  type        = string
  default     = null
}

variable "members" {
  description = "GitHub team members"
  type        = list(string)
  default     = []
}

team/main.tf

resource "github_team" "team" {
  name                      = var.name
  description               = var.description
  privacy                   = var.privacy
  create_default_maintainer = false
  parent_team_id            = var.parent_name != null ? replace(lower(var.parent_name), " ", "-") : null
}

[n0rad]

Hi @cytopia, Thx for continuing the effort on this change.
I saw this issue on my side and would have rely on replaying the apply in such a case.I did not dig deeper on edge cases because on our side, we implemented a workaround that do not need string value and is still good enough (as soon as we do not need to build a deep team tree) and so I moved to something else.

Feel free to open another PR that bring your improvement (on top of mine, or not) to propose them to the provider maintainers.

[cytopia]

Feel free to open another PR that bring your improvement

@n0rad currently working thoroughly through resource_github_team.go and will create PR's one step at a time. First one to come which is scheduled for v4.20.0 is here: #1039

[jan-ludvik]

I apologise if adding this to a PR is a spam but I haven't found an issue for this and it might help someone else - my workaround is simple and it seems to work well.
Lookup teams

data "github_organization_teams" "teams" {}

Create <team_slug>=<team id> map in locals

locals {
  teams_to_id_data = { for team in data.github_organization_teams.teams.teams : team.slug => tostring(team.id) }
  }

Lookup ID from slug in the resource (I pull the slug from a json file which I use to create teams with for_each)

resource "github_team" "this" {
......
  parent_team_id            = lookup(local.teams_to_id_data, <team_slug>, null)
}

[nickfloyd]

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

[cytopia]

Status: Pinned

[froblesmartin]

This is done by #1664 now