fix: detect out-of-band removal of environment reviewers on refresh - #3616
fix: detect out-of-band removal of environment reviewers on refresh#3616nitinjain999 wants to merge 2 commits into
Conversation
reviewers and prevent_self_review were only written to state inside the required_reviewers case of the protection_rules loop in resourceGithubRepositoryEnvironmentRead. Once the last reviewer is removed outside Terraform, the API stops returning a required_reviewers entry, so that case never runs and the stale state value survives the refresh. Reset both fields before the loop, the way wait_timer already is. Fixes integrations#3609
|
👋 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. |
|
This PR is functionally ready for review — it's still marked as a draft only because this account is currently at the repo's open-PR limit (#3464, #3462 also open). Happy to flip it to "Ready for review" as soon as a slot frees up, or a maintainer is welcome to do so directly. @deiga @stevehipwell |
|
I'll undraft this and draft the others |
…ingle pass Read collected protection rule attributes only inside the ProtectionRules loop, so a rule removed outside Terraform left the prior state untouched. Rather than pre-setting the attributes to empty, wait_timer, reviewers and prevent_self_review are now derived in one pass over the response and set unconditionally, matching how deployment_branch_policy is already handled. Replaces the httptest-based unit test with a detects_out_of_band_reviewer_removal acceptance subtest that removes the required_reviewers rule via the API and asserts refresh produces a non-empty plan.
| if err = d.Set("wait_timer", pr.WaitTimer); err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| waitTimer = pr.WaitTimer |
There was a problem hiding this comment.
If there is a GetWaitTimer funcrion, please use that
| if err = d.Set("prevent_self_review", pr.PreventSelfReview); err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| preventSelfReview = pr.PreventSelfReview |
There was a problem hiding this comment.
Use a getter function if available
| if err := d.Set("prevent_self_review", preventSelfReview); err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| if err := d.Set("can_admins_bypass", env.CanAdminsBypass); err != nil { |
There was a problem hiding this comment.
Use a getter function if available
| resource "github_team" "test" { | ||
| name = "%[1]s" | ||
| description = "test" | ||
| privacy = "closed" | ||
| } | ||
|
|
||
| resource "github_repository" "test" { | ||
| name = "%[1]s" | ||
| visibility = "public" | ||
| } | ||
|
|
||
| resource "github_team_repository" "test" { | ||
| team_id = github_team.test.id | ||
| repository = github_repository.test.name | ||
| permission = "pull" | ||
| } |
There was a problem hiding this comment.
Use the mustCreateTest functions to setup test resources
| t.Errorf("failed to remove environment reviewers out-of-band: %s", err) | ||
| } | ||
| }, | ||
| RefreshState: true, |
There was a problem hiding this comment.
This step seems to be missing the Config attribute
There was a problem hiding this comment.
🟡 Changes recommended
The regression test must directly verify that both reviewers and prevent_self_review are cleared during refresh.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes stale Terraform state when environment reviewers are removed outside Terraform.
Changes:
- Resets absent reviewer protection fields during refresh.
- Adds regression coverage for reviewer-removal drift.
File summaries
| File | Review |
|---|---|
github/resource_github_repository_environment.go |
Resets protection-rule state, but the test does not directly verify prevent_self_review refreshes to false. |
github/resource_github_repository_environment_test.go |
Adds an acceptance test, but its plan assertion does not prove both fields were cleared and differs from the described mocked unit test. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if err := d.Set("reviewers", reviewers); err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| if err := d.Set("prevent_self_review", preventSelfReview); err != nil { |
| RefreshPlanChecks: resource.RefreshPlanChecks{ | ||
| PostRefresh: []plancheck.PlanCheck{ | ||
| plancheck.ExpectResourceAction("github_repository_environment.test", plancheck.ResourceActionUpdate), | ||
| }, |
Summary
Fixes #3609.
reviewersandprevent_self_reviewinresourceGithubRepositoryEnvironmentReadwere only ever written to state insidecase "required_reviewers":of theprotection_rulesloop. Once the last required reviewer is removed outside Terraform, the GitHub API stops returning arequired_reviewersentry inprotection_rulesat all, so that case never runs,d.Setis never called for either field, and the prior state value silently survives the refresh.terraform planthen reportsNo changeseven though the reviewer is gone.wait_timeralready avoids this exact problem by being reset tonilbefore the loop runs. This PR applies the same pattern toreviewersandprevent_self_review.Changes
reviewersto[]any{}andprevent_self_reviewtofalsebefore theprotection_rulesloop ingithub/resource_github_repository_environment.go, so a rule missing from the API response is correctly treated as absent.github/resource_github_repository_environment_unit_test.go) that mocks aGetEnvironmentresponse withprotection_rules: [](the exact shape GitHub returns once the last reviewer is removed, per the debug logs in the issue) against a prior state that has reviewers set, and asserts both fields refresh to empty/false.Test plan
go build ./...,go vet ./github/...,gofmt -lall clean.go test ./github/... -run '^Test[^A]|^TestA[^c]|^TestAc[^c]').