feature/985 branch protection checks

[TheQueenIsDead]

Resolves:

• Add support for configuring checks to branch protection #985
• unable to update branch_protections for github repos #1147
• Cannot create github_branch_protection_v3 after update to v5.3.0 #1307
• [Feature Request] Add support for branch protection required_status_check 'checks' object #1212

---

Behavior

Before the change?

• Checks were not able to be set
• Setting required status checks on branch protection resources was broken

After the change?

• Checks are able to be set via a ":" separated string
• Contexts are now passed to the API as checks and users can provision branch protections again

Other information

This should render the ability to define checks in configuration such as this:

resource "github_branch_protection_v3" "bp" {

  repository = "terraform-template-module"
  branch     = "master"

  required_status_checks  {
    strict = true

    contexts = [
      "context1",
      "context2",
      "context3",
      "context4",
    ]

    checks = [
      "negative_one:-1",
      "undefined_comma:",
      "undefined",
      "zero_check:0",
      "app_id_context:1234567",
    ]
  }
}

---

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

---

[TheQueenIsDead]

/label Type: Feature

[TheQueenIsDead]

This works in the sense that is creates and deletes branch protections 👍

However, there is something interesting happening in the diff, like so:

 # github_branch_protection_v3.bp will be updated in-place
  ~ resource "github_branch_protection_v3" "bp" {
        id                              = "terraform-template-module:master"
        # (6 unchanged attributes hidden)

      ~ required_status_checks {
          ~ contexts       = [
              - "example",
              - "newcheck",
              - "noappidcontext",
            ]
            # (2 unchanged attributes hidden)

          ~ check {
              ~ app_id  = -1 -> 1
                # (1 unchanged attribute hidden)
            }
          - check {
              - app_id  = -1 -> null
              - context = "newcheck" -> null
            }

            # (1 unchanged block hidden)
        }
    }

This seems to suggest that despite not including a contexts array, they will become updated via the inclusion of check blocks.

Not too sure how to handle this currently, but will see what I can come up with.

[cfisher281]

Appreciate you doing this work @TheQueenIsDead.

[TheQueenIsDead]

@kfcampbell , small question from me. I've added the deprecated flag to the contexts attribute, and the process for carrying out this deprecation is usually to release a minor with the flag, and then remove the code in the next major release version. This is documented by Hashicorp here:
https://developer.hashicorp.com/terraform/plugin/sdkv2/best-practices/deprecations

It makes a lot of sense, but given the change to the Github go client, I cannot implement this functionality nicely without removing part of the code that flattens the context array when it reads from the API. Without that removal, subsequent applies will show the contexts array being blitzed on each plan, despite it not being defined in Terraform:

 ~ contexts       = [
              - "context_app_id_allow_all",
              - "context_no_app_id_most_recent_check",
              - "context_with_app_id_defined",
            ]

The question is, would you accept the removal of contexts in this PR (Jarring as on release it would give little time to warn users of deprecation), or is there perhaps another route to releasing this that would be preferred?

