Skip to content

fix: detect out-of-band removal of environment reviewers on refresh - #3616

Draft
nitinjain999 wants to merge 1 commit into
integrations:mainfrom
nitinjain999:fix/3609-environment-reviewer-drift-detection
Draft

fix: detect out-of-band removal of environment reviewers on refresh#3616
nitinjain999 wants to merge 1 commit into
integrations:mainfrom
nitinjain999:fix/3609-environment-reviewer-drift-detection

Conversation

@nitinjain999

Copy link
Copy Markdown
Contributor

Summary

Fixes #3609.

reviewers and prevent_self_review in resourceGithubRepositoryEnvironmentRead were only ever written to state inside case "required_reviewers": of the protection_rules loop. Once the last required reviewer is removed outside Terraform, the GitHub API stops returning a required_reviewers entry in protection_rules at all, so that case never runs, d.Set is never called for either field, and the prior state value silently survives the refresh. terraform plan then reports No changes even though the reviewer is gone.

wait_timer already avoids this exact problem by being reset to nil before the loop runs. This PR applies the same pattern to reviewers and prevent_self_review.

Changes

  • Reset reviewers to []any{} and prevent_self_review to false before the protection_rules loop in github/resource_github_repository_environment.go, so a rule missing from the API response is correctly treated as absent.
  • Added a unit test (github/resource_github_repository_environment_unit_test.go) that mocks a GetEnvironment response with protection_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

  • Added a unit test that fails against the pre-fix code and passes after the fix (no live GitHub credentials required).
  • go build ./..., go vet ./github/..., gofmt -l all clean.
  • Full non-acceptance test suite passes (go test ./github/... -run '^Test[^A]|^TestA[^c]|^TestAc[^c]').
  • Maintainer to confirm against a real environment (steps to reproduce are in the issue) if desired.

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
@github-actions

Copy link
Copy Markdown

👋 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.

@github-actions github-actions Bot added the Type: Bug Something isn't working as documented label Aug 22, 2026
@nitinjain999

nitinjain999 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

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

@deiga

deiga commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

I think this is a duplicate of #3583

Please coordinate there instead of opening a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Something isn't working as documented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Out-of-band removal of environment reviewers is not detected on refresh

2 participants