github_branch_protection does not enable "Require status checks to pass before merging"

[gertjanmaas]

Terraform Version

➜ terraform --version
Terraform v1.0.3
on darwin_amd64
+ provider registry.terraform.io/hashicorp/github v4.13.0
+ provider registry.terraform.io/integrations/github v4.13.0

Affected Resource(s)

• github_branch_protection

Terraform Configuration Files

resource "github_branch_protection" "main_branch" {
  repository_id  = github_repository.my_test_repo.name

  pattern                 = "main"
  enforce_admins   = true

  required_status_checks {
    strict  = true
  }

  required_pull_request_reviews {
    required_approving_review_count = 1
  }
}

Expected Behavior

"Require status checks to pass before merging" and "Require branches to be up to date before merging" options in branch protection should be enabled.

Actual Behavior

"Require status checks to pass before merging" is disabled and "Require branches to be up to date before merging" is enabled.

Steps to Reproduce

1. Use the configuration above
2. terraform apply

Important Factoids

We upgraded from github provider 3.0.0 to 4.13.0 a while ago and recently noticed that this configuration behaves differently.
In 3.0.0 setting strict = true in required_status_checks would enable both "Require status checks to pass before merging" and "Require branches to be up to date before merging".
In 4.13.0 it only enables "Require branches to be up to date before merging", which is weird because that option in the UI is hidden behind "Require status checks to pass before merging".

Note that the intention here is to require all status checks to pass before merging. Passing an empty contexts = [] in the required_status_checks block does not seem to help and we're applying these branch protection rules on multiple repositories where we don't know the context names.

[ShaunEgan]

Can confirm, I am experiencing the same issue with the 4.13.0 version.

[martinmigasiewicz-tomtom]

I am facing this problem as well on 4.14 and realized that it works if I go back to github_branch_protection_v3.

[kfcampbell]

I've confirmed this behavior. Using the above HCL produces a weird state in the GitHub UI where status checks aren't required before merging:

But upon clicking that checkbox, the "up to date" checkbox appears, already checked:

which is not the usual behavior of the checkbox when purely operating through the GitHub API.

Upon further debugging, the issue happens because ok is not returning true in this line of code. I am still unsure of what's causing it to be set that way. I've determined that it's not related to an Update call, so something weird is happening on the initial creation.

This definitely requires more investigation.

[kfcampbell]

Okay! I'm back with another update on this issue. It turns out that the reason that check isn't enabled is because of the statusChecksDiffSuppression function that was created in #614 to fix #572.

I'm playing around with some solutions in #954 to see if I can fix this without causing a regression of #572.

Incidentally, I found it helpful to set up a debugger to figure out what was going on here, so I've opened #955 to document that in hopes it'll be useful for others in the future.

[kfcampbell]

Upon further thought, I might it might be best to remove the diff suppression function wholesale (with other minor code changes).

The current behavior is incorrect, and I am unable to produce the bug in #572, which appears to be a constant spurious diff.

When strict status checks are set to true, the second apply (making no changes after the initial terraform apply) looks like this:

Subsequent applies look like this:

When creating branch protection rules with strict status checks, the second apply look like this:

And subsequent ones look like this:

All applies appear correct in fuctionality, in contract to the first system. I've updated #954 to remove the diff suppresion func entirely, and I'll post these findings and mention the original authors there to see if they have comments.

[kfcampbell]

I've done more extensive testing (documented here) and the fix is ready for review!

[github-actions]

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

[kfcampbell]

Closing this issue as #954 has been merged already.