fix: Avoid merge_commit_* drift when merge/squash strategies are disabled#3556
fix: Avoid merge_commit_* drift when merge/squash strategies are disabled#3556ecerulm wants to merge 2 commits into
Conversation
|
👋 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. |
e3c41ab to
b4a716f
Compare
|
A similar thing is being done in #3310 But basically disallowing setting title and message if the settings are disabled The proposed approach here is problematic, because we shouldn't be conditionally updating upstream state, unless it's specifically necessary. In this case I would consider it a user error if they disable merge commits but keep the merge commit title configuration in code |
… disabled GitHub rejects any change to merge_commit_title/message (and the squash_merge_commit_* equivalents) while the corresponding merge strategy is disabled, returning HTTP 422 "no_merge_strategy". The provider avoids that error by not sending these fields when the strategy is off -- but the plan still shows a diff for them, so a config that both disables a strategy and specifies different title/message values produces a plan that can never converge (perpetual drift). Add a DiffSuppressFunc to merge_commit_title, merge_commit_message, squash_merge_commit_title and squash_merge_commit_message that suppresses the diff while the corresponding allow_* strategy is false. Also emit a warning diagnostic on create/update so users are told their configured values are ignored in that state rather than silently having no effect. Resolves integrations#3554
b4a716f to
5833935
Compare
My case is that I want to disable it for now but if I reenable it in two weeks, I want those Right now I'm forced to comment those out, and hope that the state did not save the wrong values, because if for some reason the values were changed in GitHub UI and differ from the state , the drift will be unsolvable without editing the state. |
@ecerulm that's not how terraform works. Your local configuration is the source of truth. If something is unset locally, it can get upstream values, but upstream can't override local values, at least not without drift. In this case it's an error that the provider allows setting message and title when the actual setting is disabled, since (as you also point out) in that case upstream controls the values and not your local config. For temporarily disabling something the correct approach is to comment it out. That will remove the values from state as well. And when adding them back it will send the values upstream again. |
This PR attempts to
well it doesn't seem to work in terraform 6.13., commenting those out and the drift is still there after applying and planning again. For me the situation is that now I'm stuck with even after commenting out these No matter how many times I apply this, because It does not even try to update those values because it knows it will get a 422 from the GitHub API if it try to do that. I would need to change the terraform configuration to match the the current state, apply that, then comment out. I feel like at least the provider should WARN the user about what is going on, it was not self evident to me that those settings were ignored because
maybe so , I don't have any problem on that being an error. But it does not solve the issue of drift. I would need to change the terraform config to match the state first (and that will error out) , apply, and comment out after, right? |
|
Please attach a debug log of plan and apply to the issue to see what's going here. Removing from config should reset the state correctly AFAIK and that linked guard should have nothing to do with it |
I have this repo with a
So step4 is what I reported before that I still see the drift in the plan after commenting those lines. But it's true that if I apply that "new config" that is really a no-op towards GitHub (due to the guard when amc=false) , then the next plan will not show drift. The step4 debug log |
|
But in any case from the point of view of usability (for the next guy that faces this) I think this PR approach is better.
But I understand that if you want to handle this in a different way, I don't know when the #3310 is planned to be merged, but I guess that at least failing on the planning phase if amc=false and mct/mcm present would be an improvement (#3558 although this will still show drift after resolving the issue by removing/commenting the mct/mcm like in step4 above) |
|
And to add some more context, in production I'm not using First the module will set the Also I don't know if Atlantis has any difference in the handling of the state. |
|
I don't agree with your proposal. Those conditionals are hard to test for all edge-cases and hard to maintain. |
Resolves #3554
Summary
merge_commit_title/merge_commit_message(and thesquash_merge_commit_*equivalents) cannot be set on GitHub while the corresponding merge strategy is disabled. Ifallow_merge_commit = false, GitHub ignores/refuses these fields — the REST API returns:Because of this, a config that sets
allow_merge_commit = falseand specifiesmerge_commit_title/merge_commit_messageproduces a plan that can never converge.Behaviour: v6.13.0 vs this PR
terraform-provider-github v6.13.0 (current)
To avoid the 422 above, the provider already skips setting these fields when the strategy is disabled — it simply never puts them in the API request:
The problem: nothing tells Terraform the fields are being ignored. The values still appear in the plan as a pending change, the apply silently does not send them, GitHub keeps its existing values, and the next plan shows the same change again — perpetual, non-convergent drift:
This PR
The fix keeps the "don't send them" behaviour (still correct — GitHub would 422), but makes the provider honest and convergent about it:
merge_commit_*/squash_merge_commit_*values are ignored from plan onwards while the strategy is disabled. ADiffSuppressFuncsuppresses the diff on these fields wheneverallow_merge_commit/allow_squash_mergeisfalse, so the plan converges (no more perpetual drift) and re-applies are no-ops.When the strategy is enabled again, diffs on these fields work exactly as before.
Why the warning is emitted at apply time, not plan time
Ideally this warning would also appear during
terraform plan. Unfortunately the Terraform Plugin SDK v2 (which this provider uses) has no hook that can emit a warning at plan time conditioned on another attribute:CustomizeDiffis the only plan-time hook that can see sibling attributes (e.g. readallow_merge_commitwhile deciding aboutmerge_commit_title). But its signature returnserroronly — it can fail the plan, not attach a warning. Turning this into a plan-time signal would mean erroring out and blockingapply, which is stricter than the ignored-setting deserves.ValidateDiagFuncdoes run at plan/validate time and can returndiag.Diagnostics(including warnings), but its signature isfunc(interface{}, cty.Path)— it only receives the single attribute's own value and path, with no access to sibling attributes. So a validator onmerge_commit_titlecannot know whetherallow_merge_commitisfalse, which is exactly the condition we need to warn about.There is therefore no way in SDK v2 to emit a conditional plan-time warning here. The warning is instead emitted from the create/update path, so it surfaces at apply time via
diag.Diagnostics.Consequence: if the only change in a plan is an ignored
merge_commit_*value, that plan is a no-op, no update runs, and no warning is shown for that specific run — but theDiffSuppressFuncstill suppresses the diff, which is what prevents the drift. The warning reliably appears whenever an update actually runs (e.g. on create, or whenallow_merge_commititself flips tofalse).Verification
Verified end-to-end against a real repository, running the same sequence on both providers:
allow_merge_commit=false+ changedmerge_commit_*terraform planagainPull request checklist
Does this introduce a breaking change?