As it stands, it is not possible to only send contexts with the SDK :-( So these are interesting circumstances.

[kfcampbell]

Hmm...none of those options are very desirable.

I'm looking at the API reference and I see the following under required_status_checks:

Deprecated: The list of status checks to require in order to merge into this branch. If any of these checks have recently been set by a particular GitHub App, they will be required to come from that app in future for the branch to merge. Use checks instead of contexts for more fine-grained control.

Perhaps some of that behavior is due to the API changing to deprecate contexts as well.

The question is, would you accept the removal of contexts in this PR (Jarring as on release it would give little time to warn users of deprecation), or is there perhaps another route to releasing this that would be preferred?

For a breaking change like that, we would end up cutting a v6 version, which ideally we should not do that in this PR without a deprecation notice.

Do you mind explaining a little bit more about this description?

given the change to the Github go client, I cannot implement this functionality nicely without removing part of the code that flattens the context array when it reads from the API. Without that removal, subsequent applies will show the contexts array being blitzed on each plan, despite it not being defined in Terraform:

Is there a way we can handle contexts when they're present in Terraform by warning of deprecation and then formatting them into the new checks array of objects?

[TheQueenIsDead]

Perhaps some of that behavior is due to the API changing to deprecate contexts as well.

I wouldn't be suprised! As identified in a previous issue, the SDK "fully deprecated" contexts as part of this PR, while I believe the API itself still supports it momentarily:
google/go-github#2467 (comment)

For a breaking change like that, we would end up cutting a v6 version, which ideally we should not do that in this PR without a deprecation notice.

I appreciate that, and am happy to help go down that route where possible 👍 However, it does feel a little ceremonious given contexts on the SDK are broken due to the above PR.

Do you mind explaining a little bit more about this description?

Certainly! What I am trying to highlight is that the Github API seems to return a list of contexts even when only checks are provisioned. When this gets converted to TF schema, it assumes that on next apply the contexts array will be deleted, because it is not defined in our file.

I think you're right that we could simply convert contexts to checks (With a default of allow all apps, ie -1). We could possibly keep the schema and not try and map the response to a contexts array in that instance.
That feels a little dirty, but it could be a good midpoint to allowing users to keep doing what they're doing, and give them a little notice about moving to checks.

I'll have a play around :-)

[TheQueenIsDead]

Hmm, super interesting. I have made a change to flatten and expand all contexts into checks, using the default -1 app_id.

On initial apply, everything looks ok. When a second apply is ran, the change output looks really messy. On the third apply in a row (With no changes), Terraform detects no changes.

I would have expected it to detect no changes the first time around, but I am unsure why it creates a plan like this. Full example output can be seen in this gist:
https://gist.github.com/TheQueenIsDead/ea838b56322f4aa998cd1b84f1129cbe

Changes can be seen in the most recent commit: 567856a
It's all functional, just not performing the diff very well...

[TheQueenIsDead]

I have tried sorting the contexts and checks array alphabetically, but this did not resolve the weird diff. I feel that this may be due to the way Terraform does the diff (Which I am not too familiar with under the hood), but I am guessing it reconciles diffs based on it's own schema, where it has one schema based on current state, and another based on what the file definition says.

I also tried experimenting with ConfigMode: schema.SchemaConfigModeAttr, which did not seem to help.

I'm wondering if the 'advanced data structure' as an array is causing issues, I imagine if we used a map of strings ([]string{"context_name:-1"}) then we would get a better diff, though this is inflexible for representing new attributes that may be added to the API in the future.

[TheQueenIsDead]

@kfcampbell , I have changed the approach from using resource blocks in the schema to instead using a list of : delimited strings. This yielded a much better diff, and has the added bonus that an end user can simply rename the contexts array and have a working config.

Will update the docs soon, not terribly sure about how to approach testing.

[kfcampbell]

@TheQueenIsDead thank you for all your effort and clear communication! I really appreciate it.

is there any chance that this has broken the integration tests on main for the GithubBranchProtectionV3 tests?

Definitely, and there's also a chance that they haven't been exactly healthy for a long time.

does this require a issue to be raised?

Ideally, yes. We have a significant amount of testing debt to pay back on this project. I've opened #1414 to begin this effort, but it would be great to have an issue for this specific testing suite.

Given the above and the substantial number of issues ready to be closed, I think it would be reasonable to merge this PR before full testing support is achieved.

@TheQueenIsDead do you believe this is ready for final review/merging?

[kfcampbell]

Hi! I'm swinging by to check on this PR again as I'm really looking forward to having it in. @TheQueenIsDead do you mind giving these questions a look when you get a chance?

[TheQueenIsDead]

Ok, so I have created a new bug report for the unmarshalling error that is causing the tests to fail for this branch #1476.

Given the above and the substantial number of issues ready to be closed, I think it would be reasonable to merge this PR before full testing support is achieved. @TheQueenIsDead do you believe this is ready for final review/merging?

In a perfect world I think the recent go-github provider breakage would be reverted so that the tests can pass. As mentioned in the issue, it seems to have broken unmarshalling branch protection objects, so even if this PR is merged and released, users still wont be able to benefit from it!

I'm happy with the changes proposed here, but I am also the author, hence the most biased party ;-) The lack of tests are the only thing making me wary however.
The diff was a little confusing when running an apply to get the contexts to be written as checks, but subsequent runs yeilded a decent diff, which I am happy about given my limited experience with programming and the Terraform SDK

[kfcampbell]

I have created a new bug report for the unmarshalling error that is causing the tests to fail for this branch #1476.

This should be fixed. I believe the problem was due to this upstream issue, fixed in this google/go-github PR, released here, and added to the project as part of #1473.

On the main branch, tests for this resource are failing for me as well 😭 but not for unmarshaling reasons.

it seems to have broken unmarshalling branch protection objects, so even if this PR is merged and released, users still wont be able to benefit from it!

I don't believe this is the case anymore! 🎉

The lack of tests are the only thing making me wary

Absolutely, I agree. However, given the resource's impaired state currently and the number of issues/engagement about it, it may be appropriate to merge this PR earlier as a mitigation and add/modify earlier content afterward. Issues saying "confusing diff" are generally better than issues saying "doesn't work" IMO.

[ilmax]

I also bumped into this one, looking forward to seeing this merged :)

