Feat: Add support for the require_last_push_approval flag in github_branch_protection_v3

[matthewcostatide]

Resolves #1723

---

Before the change?

The github_branch_protection_v3 resource doesn't support setting the require_last_push_approval flag, and its presence is omitted from the documentation.

After the change?

This PR adds support for the require_last_push_approval flag in the required_pull_request_reviews block of the github_branch_protection_v3 resource type, consistent with the github_branch_protection resource.

Pull request checklist

• [x] Tests for the changes have been added (for bug fixes / features)
• [x] Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

It should not

Please see our docs on breaking changes to help!

• [ ] Yes
• [x] No

---

[matthewcostatide]

@kfcampbell any chance you can take a look at this?

[georgekaz]

It would be nice if someone would look at this. Is there any reason it's sitting around being ignored?

[kfcampbell]

Sorry for the delay here. After the addition in TestAccGithubBranchProtectionV3_required_pull_request_reviews, the acceptance tests are failing:

    testing.go:705: Step 0 error: Check failed: 1 error occurred:
        	* Check 5/12 error: github_branch_protection_v3.test: Attribute 'required_pull_request_reviews.0.require_last_push_approval' expected "true", got "false"

@matthewcostatide can you reproduce this? Do the tests succeed for you?

[georgekaz]

@kfcampbell @matthewcostatide I had a look at this and I think what is missing is "require_last_push_approval": rprr.RequireLastPushApproval, from this block: https://github.com/matthewcostatide/terraform-provider-github/blob/branch_protection_v3_require_last_push_approval/github/resource_github_branch_protection_v3_utils.go#L191-L201

		return d.Set("required_pull_request_reviews", []interface{}{
			map[string]interface{}{
				"dismiss_stale_reviews":           rprr.DismissStaleReviews,
				"dismissal_users":                 schema.NewSet(schema.HashString, users),
				"dismissal_teams":                 schema.NewSet(schema.HashString, teams),
				"dismissal_apps":                  schema.NewSet(schema.HashString, apps),
				"require_code_owner_reviews":      rprr.RequireCodeOwnerReviews,
				"require_last_push_approval":      rprr.RequireLastPushApproval,
				"required_approving_review_count": rprr.RequiredApprovingReviewCount,
				"bypass_pull_request_allowances":  bpra,
			},
		})

The tests seem pretty broken to me, they fail to delete the repos they create with a 403 Must have admin rights to Repository. [] error. However, prior to my suggested change I saw the error that @kfcampbell mentions above and after adding this line, that error is resolved.

On another note, this bit of code

requireLastPushApproval := m["require_last_push_approval"].(bool)
rprr.RequireLastPushApproval = &requireLastPushApproval

as opposed to

rprr.DismissStaleReviews = m["dismiss_stale_reviews"].(bool)

seems to be required because of a strange use of a pointer here and I think this accessor code is possibly wrong too. Something to look at outside the scope of this PR.

[kfcampbell]

@georgekaz you're a hero for fighting the good fight with our test situation. Thanks for your research! As always, should you desire to submit any PRs with fixes you'd like to see, I'm happy to review.

[georgekaz]

Yeah, sure I can take a look at the tests in general. On this occasion though the test was correct to fail, the property on the object is not being set. If @matthewcostatide would like to add the fix I suggested above it will pass. I don't really want to push to Matthew's branch, he may not like it!

[georgekaz]

Hi @matthewcostatide could you add the following line please:

"require_last_push_approval": rprr.RequireLastPushApproval,

here: https://github.com/matthewcostatide/terraform-provider-github/blob/branch_protection_v3_require_last_push_approval/github/resource_github_branch_protection_v3_utils.go#L197C6-L197C32

Then I think the tests will pass and this PR has a chance of being merged. Thanks

[aalvarezaph]

I don't really want to push to Matthew's branch, he may not like it!

IMO given this was raised beginning on Feb, seems fair enough to push this fix to the branch . This is a feature many will be able to benefit from :)

[georgekaz]

@aalvarezaph @kfcampbell I couldn't edit @matthewcostatide's branch because I'm not a repo maintainer, so I've had to open a new PR with the patch. https://github.com/integrations/terraform-provider-github/pull/2199