integrations / terraform-provider-github

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

[BUG] github_branch_protection_v3 tries to delete the contexts every time #1701

Open julioagos opened 1 year ago

julioagos commented 1 year ago

Problem

I created some branch protection rules for some branches. These rules included required checks. After I applied them, it created matching contexts and a new check that is just one of the required checks with an app_id that always changes (First 4 digits remain the same, the rest changes). Now, every time I ran plan, it says that it is going to delete them again. The branch protections are working just as intended but, running plan always generates meaningless noise that get's in the way especially since we have a lot of branch protection rules once we fully migrate it over to Terraform. The applies are all run by Atlantis on PRs. This is what an example output of what terraform state show looks like for one of the branches:

    required_status_checks {
        checks         = [
            "test 1",
            "test 2",
            "test 1:<random-numbers>",
        ]
        contexts       = [
            "test 1",
            "test 2",
        ]
    }

Running terraform plan returns something like this

      ~ required_status_checks {
          ~ checks         = [
              - "test 1:<random-numbers>",
                # (2 unchanged elements hidden)
            ]
          ~ contexts       = [
              - "test 1",
              - "test 2",
            ]
            # (2 unchanged attributes hidden)
        }

There are no error messages at any point and the apply works as intended. It states that all plans have been applied.

Versions

Terraform: v1.4.4 GitHub Provider: Originally 5.23.0 but also happens in 5.25.1

svalevka commented 1 year ago

We have a very similiar issue with 5.25.1 provider. Using github app for authentication

  # module.github_repository.github_branch_protection_v3.branch["redacted_name.master"] will be updated in-place
  ~ resource "github_branch_protection_v3" "branch" {
        id                              = "redacted_name:master"
        # (6 unchanged attributes hidden)

      ~ required_status_checks {
          ~ checks         = [
              - "Naming conventions / Pull request:0",
                # (1 unchanged element hidden)
            ]
          ~ contexts       = [
              - "Naming conventions / Pull request",
            ]
            # (2 unchanged attributes hidden)
        }
julioagos commented 1 year ago

Yea it is probably worth mentioning, I am also using github app for authentication

0x46616c6b commented 1 year ago

I have the same problem. Since #1774 (v5.31.0) the checks diffs are gone but every plan produce a diff for contexts (regardless if they are set or not).

stefanschulte commented 1 year ago

For what it's worth, if you require a workable provider again now, you could also switch to the GraphQL based API endpoint for branch protections. The behaviour of that one wasn't changed, so there are no superfluous changes for terraform plans.

Which would be this resource: https://registry.terraform.io/providers/integrations/github/latest/docs/resources/branch_protection

0x46616c6b commented 1 year ago

You are correct; the github_branch_protection works better with required_status_checks. However, you need to access apps when using pull_request_bypassers. Public apps should work, but if you reference an internal one, it will fail when using app authentication. App authentication for the provider does not make this possible; see #1248.

Sad times to use the provider :-(

kfcampbell commented 1 year ago

@0x46616c6b if you have ideas about how we can handle this better, we're very accepting of new issues and PRs!

0x46616c6b commented 1 year ago

Would it be possible to remove the deprecated field contexts from the branch protection v3? How the actual policy for removing a deprecated field/feature looks in the provider right now?

kfcampbell commented 1 year ago

I think it's reasonable to do a major version release in the near future! It's been quite a while since we last released a breaking change (September 14th). There's a few other changes I wouldn't mind batching in there as well, such as #1029, #448, #1704, and maybe even #1780.

In general, Hashicorp recommends not doing more than one breaking change a year if we can help it.

entropitor commented 1 year ago

Is there any progress on this? This kinda makes github_branch_protection_v3 unusable.

xiaoyanli-lyft commented 1 year ago

@kfcampbell any timeline we have to officially remove contexts from the branch protection v3? this issue had been on for a while

kfcampbell commented 1 year ago

Unfortunately GitHub's SDK team doesn't have the bandwidth to work on this ourselves right now, though PRs are appreciated!

yorik commented 10 months ago

We have several issues related to that:

  ~ resource "github_branch_protection_v3" "this" {
        id                              = "foo:master"
      ~ require_signed_commits          = false -> true
        # (5 unchanged attributes hidden)

      ~ required_status_checks {
          ~ checks         = [
              - "foo",
              + "foo:-1",
            ]
          ~ contexts       = [
              - "foo",
            ]
            # (2 unchanged attributes hidden)
        }

        # (2 unchanged blocks hidden)
    }

  ~ resource "github_branch_protection_v3" "this" {
        id                              = "bar:main"
        # (6 unchanged attributes hidden)

      ~ required_status_checks {
          ~ contexts       = [
              - "TruffleHog",
            ]
            # (3 unchanged attributes hidden)
        }

        # (2 unchanged blocks hidden)
    }

So there should be added special check for app_id == -1 and probably it's possible to hack that if checks do exist in terraform state just ignore contexts completely.

Would that work?

github-actions[bot] commented 1 month ago

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

yorik commented 1 month ago

This is still very annoying issue: it shows huge diff, while actual change could be small, so it's easy to miss something.

rsi-mrobinson commented 2 weeks ago

Also facing this, tried adding the contexts to my terraform as I was not using the field since it's been marked as deprecated. So instead of useless plan output showing changes that don't matter, now I have GitHub is deprecating the use ofcontexts. Use achecksarray instead. yippee...