[TheQueenIsDead]

Apologies for the delay, I've been struggling to find time to commit to other projects.

On the main branch, tests for this resource are failing for me as well sob but not for marshalling reasons.

I'll have a quick look at the tests to see if I can identify the issue and resolve it here, otherwise I'm all for merging this to give a path forwards, with room to refine later on. Pragmatism 😊

[TheQueenIsDead]

It looks like the TestAccGithubBranchProtectionV3* tests are all failing due to the fact that after an apply, there are changes present in the plan.

configures_branch_push_protections, defaults, required_pull_request_reviews, and configures_default_settings_when_empty fail as some of the security and analysis settings want to be set to empty strings (Incorrect defaults?)

          security_and_analysis.#:                                          "1" => "0"
          security_and_analysis.0.advanced_security.#:                      "1" => ""
          security_and_analysis.0.advanced_security.0.status:               "" => ""
          security_and_analysis.0.secret_scanning.#:                        "1" => ""
          security_and_analysis.0.secret_scanning.0.status:                 "disabled" => ""
          security_and_analysis.0.secret_scanning_push_protection.#:        "1" => ""
          security_and_analysis.0.secret_scanning_push_protection.0.status: "disabled" => ""

required_status_checks fails due to the following diff, which I am uncertain about

        UPDATE: github_branch_protection_v3.test
          branch:                                  "main" => "main"
          enforce_admins:                          "false" => "false"
          etag:                                    "W/\"523f6d38db8bde2e6366e192aceef11125983f1d19d6faea81d27d4107d5aade\"" => "W/\"523f6d38db8bde2e6366e192aceef11125983f1d19d6faea81d27d4107d5aade\""
          id:                                      "tf-acc-test-sfz48:main" => "tf-acc-test-sfz48:main"
          repository:                              "tf-acc-test-sfz48" => "tf-acc-test-sfz48"
          require_conversation_resolution:         "false" => "false"
          require_signed_commits:                  "false" => "false"
          required_pull_request_reviews.#:         "0" => "0"
          required_status_checks.#:                "1" => "1"
          required_status_checks.0.checks.#:       "1" => "1"
          required_status_checks.0.checks.0:       "github/foo" => "github/foo"
          required_status_checks.0.contexts.#:     "1" => "0"
          required_status_checks.0.contexts.0:     "github/foo" => ""
          required_status_checks.0.include_admins: "false" => "false"
          required_status_checks.0.strict:         "true" => "true"
          restrictions.#:                          "0" => "0"

I tested the branch protection creation against my own repo and it still works with the new checks object. As mentioned it has a slightly ambiguous plan for the second apply, but everything still works just fine thereafter (It seems to be a little confused about contexts vs checks initially. Github does interesting things on the backend there though, maintaining both attributes....This wont be an issue when checks is dropped).

@kfcampbell I'd be happy to have this merged, and if anyone can help improve the apply flow in future, then it'd be greatly appreciated

Apologies that it's not perfect, first time developing Terraform 👍

[kfcampbell]

@TheQueenIsDead thank you for all the work here!

I realized I merged #1489; does that affect this PR/testing scenarios at all? If you're comfortable with the current state after #1489, I'll approve/merge this and cut a release soon.

[TheQueenIsDead]

Awesome, that resolves all the issues that aren't to do with required_status_checks!
The only issue with this PR is that it takes one more apply before it is happy reconciling the diffs on future runs.

I'm happy to have this merged with the goal of improving it later 👍

[kfcampbell]

Thanks for all the work on this, @TheQueenIsDead! I'm looking forward to having this in.

[rpdelaney]

Any guess when we might get this in a release? :)

[mbarany]

what if my status check context includes a colon?
Our CircleCi jobs for example ci/circleci: unit_test produces the following error:

Error: Could not parse unit_test:-1 as valid app_id

[kfcampbell]

@mbarany do you mind creating an issue for that?