integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
890 stars 733 forks source link

[BUG] Apps defined as pull_request_bypassers in github_branch_protection are not properly tracked by provider #1367

Closed djonser closed 1 year ago

djonser commented 1 year ago

Terraform Version

v1.3.1

Provider Version

v5.8.0

Affected Resource(s)

github_branch_protection

Terraform Configuration Files

resource "github_repository" "repo" {
  name = "my-repo"
  auto_init = true
  visibility = "internal"
}

resource "github_branch_protection" "protection" {
  pattern       = "main"
  repository_id = github_repository.repo.node_id

  required_pull_request_reviews {
    required_approving_review_count = 1
    pull_request_bypassers = ["A_abcd1234efgh5678"]
  }
}

Debug Output

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # github_branch_protection.pro will be updated in-place
  ~ resource "github_branch_protection" "protection" {
        id                              = "BPR_efgh5678abcd1234"
        # (10 unchanged attributes hidden)

      ~ required_pull_request_reviews {
          ~ pull_request_bypassers          = [
              + "A_abcd1234efgh5678",
            ]
            # (5 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Expected Behavior

GitHub App that has already been approved as bypasser should not trigger required change

Actual Behavior

Change to BPR is required every time terraform plan/apply runs even if no other changes.

Steps to Reproduce

Apply and then apply again.

  1. terraform apply
  2. terraform apply

Quick investigation

I looked around the source code and found a likely missing piece of logic in ./github/util_v4_branch_protection.go function setBypassPullRequestActorIDs, as well as a missing field in struct BypassPullRequestActorTypes.

I then followed the contribution guide which have a good explanation for how to use dev_overrides in terraform (first time trying this feature), made a change, built locally, and it fixed the problem for me.

I did not test any other behaviour or look around if this change had any consequences elsewhere.

Before

type BypassPullRequestActorTypes struct {
    Actor struct {
        Team Actor     `graphql:"... on Team"`
        User ActorUser `graphql:"... on User"`
    }
}

if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) {

}

After

type BypassPullRequestActorTypes struct {
    Actor struct {
        App  Actor     `graphql:"... on App"`
        Team Actor     `graphql:"... on Team"`
        User ActorUser `graphql:"... on User"`
    }
}

if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) || (a.Actor.App.ID != nil && a.Actor.App.ID.(string) == v) {

}
kfcampbell commented 1 year ago

@cormack do you have any interest in opening a PR with your change?

Nexus2k commented 1 year ago

Any update or fix for this?