fix: Add etag diff suppression to remaining resources (supersedes #3311)#3474
Open
mtb-xt wants to merge 5 commits into
Open
fix: Add etag diff suppression to remaining resources (supersedes #3311)#3474mtb-xt wants to merge 5 commits into
mtb-xt wants to merge 5 commits into
Conversation
… 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>
|
👋 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. |
Contributor
There was a problem hiding this comment.
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, andDiffSuppressOnRefresh: trueto etag fields in 22 resources - Extended
TestEtagSchemaConsistencyto cover all 29 resources (7 existing + 22 new) - Added
InternalValidatecall in the test to guard against future regressions ifOptionalis 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
DiffSuppressFuncandDiffSuppressOnRefreshto 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_groupgithub_branch_protection_v3github_emu_group_mappinggithub_enterprise_actions_runner_groupgithub_issuegithub_membershipgithub_organization_projectgithub_organization_rulesetgithub_organization_webhookgithub_project_cardgithub_project_columngithub_releasegithub_repository_autolink_referencegithub_repository_deploy_keygithub_repository_rulesetgithub_teamgithub_team_membershipgithub_team_repositorygithub_team_sync_group_mappinggithub_user_gpg_keygithub_user_ssh_keyorganization_blockTestEtagSchemaConsistencycovers all 29 resources (7 existing + 22 new) and runsInternalValidateon each.Answering the open review questions from #3311
Why
Optional: trueon the etag fields? (thread) — The SDK requires it:InternalValidaterejectsDiffSuppressFuncon 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
InternalValidatein the consistency test? (thread) — It guards exactly the constraint above: if someone later removesOptionalfrom 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 refreshedgithub_repository_rulesetetag, 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/125Verification
go build ./...,go vet ./github/cleanTestEtagSchemaConsistencysubtests passmainsince the original PR — no new etag fields need coverageFixes #3103
Fixes #3291
Likely fixes #3085