WIP: Migrate include_admins to enforce_admins in Github API

[jonathanio]

There's been a breaking change in the GitHub API in the last day or so which changes the format of the request/response for the Branches Protection API call. The options for include_admins on each of the settings has been removed, and an all-encompassing enforce_admins has been provided for setting at the branch level.

Also, the API supports control over who can dismiss pull request reviews and management of stale reviews.

I've updated the code, tests, and documentation with the new details, and run some Acceptance Tests for Github. I've had some issues with this whereby users are getting removed from the organisation on the test, and it's not repeatable, so I'm not 100% confident in the results. For example:

--- FAIL: TestAccGithubBranchProtection_basic (1.50s)
        testing.go:280: Step 0 error: Check failed: Check 3/12 error: Expected Restrictions.Users to be [some_user], got []
=== RUN   TestAccGithubBranchProtection_importBasic
--- FAIL: TestAccGithubBranchProtection_importBasic (1.05s)
        testing.go:280: Step 0 error: After applying this step, the plan was not empty:

                DIFF:

                UPDATE: github_branch_protection.master
                  required_pull_request_reviews.0.dismissal_restrictions.#: "1" => "0"
                  restrictions.0.users.#:                                   "0" => "1"
                  restrictions.0.users.0:                                   "" => "some_user"

                STATE:

                github_branch_protection.master:
                  ID = test-repo:master
                  branch = master
                  enforce_admins = true
                  repository = test-repo
                  required_pull_request_reviews.# = 1
                  required_pull_request_reviews.0.dismiss_stale_reviews = false
                  required_pull_request_reviews.0.dismissal_restrictions.# = 1
                  required_pull_request_reviews.0.dismissal_restrictions.0.teams.# = 0
                  required_pull_request_reviews.0.dismissal_restrictions.0.users.# = 0
                  required_status_checks.# = 1
                  required_status_checks.0.contexts.# = 1
                  required_status_checks.0.contexts.0 = github/foo
                  required_status_checks.0.strict = true
                  restrictions.# = 1
                  restrictions.0.teams.# = 0
                  restrictions.0.users.# = 0

But some_user cannot be added back in as they are no longer part of the Organisation. The requests and response processing do at least seem to work as expected.

TODO:

• Improve handling of the default case for dismissal_restrictions so that the default generated by resourceGithubBranchProtection matches the "empty" value returned by the API response.
• See what additional tests can be added around enforce_admins and the new dismiss* options.
• Sync vendor/ updates with upstream repositories.
• Add a possible note in the documentation this is linked with a preview API call and there is a risk of further breaking changes until release?

I will aim to add some of these in the coming days, but I welcome reviews on my code so far (bit rusty in Golang) and make sure I'm on the right track for Terraform.

[jonathanio]

It looks like some of the upstream changes have already been created and merged (google/go-github#617) Somehow missed that. That'll need to be synced here and the relevant code refactored before progressing.

[netflash]

Any chances this gonna be implemented soon ?

[mechastorm]

Any chance this can be updated and merged? This is breaking any integration between Terraform and Github for protecting branches. We had to ditch using Terraform to manage Github repos for now until this can be fixed as this was a blocker to our applies.

[grubernaut]

Hey @jonathanio, thank you for your awesome work on this! This fix has now been implemented in https://github.com/terraform-providers/terraform-provider-github/pull/26. Going to close this, but happy to discuss anything further.
Much apologies for not reviewing this sooner as well. With the provider-split we should be able to get to reviewing PR's like these in a much faster manner. Thanks again!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.