fix: preserve repository fields a read-only token cannot see - #1
Open
th7 wants to merge 1 commit into
Open
Conversation
GitHub omits merge-related settings from the repository object unless the caller has push or admin access: "To view merge-related settings, you must have the contents:read and contents:write permissions." Read through a Get* accessor, an omitted field yields the zero value, which is indistinguishable from the field genuinely being false or empty. Writing that to state produces a diff on every plan that no apply can resolve, because the value an apply would write back is not readable either. Setting only what the response actually contained leaves prior state in place instead. integrations#3144 took this approach for vulnerability_alerts and security_and_analysis, keying off the presence of security_and_analysis in the response and deprecating ignore_vulnerability_alerts_during_read as "ignored as the provider now handles lack of permissions automatically". setIfPresent extends the same treatment to the merge-related fields, and sits in util.go next to resourceKeysGetOk because the bug class is not repository-specific. Skipping a field silently would hide the trade-off, so the read logs one tflog.Warn per repository naming the fields it could not read. integrations#3553 uses the same signal for archived and deleted repositories. A user-facing warning diagnostic would fire on every plan for a token that is permanently read-only, which is noise rather than information. The archived special-case is deliberately left in place. It may be redundant — the TODO added alongside integrations#3144 doubts that GitHub omits these fields for archived repositories at all — but that is unverified, and removing it would be a second, untested behaviour change on top of this one. The cost is that drift in these fields goes undetected while the token cannot read them, which is preferable to a permanent diff no apply can clear. Verified with a scoped installation token holding administration:read and contents:read against a configuration managing nine repositories: every one reported the diff on every plan before, none do after. Reverting to the released provider reproduces it, so the binary is the only variable. Refs integrations#3382
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
1 participant
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.
Refs integrations#3382
Before the change?
GitHub omits merge-related settings from the repository object unless the caller
has push or admin access — per the REST docs, "To view merge-related settings,
you must have the
contents:readandcontents:writepermissions."The read path could not tell "omitted" from "false", since a
Get*accessorreturns the zero value for both. An under-privileged token therefore wrote
false/""into state for fields it had never seen, producing a diff on everyplan that no apply could clear — the value an apply would write back is not
readable either. Any GitHub App token scoped to
administration:read+contents:readhas a permanently dirty plan.After the change?
Only fields present in the response are written, so an omitted field keeps its
prior state.
Preserving state rather than raising an error is what the provider already
decided. integrations#3144 established that an absent field means "the caller cannot see
it", and that the provider should detect that itself rather than make the
practitioner declare it — it keyed the vulnerability-alerts read off the
presence of
security_and_analysis, and retired the manual opt-outignore_vulnerability_alerts_during_readas "ignored as the provider nowhandles lack of permissions automatically." integrations#3553 continued in that direction,
replacing hard errors on archived/deleted repos with a warning and a state
removal. The merge-related fields have the identical failure mode and were
simply never covered.
The trade-off is the one integrations#3144 already accepted: drift goes undetected while the
token cannot read these fields. That beats a diff no apply can resolve.
Relationship to integrations#3556. That PR fixes a different bug on four of the same
fields — GitHub rejects changes to
merge_commit_*while the strategy isdisabled (422
no_merge_strategy) — viaDiffSuppressFuncin the schema. Thisone is about fields missing from the response. Complementary, and they merge
cleanly: integrations#3556 touches the schema block, this touches the read path.
Scope — the
archivedguard is untouched, byte-identical including its TODO.Folding archiving into the same presence check was tempting, since that TODO
doubts the fields are omitted for archived repos at all, but I have no archived
repository to verify against and would rather not ship a second, untested
behaviour change alongside this one. Happy to follow up if a maintainer can
confirm.
Verification. Reproduced with a scoped installation token holding
administration:readandcontents:readacross nine repositories: every onereported the diff on every plan before, none do after. Reverting to the released
provider brings it back, so the binary is the only variable.
Pull request checklist
TestResourceGithubRepositoryRead_omittedFieldsdrives the real read against anhttptestserver, with the field omitted and with it present. It fails if thefix is reverted, so it pins the regression rather than just exercising the
helper. No acceptance test: reproducing this needs a deliberately
under-privileged token, which the acceptance suite cannot provision.
Does this introduce a breaking change?
A token that can read these fields sees no change. A token that cannot stops
seeing an unresolvable diff and keeps its last known values instead.