fix(branch_protection_v3): send each required status check only once - #3585
fix(branch_protection_v3): send each required status check only once#3585dekokun wants to merge 3 commits into
Conversation
The branch protection API returns every required check under both `contexts` and `checks`, so a read populates both fields even though configuration can only set one of them. Updating the resource then sent each check twice and GitHub rejected the request with "Context must be unique per branch protection". Deduplicate by context when building the request, keeping an app_id that came from `checks`. A check read back with `app_id: null` allows any app, which is -1 on write, so set that explicitly rather than omitting app_id (omitting it lets GitHub pick an app).
The existing no-churn tests only re-plan the same config, so they never exercise an update on a resource whose state holds both `contexts` and `checks`. Add a case that changes an unrelated setting after the first apply, for both status check fields.
|
👋 Hi, and thank you for this contribution! This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can. You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions. 🤖 This is an automated message. |
There was a problem hiding this comment.
🟡 Not ready to approve
The new acceptance step must use the repository-required ConfigStateChecks API.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
These provider review instructions are being used.
Fixes duplicate required-status-check contexts in branch protection updates.
Changes:
- Deduplicates checks by context while preserving
app_id. - Adds unit and acceptance regression coverage.
File summaries
| File | Description |
|---|---|
github/resource_github_branch_protection_v3_utils.go |
Deduplicates expanded status checks. |
github/resource_github_branch_protection_v3_utils_test.go |
Tests expansion and deduplication. |
github/resource_github_branch_protection_v3_test.go |
Tests updates after status-check reads. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| Check: resource.ComposeAggregateTestCheckFunc( | ||
| resource.TestCheckResourceAttr( | ||
| "github_branch_protection_v3.test", "enforce_admins", "true", | ||
| ), | ||
| resource.TestCheckResourceAttr( | ||
| "github_branch_protection_v3.test", | ||
| fmt.Sprintf("required_status_checks.0.%s.#", statusChecksField), "2", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Converted in dd7463c: the step now uses ConfigStateChecks with statecheck.ExpectKnownValue (knownvalue.Bool(true) for enforce_admins and knownvalue.SetSizeExact(2) for the status-check set), with the knownvalue/statecheck/tfjsonpath imports added.
| resource "github_repository" "test" { | ||
| name = "%s" | ||
| auto_init = true | ||
| } |
There was a problem hiding this comment.
Please use mustCreateTestRepository instead
There was a problem hiding this comment.
Done in dd7463c — switched to mustCreateTestRepository(t) and dropped the inline github_repository resource from the config.
| } | ||
|
|
||
| resource.Test(t, resource.TestCase{ | ||
| PreCheck: func() { skipUnlessHasOrgs(t) }, |
There was a problem hiding this comment.
Does branch protection rules need an Org?
There was a problem hiding this comment.
No — branch protection works on user-owned repos too, so I copied skipUnlessHasOrgs from the neighbouring test without thinking it through. Relaxed it to skipUnauthenticated(t) in dd7463c, matching what the github_branch_protection (v4) tests do.
… test - use mustCreateTestRepository instead of an inline github_repository resource - relax the precheck to skipUnauthenticated, since branch protection does not require an organization - express assertions with ConfigStateChecks/statecheck instead of the legacy Check/TestCheckResourceAttr API
|
Thanks for the review! Pushed dd7463c addressing all three points:
@deiga ready for another look when you have a moment. |
|
@deiga gentle ping on this one — the three review comments were addressed in dd7463c a week ago and CI is green. Happy to make further changes if anything still looks off. For context on why this might be worth prioritising: with Also happy to rebase onto |
Resolves #3420
Before the change?
Updating a
github_branch_protection_v3resource that has non-emptyrequired_status_checksfails:The branch protection API returns every required check under both
contextsandchecks, soflattenAndSetRequiredStatusChecksputs the same set into both state fields. Both areOptional + Computed, so the field that is not configured keeps its state value forever, andexpandRequiredStatusChecksmerges both fields into onechecksarray — sending each context twice.Configuration cannot hit this on its own (
checksconflicts withcontexts); the duplicate always comes from state.importis one way to get there (as in #3420), but so is a plainapply:Createonly sees the configured field and succeeds, and the read that follows it populates both. From then on any update to that resource fails, even when the change has nothing to do with status checks — I hit this in production on a change that only flippedrequire_code_owner_reviews, on resources this provider had created itself.Note on #2232, which proposed a similar fix and was closed as unnecessary because both fields would be made
Computed: true: both fields already areOptional + Computedin v6.13.0 and the bug is still there. The schema is not what produces the duplicate — the read path writing both fields plus the write path merging them is.After the change?
Requests carry each context once. When a context is present in both fields, an
app_idthat came fromcheckswins, sincecontextscannot express one.A check read back as
app_id: nullallows any app, which is-1on write, so the deduplicated check keepsapp_id: -1instead of omitting the field — omitting it makes GitHub select the app that recently provided the check, which would silently pin an app that was not pinned before.Tests: a unit test on
expandRequiredStatusChecksfor the state shapes above, and an acceptance case that updates an unrelated setting after the first apply (the existing no-churn tests only re-plan the same config, so they never perform an update).I ran the acceptance case against a real organization in
organizationmode. With only the fix reverted it fails at step 2 with the 422 above, for both thecontextsand thechecksvariant; with the fix in place both pass.make test,golangci-lint run ./...andmake lintcheck-neware clean.Per the AI use policy: the patch and this description were drafted with an AI coding assistant. I reviewed the change and ran every test and lint run described above myself.
Pull request checklist
Does this introduce a breaking change?