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_repository cannot handle custom role after creation #1183

Closed
joshua-hancox opened this issue Jun 7, 2022 · 11 comments · Fixed by #1192 or #1369
Closed

resource_github_team_repository cannot handle custom role after creation #1183

joshua-hancox opened this issue Jun 7, 2022 · 11 comments · Fixed by #1192 or #1369
Labels
Type: Bug Something isn't working as documented

Comments

@joshua-hancox
Copy link
Contributor

Affected Resource(s)

  • resource_github_team_repository

Expected Behavior

After creating a resource_github_team_repository with a custom role set in the permission field, making no changes, and re-running the plan, I expect no changes to be planned by terraform.

Actual Behavior

After creating the resource and re-running the plan, terraform plan tries to change from the built in role that the custom role was based on, to the role that was originally set.

Steps to Reproduce

e.g. config

resource "github_team_repository" "custom_role" {
  team_id    = github_team.this.id
  repository = github_repository.this.name
  permission = "my-custom-role"
}

(assume that my-custom-role was created in github, and based on the built in maintainer role, with some added permissions)

  1. Run terraform apply
  2. Once the terraform apply has been applied, run terraform plan

The plan output will show:

  # github_team_repository.custom_role will be updated in-place
  ~ resource "github_team_repository" "custom_role" {
        id         = "5817097:my-repo-name"
      ~ permission = "maintainer" -> "my-custom-role"
    }

Suggested fix

I think this issue comes from this function being called here to guess what the permission is that is applied.

It assumes that there are only built in roles, which is no longer the case.

I have extended the Repository struct in go-github with this PR to include the role_name field in the repository that is returned(automatically includes the .GetRoleName() function), and also raised a PR in this repo to bump to the latest version of that module.

Once that is complete I think we should be able to remove the function getRepoPermission altogether and amend resource_github_team_repository to do it's permission look up from:

	permName, permErr := getRepoPermission(repo.GetPermissions())
	if permErr != nil {
		return permErr
	}

	d.Set("permission", permName)

to

	d.Set("permission", repo.GetRoleName())

If that makes sense, and we can merge the PR to bump the module then let me know and I can raise a new PR for the fix.

@joshua-hancox
Copy link
Contributor Author

Hey @kfcampbell - sorry to pester you, I noticed this behaviour when using the custom roles that we merged changes for recently. I wonder if you could take a look.

@kfcampbell
Copy link
Member

@joshuahancox No worries at all. I'll set aside some time to troubleshoot this next week.

@joshua-hancox
Copy link
Contributor Author

joshua-hancox commented Jun 16, 2022

I raised this PR (#1192) with the idea that I had to resolve this, still in draft currently though as I noticed that I couldn't access the role_name from go-github / Repositories.ListCollaborators either.

I am now waiting on a new PR there (google/go-github#2386)

If I can do that, then I think the getRepoPermission() becomes redundant and can be removed.

Let me know what you think, happy to make more changes if required.

@joshua-hancox
Copy link
Contributor Author

Hi @kfcampbell!

Could we merge https://github.com/integrations/terraform-provider-github/pull/1205/files so that I can continue working on this?

@joshua-hancox
Copy link
Contributor Author

@kfcampbell did you have any time to look at this one?

@SharpEdgeMarshall
Copy link
Contributor

This is blocking us from adopting terraform github, any news?

@kfcampbell
Copy link
Member

I've merged the dependency PR and updated @joshua-hancox's PR with main before merging.

Thank you for the contribution, @joshua-hancox!

@SharpEdgeMarshall
Copy link
Contributor

@kfcampbell may we reopen this?

@joshua-hancox
Copy link
Contributor Author

Apologies for the oversight @kfcampbell! I've raised a new PR that may help, reusing the existing function getInvitationPermission() that already does some conversion for this issue.

Perhaps we should also rename the function? (since it is now not just related to invitations)

Could someone take a look, I am not 100% sure on what the edge case was that we broke before.

PR: #1230

@Luis-3M
Copy link

Luis-3M commented Oct 27, 2022

Looking forward to seeing this fixed guys 👍

@nickfloyd nickfloyd added Type: Bug Something isn't working as documented Priority: High labels Oct 31, 2022
@kfcampbell
Copy link
Member

Sorry for the delay here @joshua-hancox! If you want to take a look at fixing the merge conflicts in #1230, I'll prioritize a quick review and release. If not, I can take a look at some point. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
5 participants