[BUG] Unable to add github_repository_collaborators.

[jarnohenneman]

Hello,

We are trying to add github_repository_collaborators to a github_repository when we run the plan we see the outcome of what we expect/want. A team is added to a repository with the permission set maintain, but when running the terraform apply, it comes back with an API not found error.

Plan output;

  # github_repository.this["example-repository"] will be updated in-place
  ~ resource "github_repository" "this" {
        id                          = "example-repository"
        name                        = "example-repository"
      - vulnerability_alerts        = true -> null
        # (33 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

# github_repository_collaborators.this["example-repository"] will be created
  + resource "github_repository_collaborators" "this" {
      + id             = (known after apply)
      + invitation_ids = (known after apply)
      + repository     = "example-repository"

      + team {
          + permission = "maintain"
          + team_id    = "example-team"
        }
    }

Error response in the terminal;

│ Error: PUT https://api.github.com/orgs/---/teams/example-team/repos/---/example-repository: 404 Not Found []
│ 
│   with github_repository_collaborators.this["example-repository"],
│   on teams.tf line 75, in resource "github_repository_collaborators" "this":
│   75: resource "github_repository_collaborators" "this" {
│ 
╵
ERRO[0266] Terraform invocation failed in ....
ERRO[0266] 1 error occurred:
        * exit status 1

We have updated to the latest version of Terraform Github Provider.

terraform -v
Terraform v1.4.2

• provider registry.terraform.io/integrations/github v5.23.0

[kfcampbell]

Can you please provide an example of HCL that can be used to reproduce this issue?

[kdean-hdai]

I am also seeing this. I can replicate with something as simple as:

terraform {
  required_providers {
    github = {
      source  = "integrations/github"
      version = "~> 5.0"
    }
  }

  required_version = ">= 1.2.0"
}

provider "github" {}

resource "github_repository_collaborator" "main" {
  repository = "my-org/my-repo"
  username   = "kdean-hdai"
  permission = "push"
}

I am using GITHUB_TOKEN in my environment for auth. terraform version output:

Terraform v1.4.5
on darwin_arm64
+ provider registry.terraform.io/integrations/github v5.23.0

[kfcampbell]

Ahh, thank you! Okay so I think what's going on here is that this code is pulling the owner from the set GITHUB_OWNER or, if that's not set, falling back to the owner of the token used.

When a repo is given with an owner present in the string, it's not used as the owner in the actual call. My vote would be to either clarify that the repo must include its owner in the string and then parse the owner out for the API call, or else add a separate owner parameter to the schema.

Do you have thoughts on what the more elegant solution might be?

[kdean-hdai]

A-ha!

Those both sound reasonable to me. I have a slight preference for requiring owner in the repository string, since that matches the output available in https://registry.terraform.io/providers/integrations/github/latest/docs/data-sources/organization#repositories, and is accessible via https://registry.terraform.io/providers/integrations/github/latest/docs/data-sources/repositories#full_names.

I had an initial thought that it could be more flexible, supporting both my-repo and my-org/my-repo, by checking for / in the string. However, I don't have a great sense of how to weigh the benefit of flexibility there with the added complexity for folks who just pass in my-repo. It seems easier to have nice, unambiguous, errors with your suggestion.

[kfcampbell]

Sounds good! I'd be happy to go the route of requiring a name with owner in the repo string and defaulting to the token owner otherwise. In a perfect world, we'd throw an error if a slash isn't present, but that would make this a breaking change.

This item is up for grabs if anybody has the inclination to submit such a PR!

[ppatel1604]

@kfcampbell What do you think about something like below:

	var owner string

	// Github replaces '/' with '-' for a repo name so it is safe to assume that if repo name contains '/'
	// then first part will be the owner name and second part will be the repo name
	if strings.Contains(repoName, "/") {
		repoNameWithOwnerName := strings.Split(repoName, "/")
		owner = repoNameWithOwnerName[0]
		repoName = repoNameWithOwnerName[1]
	} else {
		owner = meta.(*Owner).name
	}

If that looks good, I am happy to create a PR with some unit tests and some documentation updates :)

[kfcampbell]

@ppatel1604 Sorry for the late reaction here; I've been on vacation recently. That sounds reasonable to me!

[ppatel1604]

@ppatel1604 Sorry for the late reaction here; I've been on vacation recently. That sounds reasonable to me!

Great. Can you please assign it to me. I will raise the PR soon :)

[ppatel1604]

@kfcampbell Apologies if you have spammed with the invite emails. I am working on some tests for this bug. I have copied the lunch.json from https://github.com/integrations/terraform-provider-github/blob/main/CONTRIBUTING.md#example-vscodelaunchjson-file which has your username as the Test username. I didn't realise that until now. Do we have a test user that I can used for the testing or I have to create my own?

[kfcampbell]

@ppatel1604 no worries! I did get several emails for what it's worth. You should create your own test user as we don't have a good way of sharing credentials across all contributors to this repo.

[ppatel1604]

@kfcampbell What do you think about something like below:

	var owner string

	// Github replaces '/' with '-' for a repo name so it is safe to assume that if repo name contains '/'
	// then first part will be the owner name and second part will be the repo name
	if strings.Contains(repoName, "/") {
		repoNameWithOwnerName := strings.Split(repoName, "/")
		owner = repoNameWithOwnerName[0]
		repoName = repoNameWithOwnerName[1]
	} else {
		owner = meta.(*Owner).name
	}

If that looks good, I am happy to create a PR with some unit tests and some documentation updates :)

I have tried this approach but came across some issues. With this approach, the repository name in the state file is stored without the org/owner name, but the TF file contains the repository name with the owner name. Having two different strings creates a DIFF between TF state and TF files and a forces the re-creation of the resource. Therefore, we used a different approach where we kept the attributes the same as supplied via TF file but split the values from repository strings during the api call.