fix: Add etag diff suppression to remaining resources#3311
fix: Add etag diff suppression to remaining resources#3311ei-grad wants to merge 5 commits intointegrations:mainfrom
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>
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
| }, | ||
| "etag": { | ||
| Type: schema.TypeString, | ||
| Optional: true, |
There was a problem hiding this comment.
issue: setting Optional makes them user editable, we don't want that. Please remove from all resources
There was a problem hiding this comment.
Good catch — removed Optional: true from all 29 etag fields (including the 7 from #2840 that also had it) and updated the unit test accordingly.
There was a problem hiding this comment.
Though, this is covered in original PR:
Lastly these fields have to be marked as optional=true - [SDK ref](https://github.com/integrations/terraform-provider-github/blob/16872b724254fdddc3441c713b087cb4d7005f83/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/schema.go#L841)
Computed-only fields: Computed: true, Optional: false → Can't have DiffSuppressFunc
Optional computed fields: Computed: true, Optional: true → Can have DiffSuppressFunc
There was a problem hiding this comment.
I added tests that confirm this SDK constraint. TestEtagComputedOnlyRejectsSuppress shows InternalValidate rejects DiffSuppressFunc on Computed-only fields with "There is no config for computed-only field, nothing to compare." TestEtagSchemaConsistency now also runs InternalValidate on all 29 resources to ensure they pass.
Restored Optional: true on all etag fields (including the ones from #2840 that I removed in the previous push).
There was a problem hiding this comment.
Removed the standalone SDK validation tests — TestEtagSchemaConsistency now runs InternalValidate on its resources, which covers the same check.
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>
|
I don't like that it is copy-pasted everywhere, and that there is no dedicated test which performs |
…date in consistency test Co-Authored-By: Claude <noreply@anthropic.com>
| resourceName: resource, | ||
| }, | ||
| } | ||
| if err := p.InternalValidate(); err != nil { |
There was a problem hiding this comment.
question: this is meant for provider validation and not resource, what is the benefit of adding it?
2 participants
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_blockThe existing
TestEtagSchemaConsistencyunit test is extended to cover all 29 resources (7 existing + 22 new).Fixes #3103
Fixes #3291