fix: Add etag diff suppression to remaining resources (supersedes #3311) by mtb-xt · Pull Request #3474 · integrations/terraform-provider-github

mtb-xt · 2026-06-04T04:40:49Z

This is #3311 by @ei-grad, rebased onto current main (the original branch had fallen 64 commits behind). All commits retain original authorship; review feedback from that PR is already incorporated. Opening this to keep the fix moving — @ei-grad, happy to close in favor of yours if you'd rather rebase the original.

Summary

PR #2840 added DiffSuppressFunc and DiffSuppressOnRefresh to etag fields in 7 resources to prevent spurious diffs on plan/apply. This PR applies the same fix to the remaining 22 resources that were missed:

github_actions_runner_group
github_branch_protection_v3
github_emu_group_mapping
github_enterprise_actions_runner_group
github_issue
github_membership
github_organization_project
github_organization_ruleset
github_organization_webhook
github_project_card
github_project_column
github_release
github_repository_autolink_reference
github_repository_deploy_key
github_repository_ruleset
github_team
github_team_membership
github_team_repository
github_team_sync_group_mapping
github_user_gpg_key
github_user_ssh_key
organization_block

TestEtagSchemaConsistency covers all 29 resources (7 existing + 22 new) and runs InternalValidate on each.

Answering the open review questions from #3311

Why Optional: true on the etag fields? (thread) — The SDK requires it: InternalValidate rejects DiffSuppressFunc on Computed-only fields with "There is no config for computed-only field, nothing to compare." This matches how the 7 resources fixed in #2840 are declared.

What's the benefit of running InternalValidate in the consistency test? (thread) — It guards exactly the constraint above: if someone later removes Optional from an etag field (a natural-looking cleanup — it was in fact requested in the first review round), the test fails immediately instead of the provider breaking at runtime. The two review threads on #3311 are each other's justification.

Real-world impact

The etag churn makes no-op plans non-reproducible: a planfile saved at PR-review time never byte-matches the apply-time plan, which breaks plan-equality gates in CI (e.g. atmos terraform plan-diff / Cloud Posse's terraform-apply GitHub Action). Byte-level evidence from our production environment: a no-op plan differs from its immediate re-plan in 6 of 7352 lines — five occurrences of a refreshed github_repository_ruleset etag, plus the plan timestamp. lifecycle.ignore_changes = [etag] cannot work around this since the attribute is provider-managed. Downstream report with details: https://github.com/orgs/cloudposse/discussions/125

Verification

go build ./..., go vet ./github/ clean
Full unit suite passes; all 28 TestEtagSchemaConsistency subtests pass
Audited resources added to main since the original PR — no new etag fields need coverage

Fixes #3103
Fixes #3291
Likely fixes #3085

… fields PR integrations#2840 added etag diff suppression to 7 resources but missed the remaining 22. This causes spurious diffs on every plan/apply for resources like github_team, github_membership, github_team_repository, github_repository_deploy_key, and others. Apply the same pattern (Optional + DiffSuppressFunc returning true + DiffSuppressOnRefresh) to all remaining resource etag schema fields and extend the unit test to cover all 29 resources. Fixes integrations#3103 Fixes integrations#3291 Co-Authored-By: Claude <noreply@anthropic.com>

etag is server-computed and should not be user-editable. Remove Optional: true from all etag schema fields (both the 22 new ones and the 7 from PR integrations#2840) and update the unit test accordingly. Co-Authored-By: Claude <noreply@anthropic.com>

…sFunc The SDK explicitly rejects DiffSuppressFunc on Computed-only fields (InternalValidate returns: "DiffSuppressFunc is for suppressing differences between config and state representation. There is no config for computed-only field, nothing to compare."). Add tests demonstrating this SDK constraint, and verify all 29 resources pass InternalValidate with their current schema. Co-Authored-By: Claude <noreply@anthropic.com>

…ssFunc The terraform-plugin-sdk v2 InternalValidate rejects DiffSuppressFunc on Computed-only fields with: "There is no config for computed-only field, nothing to compare." Optional: true is required to use DiffSuppressFunc, as demonstrated by the new tests. Co-Authored-By: Claude <noreply@anthropic.com>

…date in consistency test Co-Authored-By: Claude <noreply@anthropic.com>

github-actions · 2026-06-04T04:40:58Z

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

Copilot

Pull request overview

These provider review instructions are being used. No blocking findings found.

This PR extends the etag diff suppression fix from PR #2840 to the remaining 22 resources that were missed. It adds Optional: true, DiffSuppressFunc (always returns true), and DiffSuppressOnRefresh: true to all etag schema fields across the provider, preventing spurious plan diffs caused by etag value changes on refresh. The test suite is extended to validate the pattern across all 29 resources.

Changes:

Added Optional: true, DiffSuppressFunc, and DiffSuppressOnRefresh: true to etag fields in 22 resources
Extended TestEtagSchemaConsistency to cover all 29 resources (7 existing + 22 new)
Added InternalValidate call in the test to guard against future regressions if Optional is removed

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
github/resource_github_actions_runner_group.go	Add etag diff suppression
github/resource_github_branch_protection_v3.go	Add etag diff suppression
github/resource_github_emu_group_mapping.go	Add etag diff suppression
github/resource_github_enterprise_actions_runner_group.go	Add etag diff suppression
github/resource_github_issue.go	Add etag diff suppression
github/resource_github_membership.go	Add etag diff suppression
github/resource_github_organization_project.go	Add etag diff suppression
github/resource_github_organization_ruleset.go	Add etag diff suppression
github/resource_github_organization_webhook.go	Add etag diff suppression
github/resource_github_project_card.go	Add etag diff suppression
github/resource_github_project_column.go	Add etag diff suppression
github/resource_github_release.go	Add etag diff suppression
github/resource_github_repository_autolink_reference.go	Add etag diff suppression
github/resource_github_repository_deploy_key.go	Add etag diff suppression
github/resource_github_repository_ruleset.go	Add etag diff suppression
github/resource_github_team.go	Add etag diff suppression
github/resource_github_team_membership.go	Add etag diff suppression
github/resource_github_team_repository.go	Add etag diff suppression
github/resource_github_team_sync_group_mapping.go	Add etag diff suppression
github/resource_github_user_gpg_key.go	Add etag diff suppression
github/resource_github_user_ssh_key.go	Add etag diff suppression
github/resource_organization_block.go	Add etag diff suppression
github/resource_github_etag_unit_test.go	Extend consistency test to cover all 29 resources with `InternalValidate`

deiga · 2026-07-02T09:32:48Z

@mtb-xt You don't want to continue on this?

mtb-xt · 2026-07-02T09:50:12Z

@mtb-xt You don't want to continue on this?

I just saw there was some traction in #3311 ? I can reopen it if needed.

deiga · 2026-07-02T10:02:38Z

It stalled 2w ago, so honestly either one works.
Not that I can promise you when we'll get to merging 😬

deiga

LGTM

The etag diff-suppression change added a second "etag" schema entry to resourceGithubRelease while the original computed-only "etag" field was still present, producing a duplicate map key. This is not caught by `go build` but fails `go vet` (and therefore `make test`), breaking CI: github/resource_github_release.go: duplicate key "etag" in map literal Merge the two entries into a single Optional+Computed "etag" field that keeps the DiffSuppressFunc / DiffSuppressOnRefresh behaviour and retains the original description. Regenerate docs/resources/release.md so the field is listed under Optional and matches the current schema. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuWwVsizoG5izMaB75qqmt

mtb-xt · 2026-08-08T06:52:19Z

LGTM

Sorry, last push was failing the checks. I've fixed it, could you please reapprove the run and merge

deiga

Please add Description fields to all the modified etag fields 🙏

Per review feedback on integrations#3474: the 22 etag fields that gained DiffSuppressFunc / DiffSuppressOnRefresh had no schema Description. Add a short description to each, matching the existing "An etag representing the <object>." style used by the originally-suppressed resources. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuWwVsizoG5izMaB75qqmt

mtb-xt · 2026-08-08T09:30:47Z

Please add Description fields to all the modified etag fields 🙏

Done, please have a look when you have a moment!

deiga

LGTM

robert-crandall

Thanks for picking this up and rebasing it - the etag work looks good and the test coverage across all 29 resources is a nice touch. One thing to fix before merge: the github_release change re-adds ForceNew: true to discussion_category_name, which looks like a rebase artifact from #3526 (main removed it and handles the field in the update path now). Since that would force releases to be recreated, I want to make sure this was intentional. Everything else looks ready to go.

Rebase artifact: the github_release change re-introduced ForceNew:true on discussion_category_name, which integrations#3526 had removed on main in favour of handling the field in the update path. With ForceNew back, that update handling was dead and any change to the field would force the release to be recreated. Restore the field to match main exactly: remove ForceNew and restore the full description. Regenerate docs/resources/release.md accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuWwVsizoG5izMaB75qqmt

mtb-xt · 2026-08-11T03:32:45Z

@robert-crandall good catch - that was exactly a rebase artifact.

Fixed in a69c46f: removed the stray ForceNew: true from discussion_category_name so it matches main and stays handled in the update path (no forced recreation), and restored the description main uses. Diffed the whole file against main to confirm that was the only stray change - the only intended delta left is the etag diff-suppression.

I also updated the branch, and all this means we need new approvals to run checks :)

Thanks!

robert-crandall

Thank you for making that fix!

ei-grad and others added 5 commits June 4, 2026 16:35

test: Remove standalone SDK validation tests, covered by InternalVali…

b451ffa

…date in consistency test Co-Authored-By: Claude <noreply@anthropic.com>

github-actions Bot added the Type: Bug Something isn't working as documented label Jun 4, 2026

mtb-xt mentioned this pull request Jun 4, 2026

fix: Add etag diff suppression to remaining resources #3311

Open

deiga requested a review from Copilot June 6, 2026 06:03

Copilot started reviewing on behalf of deiga June 6, 2026 06:03 View session

Copilot AI reviewed Jun 6, 2026

View reviewed changes

Merge branch 'main' into fix-etag-drift-remaining-resources-rebased

e0b1e8e

mtb-xt closed this Jul 2, 2026

mtb-xt reopened this Jul 2, 2026

mtb-xt added 3 commits July 2, 2026 22:28

Merge branch 'main' into fix-etag-drift-remaining-resources-rebased

866a611

Merge branch 'main' into fix-etag-drift-remaining-resources-rebased

8228630

Merge branch 'main' into fix-etag-drift-remaining-resources-rebased

5a57548

deiga previously approved these changes Aug 8, 2026

View reviewed changes

mtb-xt dismissed deiga’s stale review via 220dd66 August 8, 2026 06:50

deiga requested changes Aug 8, 2026

View reviewed changes

deiga previously approved these changes Aug 8, 2026

View reviewed changes

deiga added the needs-github-review Request a review from GitHub label Aug 8, 2026

team-review-router Bot requested a review from a team August 8, 2026 10:08

robert-crandall reviewed Aug 10, 2026

View reviewed changes

mtb-xt dismissed deiga’s stale review via a69c46f August 11, 2026 03:26

Merge branch 'main' into fix-etag-drift-remaining-resources-rebased

94a89fc

deiga approved these changes Aug 11, 2026

View reviewed changes

robert-crandall approved these changes Aug 11, 2026

View reviewed changes

deiga merged commit c55240a into integrations:main Aug 11, 2026
11 checks passed

Uh oh!

Conversation

mtb-xt commented Jun 4, 2026

Summary

Answering the open review questions from #3311

Real-world impact

Verification

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

deiga commented Jul 2, 2026

Uh oh!

mtb-xt commented Jul 2, 2026

Uh oh!

deiga commented Jul 2, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

mtb-xt commented Aug 8, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

mtb-xt commented Aug 8, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

robert-crandall left a comment

Choose a reason for hiding this comment

Uh oh!

mtb-xt commented Aug 11, 2026

Uh oh!

robert-crandall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants