[BUG]: github_branch_protection_v3.required-status-checks.checks - app_id changes all the time

[bahag-klickst]

Describe the need

Hej all,

We are in the middle of moving from the hashicorp/github provider to integrations/github provider and are also doing a major jump from v4.xx to latest 5.xx (as of writing/testing it was 5.22).

We are now facing an issue due to changing from contexts to checks in the required-status-checks block, because the new structure of the status checks is "context:app_id", but app_id changes with every run of our pipeline?

This is of course a bad thing, because it would lead to "unnecessary" changes with every terraform plan/apply.

SDK Version

No response

API Version

No response

Relevant log output

Result of first plan:
---------------------
~ resource "github_branch_protection_v3" "main" {
        id                              = "repo-name:main"
        # (6 unchanged attributes hidden)

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

        # (1 unchanged block hidden)
    }

Result of second plan, without any changes in between:
------------------------------------------------------
  ~ resource "github_branch_protection_v3" "main" {
        id                              = "repo-name:main"
        # (6 unchanged attributes hidden)

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

        # (1 unchanged block hidden)
    }

As one can see, the app_id changed from 824642352016 to 824635284400
Another plan then delivered 824642245200.

So the app_id is kind of generic and this makes it hard for us to use this new version.

Code of Conduct

• I agree to follow this project's Code of Conduct

[kfcampbell]

@bahag-klickst can you provide an example of HCL that can be used to reproduce this issue?

[bahag-klickst]

@kfcampbell , sure but I am afraid that I have to explain a bit what we are doing exactly.
We are a central cloud platform team responsible for e.g GitHub and decided to provide a self written github repository module to allow our endusers to create their repos themselves by calling our module and passing the according variables to it.

Here is an example Call for one of the repos giving us a hard time right now, due to the mentioned issue:

variable "repositories" {}

repositories = {
ops-terraformmodule = {
description = "maintain hub-terraformmodule-prod"
branch_protection = {
main = {
enforce_admins = true
required_approving_review_count = "0"
require_code_owner_reviews = false
strict_status_checks = true
contexts = ["Terraform"]
dismiss_stale_reviews = true
}
}
}
}

module "github_repositories" {
for_each = var.repositories
source = "../module"
repository_name = each.key
description = try(each.value.description, "default description")
team_permissions = try(each.value.team_permissions, {})
visibility = try(each.value.visibility, "internal")
user_permissions = try(each.value.user_permissions, {})
branch_protection = try(each.value.branch_protection, {})
auto_init = try(each.value.auto_init, true)
default_branch = try(each.value.default_branch, "")
delete_branch_on_merge = try(each.value.delete_branch_on_merge, false)
team_short_name = ["ops"]
branches = try(each.value.branches, [])
vulnerability_alerts = try(each.value.vulnerability_alerts, false)
}

The simplified module code looked like this, using the hashicorp/github version 4.xx:

resource "github_repository" "repository" {
lifecycle {
ignore_changes = [
is_template, has_issues, gitignore_template, pages
]
precondition {
condition = contains(var.team_short_name, split("-", var.repository_name)[0]) == true
error_message = "Your repo name does not match the defined naming convention"
}
}
name = var.repository_name
description = var.description
visibility = var.visibility
auto_init = var.auto_init
is_template = false
gitignore_template = ""
allow_merge_commit = true
allow_rebase_merge = false
allow_squash_merge = false
allow_auto_merge = false
delete_branch_on_merge = var.delete_branch_on_merge
has_issues = true
has_projects = true
has_wiki = true
vulnerability_alerts = var.vulnerability_alerts
}

resource "github_branch_protection_v3" "main" {
depends_on = [github_repository.repository]
for_each = var.branch_protection
repository = var.repository_name
branch = each.key
enforce_admins = each.value.enforce_admins
dynamic "required_status_checks" {
for_each = { for k, v in var.branch_protection : k => v if each.value.strict_status_checks && length(each.value.contexts) != 0 && k == each.key }
content {
strict = each.value.strict_status_checks
contexts = each.value.contexts
}
}
required_pull_request_reviews {
required_approving_review_count = each.value.required_approving_review_count
require_code_owner_reviews = each.value.require_code_owner_reviews
dismiss_stale_reviews = each.value.dismiss_stale_reviews
}
}

With moving to integrations/github version 5.xx, we just changed the following:

