Allow to set required_approving_review_count=0

[jtyr]

When using this branch protection settings:

[X] Require a pull request before merging
    [ ] Require approvals
    [X] Dismiss stale pull request approvals when new commits are pushed
    [ ] Require review from Code Owners
    [ ] Restrict who can dismiss pull request reviews

The following Terraform code is trying to change the required_approving_review_count value to 1:

resource "github_branch_protection" "main" {
  repository_id = github_repository.this.name
  pattern       = data.github_repository.this.default_branch

  required_pull_request_reviews {
    dismiss_stale_reviews = true
  }
}

This PR allows to set the required_approving_review_count to 0 as that seems to be the default value if Require a pull request before merging is checked.

[majormoses]

While I agree with the technical correctness I think we want to do this in a major release as it breaks existing expectations for anyone using it. I think I will likely need to update some internal modules within my org.

[jspiro]

Simply remove the default and this can be merged as a minor release. I actually disagree with changing the default–no reviews should be an intentional setting of zero, and maintains backwards-compatibility.

[jtyr]

@jspiro Do you want me to change the Default back to 1 or remove the Default completely?

[jspiro]

Well, I'm not a maintainer, but I've been using this for a few years and was a contributor at least once :-)

My intuition says removing the default is actually the best course of action, I like that–the count should be intentional IMO. It doesn't change previous behavior by accident, it'll result in errors where missing that are easy to fix.

BUT: To get this merged ASAP, leaving the previous default but allowing it to be set to zero may satisfy the maintainers releasing it as a minor release, since it only adds new functionality. If it was set to zero before it would have failed validation.

[jtyr]

I have reverted the Default value back to 1.

[jspiro]

I think we should merge this!

(It occurs to me we may need one test to show it can be set to zero, depending whether we have tests for this functionality already. It's NBD to me but might be to maintainers.)

[cooperwalbrun]

Is this still targeted to be merged for the v5.0.0 milestone? I recently ran into the issue mentioned in #294 and am curious what the resolution was regarding the major/minor discussion above, since it seems this change will also address #294. I agree with @jspiro that this would be nice to have as soon as possible, especially if it is non-breaking (per @jtyr's change to the default).

[jcudit]

Looks good for a minor release, thanks for all the discussion that went into adjusting this. Aiming to get this released ahead of v5 🙇🏾

[majormoses]

LGTM

Thanks for fixing this quirky behavior.

[k24dizzle]

oops I made my own PR to fix this here before realizing this PR existed: #1042

I added a couple of unit tests to test this new behavior, but looking forward for either one of these PRs to get merged in.