integrations / integrations/terraform-provider-github
[BUG]: Out-of-band removal of environment reviewers is not detected on refresh
- Dominant language
- Go
- Stars
- 1.2k
- Forks
- 1k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 8
Description
### Expected Behavior
When a required reviewer is removed from an environment outside Terraform, the next `terraform plan` should report a change, because the configuration still declares the reviewer and GitHub no longer has it.
### Actual Behavior
`terraform plan` reports `No changes`, and the reviewer stays removed. The drift is invisible for as long as the configuration is left alone.
I checked whether the API is at fault and it is not. `GET /repos/{owner}/{repo}/environments/{name}` reports the removal faithfully: once the last reviewer is gone, the response's `protection_rules` array simply has no `required_reviewers` entry in it. The information the provider needs is in the response.
The cause is in `resourceGithubRepositoryEnvironmentRead`, in `github/resource_github_repository_environment.go`. Five fields are populated by three different patterns, and only two of them survive an entry disappearing from `protection_rules`:
| Field | How it is populated | Refreshes on removal? |
|---|---|---|
| `wait_timer` | Reset to `nil` before the loop | Yes |
| `can_admins_bypass` | Read from the top-level `env` object | Yes |
| `deployment_branch_policy` | `if env.DeploymentBranchPolicy != nil { … } else { d.Set(…, []any{}) }` | Yes |
| `reviewers` | Set **only** inside `case "required_reviewers":` | **No** |
| `prevent_self_review` | Set **only** inside the same case | **No** |
`wait_timer` gets it right, and it is worth reading as the fix already present in the same function:
```go
if err := d.Set("wait_timer", nil); err != nil {
return diag.FromErr(err)
}
```
That pre-loop reset means a vanished `wait_timer` entry leaves `nil` behind. `reviewers` and `prevent_self_review` have no equivalent, so when the `required_reviewers` case never executes, `d.Set` is never called for either and the prior state value survives the refresh untouched. `terraform plan` then compares the configuration against stale state and correctly reports no changes about a state that is wrong.
`prevent_self_review` is caught by the same defect for the same reason, since it is set inside that case as well.
This looks like a two-line fix: reset both fields before the loop, the way `wait_timer` already is.
Note this is not #3156, which was about removing `reviewers` through configuration and is fixed. This is the reverse direction: a removal made outside Terraform is never detected.
### Terraform Version
```
Terraform v1.15.8
on darwin_arm64
+ provider registry.terraform.io/integrations/github v6.13.0
```
Also read at `main`, where `github/resource_github_repository_environment.go` is byte-identical to `v6.13.0` apart from a `go-github` v88 to v89 bump and a type-assertion style change. Neither touches this logic.
### GitHub Installation Type
GitHub.com (Free, Pro, or Team)
### Affected Resource(s)
- `github_repository_environment`
### Terraform Configuration Files
```hcl
resource "github_repository_environment" "example" {
repository = "example-repo"
environment = "production"
reviewers {
users = [1234567]
}
}
```
### Steps to Reproduce
1. `terraform apply` the configuration above, on a public repository or a plan tier that allows required reviewers.
2. In the GitHub UI, go to Settings > Environments > production and remove the reviewer, so no reviewers remain.
3. `terraform plan`.
Expected: a change is reported, restoring the reviewer. Actual: `No changes`.
Comparing the API response before and after step 2 shows the `required_reviewers` entry present, then absent, with the rest of the object unchanged.
### Code of Conduct
- [X] I agree to follow this project's Code of Conduct
Contributor guide
Research direction
Read resourceGithubRepositoryEnvironmentRead in github/resource_github_repository_environment.go, starting with the existing wait_timer reset before the protection_rules loop. Verify the refresh behavior when required_reviewers is absent, and consider the issue's described reset pattern for reviewers and prevent_self_review; done means terraform plan detects the externally removed reviewer and restores the declared state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, terraform
- Domain
- infrastructure
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 88/100