Attempted fix for #1657

[yaakov-h]

Resolves #1657

---

Behavior

Given the following resource:

resource "github_branch_protection_v3" "myrepo-master" {
  repository     = "MyRepo"
  branch         = "master"

  # irrelevant fields omitted

  required_status_checks {
    checks = [
      # format: "<name>:<GitHub App ID>"
      "MyStatusCheck:123456"
    ]
  }
}

Before the change?

• Generating the plan would always show too many checks
• If a GitHub App ID was specified it would not be seen as correct:

  ~ resource "github_branch_protection_v3" "myrepo-master" {
        id                              = "MyRepo:master"
        # (6 unchanged attributes hidden)

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

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

After the change?

• Status checks with GitHub App IDs are seen as valid and do not get recreated.
• There are no extra "checks" elements
• There are extra "contexts" elements, but this is due to server behaviour - see below.

  ~ resource "github_branch_protection_v3" "myrepo-master" {
        id                              = "MyRepo:master"
        # (6 unchanged attributes hidden)

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

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

The GitHub API returns the contexts value, so users will either need to include this deprecated field in their HCL or set ignore_changes for required_status_checks.contexts, and then ignore the deprecation warning that the field they are ignoring is deprecated.

Other information

I'm not very experienced in Go so this can probably be done better.

I've also ignored the AppID entirely if it is omitted from JSON or is a JSON null, which can happen if a user changes the branch protection policy from requiring a specific app to allowing any GitHub app.

This will probably need some unit tests added too, though #1415 didn't have any when it added checks in the first place.

---

Additional info

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes (Please add the Type: Breaking change label)
• No

If Yes, what's the impact:

• N/A

Pull request type

Please add the corresponding label for change this PR introduces:

• Bugfix: Type: Bug
• Feature/model/API additions: Type: Feature
• Updates to docs or samples: Type: Documentation
• Dependencies/code cleanup: Type: Maintenance

---

[kfcampbell]

@yaakov-h can you describe how you've tested this manually so I can replicate it? Thank you for contributing!

[domsleee]

I believe this is a valid repro: #1657 (comment)

The current behaviour is that this line is printing a pointer to the app ID as an integer:
> checks = append(contexts, fmt.Sprintf("%s:%d", chk.Context, chk.AppID))

From the end-user perspective, the App ID is "changing all the time" described in the issue. Because we are printing the pointer and not the actual value. The suggested fix here looks reasonable to me 👍

[yaakov-h]

@kfcampbell I used the existing config I was working on (the resource I posted at the top of the PR) and used Terraform dev_overrides to use a custom build with these changes. Running terraform plan produced the output shown in "After the change".

[pgressa]

@kfcampbell is it possible to validate this one and release new version of the provider? We're migrating to v5 provider given the hundreds of the repositories we have it makes the plan pretty unusable

[entropitor]

The GitHub API returns the contexts value, so users will either need to include this deprecated field in their HCL or set ignore_changes for required_status_checks.contexts, and then ignore the deprecation warning that the field they are ignoring is deprecated.

@yaakov-h The problem is that if you set both contexts and checks it's trying to apply both and that fails as you can only specify one. Should we instead only look at checks if both are set while applying? The same is true for ignore_changes, even when setting contexts to [] given the server values is fetched upon refresh...

[yaakov-h]

@entropitor I think if you set both then that would be consumer error, but tbh I'd be in favour of removing contexts entirely as mentioned in #1701 as that also gets rid of the problem of deprecation warnings.