resource "github_branch_protection_v3" "main" {
depends_on = [time_sleep.wait_30_seconds, github_branch.main]
for_each = var.branch_protection
repository = var.repository_name
branch = each.key
enforce_admins = each.value.enforce_admins
dynamic "required_status_checks" {
for_each = { for k, v in var.branch_protection : k => v if each.value.strict_status_checks && length(each.value.contexts) != 0 && k == each.key }
content {
strict = each.value.strict_status_checks
checks = each.value.contexts
}
}
required_pull_request_reviews {
required_approving_review_count = each.value.required_approving_review_count
require_code_owner_reviews = each.value.require_code_owner_reviews
dismiss_stale_reviews = each.value.dismiss_stale_reviews
}
}

As mentioned in the issue description, as checks has a different requirement reagrding the string (https://registry.terraform.io/providers/integrations/github/latest/docs/resources/branch_protection_v3#checks) we tried to fix it by adding the app_id. But unfortunately it changes with every run. We were not able to figure out why this keeps happening.

[mmuth]

@bahag-klickst can you provide an example of HCL that can be used to reproduce this issue?

Hi! We have the very same issue. I created a minimal example (https://github.com/mmuth/github-terraform-provider-repro/blob/main/terraform/main.tf and the HCL: https://github.com/mmuth/github-terraform-provider-repro/blob/main/terraform/.terraform.lock.hcl)

It is always inconsistent, means a successful terraform apply followed by another apply will still try to change the Github repo.
Sample log output of a retry:

# github_branch_protection_v3.master will be updated in-place
  ~ resource "github_branch_protection_v3" "main" {
        id                              = "sample-repo-with-statuschecks:main"
        # (6 unchanged attributes hidden)

      ~ required_status_checks {
          ~ checks         = [
              - "test-libraries",
              - "test-react",
              - "test-server",
              + "test-libraries:***OURAPPID***",
              + "test-react:***OURAPPID***",
              + "test-server:***OURAPPID***",
            ]
            # (3 unchanged attributes hidden)
        }

        # (1 unchanged block hidden)
    }

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

we also tried around with checks like test-libraries:-1, test-libraries and others but failed to ever recieve a repeatable consistent terraform.

New checks can basically be added, but we also noticed that we can never delete an existing check. It is converted to "any source" only but not removed. But this might also by a symptom of the same origin (?)...

[bahag-klickst]

@kfcampbell
Do you have any idea on when someone will grab this issue?

[kfcampbell]

@bahag-klickst unfortunately our team very rarely gets the chance to pick up items like this as part of our daily work. We are very receptive to PRs though!

[GergelyKalmar]

It seems that contexts have been deprecated but the alternative doesn't work at all. The issue with the branch protection resources isn't fixed either (see #670). It seems that this provider is becoming really difficult to use even for basic things like applying proper branch protection.

[yaakov-h]

I encountered the same issue today. Upon checking the raw API response, the app_id is correct, so the issue has to be in client code.

My gut sank when I realised that if the integer value changes every time the app runs... and converting it to hex makes it look quite suspicious...

Anyway I dug out my old Go docs and I'm 99% certain that the bug is here:

terraform-provider-github/github/resource_github_branch_protection_v3_utils.go

Line 63 in f8502b8

checks = append(contexts, fmt.Sprintf("%s:%d", chk.Context, chk.AppID))

The code tries to flatten the API response object ({"context": "foo", "app_id": 123}) into a single string (foo:123).

Unfortunately for this repo, that AppID field is optional, so it is defined in go-github as *int64, i.e. as a pointer. In order to get the actual app_id value, it has to be dereferenced. (Of course, we cannot blindly reference it, because then we can crash when it is JSON null / Golang nil.)

There also seems to be some incorrect copy-pasting going on, as we are appending elemnts to contexts and assigning it back to contexts, but then we are also appending elements to contexts and assigning to back to checks.

These two loops look to me like they have a few problems that cause terraform to re-update the branch_protection_v3 resource on every run, and cleaning them up should fix this Issue as well as other problems:

terraform-provider-github/github/resource_github_branch_protection_v3_utils.go

Lines 53 to 64 in f8502b8

	for _, c := range rsc.Contexts {
	// Parse into contexts
	contexts = append(contexts, c)
	checks = append(contexts, c)
	}
	// Flatten checks
	for _, chk := range rsc.Checks {
	// Parse into contexts
	contexts = append(contexts, chk.Context)
	checks = append(contexts, fmt.Sprintf("%s:%d", chk.Context, chk.AppID))
	}