fix: Error at plan when merge/squash commit title/message set with strategy disabled (alt. to #3556)#3558
Draft
ecerulm wants to merge 2 commits into
Draft
fix: Error at plan when merge/squash commit title/message set with strategy disabled (alt. to #3556)#3558ecerulm wants to merge 2 commits into
ecerulm wants to merge 2 commits into
Conversation
…rategy 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". A config that both disables a strategy and explicitly sets these values can therefore never be applied. Add a CustomizeDiff validation that fails the plan with a clear message when the user has explicitly configured these fields (detected via the raw config, so schema defaults do not trigger it) while allow_merge_commit / allow_squash_merge is false. This surfaces the mistake at plan time instead of producing a plan that silently never converges. Alternative approach to integrations#3554 (see also the diff-suppression + warning approach). Resolves integrations#3554
|
👋 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. |
5 tasks
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.
Resolves #3554
Background
merge_commit_title/merge_commit_message(and thesquash_merge_commit_*equivalents) cannot be set on GitHub while the corresponding merge strategy is disabled. Withallow_merge_commit = false, the REST API returns:The current provider avoids the 422 by simply not sending these fields when the strategy is off — but nothing tells the user, so the plan keeps showing a change that the apply silently never makes: perpetual, non-convergent drift.
After the change?
A
CustomizeDiffvalidation fails the plan when the user has explicitly configuredmerge_commit_*/squash_merge_commit_*while the correspondingallow_merge_commit/allow_squash_mergeisfalse:Key details:
GetRawConfig), i.e. the value is actually present in the user's.tf. Becausemerge_commit_*have schema defaults, a config that only setsallow_merge_commit = falseand never mentions the title/message is not affected.CustomizeDiff), so it's caught before any apply.Trade-off vs #3556
Verification
Verified against a real repository:
allow_merge_commit=false+merge_commit_titleexplicitly set → plan errors ✅allow_merge_commit=false, title/message not set → plan succeeds ✅ (no false positive)allow_merge_commit=true+ title set → plan succeeds ✅Pull request checklist
Does this introduce a breaking change?
merge_commit_*/squash_merge_commit_*while the corresponding strategy is disabled will now fail at plan time (previously it applied with perpetual drift).