Repository navigation
feat(scaffold): add --merge-driver flag to force text-based merging - #2925
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
|
Per the semver label decision tree, this should be labeled |
The YAML-aware 3-way merger re-encodes matched files through gopkg.in/yaml.v3, which has no concept of blank lines between blocks and silently drops them on every --update, even when nothing meaningful changed. --merge-driver=text (default stays auto) forces every file through the line-oriented diff3 merger instead, preserving formatting conventions YAML re-encoding can't. Applies to both `atmos scaffold generate --update` and `atmos init --update`, which share the same merge engine. Closes cloudposse#2886.
Adds the scaffold-merge-driver blog post announcing the new flag, links it from the dx initiative's roadmap milestone, and registers jorrite in authors.yml.
622e0f4 to
1589d65
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesMerge-driver selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InitOrScaffold
participant InitUI
participant Processor
participant ThreeWayMerger
InitOrScaffold->>InitUI: apply parsed merge.Driver
InitUI->>Processor: SetMergeDriver(driver)
Processor->>ThreeWayMerger: SetDriver(driver)
ThreeWayMerger->>ThreeWayMerger: select YAML-aware or text merge
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
website/docs/cli/commands/init.mdx (1)
122-128: 📐 Maintainability & Code Quality | 🔵 TrivialRun the required website validation for both changed command pages.
website/docs/cli/commands/init.mdx#L122-L128: runcd website && npm run build, then verify links, images, and MDX rendering.website/docs/cli/commands/scaffold/generate.mdx#L43-L46: run the same build and verification.website/docs/cli/commands/scaffold/generate.mdx#L144-L150: run the same build and verification.As per coding guidelines: "
website/docs/**/*.mdx: Always runcd website && npm run buildafter documentation changes and verify links, images, and MDX rendering."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/cli/commands/init.mdx` around lines 122 - 128, Run `cd website && npm run build` after the documentation changes, then verify links, images, and MDX rendering for website/docs/cli/commands/init.mdx lines 122-128, website/docs/cli/commands/scaffold/generate.mdx lines 43-46, and website/docs/cli/commands/scaffold/generate.mdx lines 144-150.Source: Coding guidelines
cmd/scaffold/interfaces.go (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
SetMergeDriver.Add a method comment that states that it selects the merger used by scaffold updates. This method is part of the public
ScaffoldUIcontract.As per coding guidelines, “Document all exported functions, types, and methods following Go's documentation conventions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/scaffold/interfaces.go` at line 16, Add a Go-style documentation comment immediately before ScaffoldUI.SetMergeDriver stating that it selects the merger used by scaffold updates, and ensure the comment begins with SetMergeDriver.Source: Coding guidelines
pkg/generator/merge/merge_test.go (1)
361-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a table for the driver scenarios.
Lines 361-388 test two variants of the same merge flow. Put the driver and expected formatting properties in a test table. This keeps new driver cases in one execution path.
As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/generator/merge/merge_test.go` around lines 361 - 388, Refactor the two driver-specific subtests around NewThreeWayMerger into a table-driven test covering each driver and its expected blank-line and formatting behavior. Iterate over the cases through one shared Merge execution path, while preserving the existing assertions for DriverAuto and DriverText, including list indentation and merged task checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/generator/engine/update_test.go`:
- Around line 268-278: Strengthen TestProcessorSetMergeDriverForcesTextMerge in
pkg/generator/engine/update_test.go (lines 268-278) by asserting the complete
expected output and adding a changed-side merge case that verifies text-driver
selection and byte-for-byte formatting. Apply the same exact-output and
changed-side assertions in pkg/generator/ui/ui_test.go (lines 131-141) through
InitUI.SetMergeDriver; both sites require direct test updates.
In `@website/src/data/roadmap.js`:
- Line 250: Update the roadmap entry’s description for the
`scaffold-merge-driver` changelog to describe `text` as using a line-oriented
diff3-style merge, removing the claim that it is the same algorithm Git uses
while preserving the YAML-aware merger contrast.
---
Nitpick comments:
In `@cmd/scaffold/interfaces.go`:
- Line 16: Add a Go-style documentation comment immediately before
ScaffoldUI.SetMergeDriver stating that it selects the merger used by scaffold
updates, and ensure the comment begins with SetMergeDriver.
In `@pkg/generator/merge/merge_test.go`:
- Around line 361-388: Refactor the two driver-specific subtests around
NewThreeWayMerger into a table-driven test covering each driver and its expected
blank-line and formatting behavior. Iterate over the cases through one shared
Merge execution path, while preserving the existing assertions for DriverAuto
and DriverText, including list indentation and merged task checks.
In `@website/docs/cli/commands/init.mdx`:
- Around line 122-128: Run `cd website && npm run build` after the documentation
changes, then verify links, images, and MDX rendering for
website/docs/cli/commands/init.mdx lines 122-128,
website/docs/cli/commands/scaffold/generate.mdx lines 43-46, and
website/docs/cli/commands/scaffold/generate.mdx lines 144-150.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f4e2e9a8-7593-4d92-acc3-bb28e85b8492
📒 Files selected for processing (18)
cmd/init/init.gocmd/init/init_test.gocmd/scaffold/interfaces.gocmd/scaffold/mock_interfaces.gocmd/scaffold/scaffold.gocmd/scaffold/scaffold_coverage_test.gocmd/scaffold/scaffold_test.goerrors/errors.gopkg/generator/engine/merge_update.gopkg/generator/engine/update_test.gopkg/generator/merge/merge.gopkg/generator/merge/merge_test.gopkg/generator/ui/ui.gopkg/generator/ui/ui_test.gowebsite/blog/authors.ymlwebsite/docs/cli/commands/init.mdxwebsite/docs/cli/commands/scaffold/generate.mdxwebsite/src/data/roadmap.js
Strengthens merge-driver tests with exact-content assertions and a genuine-divergence case (instead of partial substring checks), adds the missing SetMergeDriver doc comment, converts the driver test to table-driven, and drops the overclaimed 'same as git' phrasing from the roadmap entry and changelog post (the text driver uses a diff3-style merge, not git's own diff3/merge-file implementation).
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/blog/2026-08-11-scaffold-merge-driver.mdx`:
- Around line 9-10: Revise the merge-preservation claims in the scaffold
merge-driver description, including the corresponding text at the later
referenced section, to use conditional wording. Clarify that local
customizations and comments/blank lines are preserved only when changes are
non-conflicting, while --merge-strategy=theirs may replace local changes in
genuine conflicts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 368ae820-d041-4fca-97f5-ff3c94363254
📒 Files selected for processing (1)
website/blog/2026-08-11-scaffold-merge-driver.mdx
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2925 +/- ##
==========================================
- Coverage 83.21% 83.20% -0.01%
==========================================
Files 1893 1893
Lines 184127 184170 +43
==========================================
+ Hits 153216 153238 +22
- Misses 23082 23096 +14
- Partials 7829 7836 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.226.0-rc.8. |
3 participants
what
--merge-driverflag (auto/text) toatmos scaffold generate --updateandatmos init --update, alongside the existing--merge-strategyflag.auto(default) is today's existing behavior: pick the merger by file extension (YAML-aware for.yaml/.yml, line-oriented text otherwise).textforces every file — YAML included — through the line-oriented diff3 merger, bypassing the YAML-aware re-encode that has no concept of blank lines between blocks and silently drops them on every update, even when nothing meaningful changed.why
Structure-aware YAML merging is the right default for most files, but it re-encodes the whole document through a YAML parser/serializer, and formatting like blank lines between top-level blocks isn't part of what a YAML parser models. Templates that bundle CI pipeline YAML (a common convention uses blank lines to visually separate jobs/stages) lost that formatting on every
--update, whether or not the file actually changed.--merge-driver=textgives users an explicit opt-out, mirroring git's own merge driver concept (auto/text), so this class of file can go through the same merge algorithmgit mergeitself uses on ordinary text files.references
Closes #2886.