Skip to content

[Backport pipeline] Fix manifest.yml data loss during cherry-pick conflict resolution#19962

Open
mrodm wants to merge 13 commits into
elastic:mainfrom
mrodm:apply_changes_in_manifest
Open

[Backport pipeline] Fix manifest.yml data loss during cherry-pick conflict resolution#19962
mrodm wants to merge 13 commits into
elastic:mainfrom
mrodm:apply_changes_in_manifest

Conversation

@mrodm

@mrodm mrodm commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

[Backport pipeline] Fix manifest.yml data loss during cherry-pick conflict resolution

TL;DR

The backport-apply pipeline (dev/backports/apply/apply.go) discarded all changes to a package's manifest.yml whenever a cherry-pick conflicted on it — not just the version bump, but any other legitimate content the source commit touched (new fields, category/owner changes, data stream additions, etc.). This PR fixes that: only a genuine version-line conflict is auto-resolved (version normalized back to the branch's own lineage), everything else is preserved or surfaced as a real conflict for a human to resolve. Includes several follow-up hardening, observability, and cleanup passes from two rounds of code review.

Proposed commit message

[Backport pipeline] Fix manifest.yml data loss during cherry-pick conflict resolution

WHAT:
- cherryPickOrConflict no longer unconditionally resets manifest.yml to HEAD
  after a cherry-pick. manifest.yml is left as the cherry-pick merges it; a
  conflict block that is purely a "version:" line difference (expected,
  since each backport branch owns its own version lineage independently of
  main) is auto-resolved by keeping the incoming content, then the version
  field is force-reset to the branch's own pre-cherry-pick value. Any
  conflict block containing anything else is left as a genuine, reported
  conflict.
- manifest.yml going missing entirely (before the cherry-pick even starts,
  or as a result of it — delete/modify conflict or a clean
  delete/rename) is now reported as a normal conflict result instead of
  crashing with an opaque file-read error.
- The cherry-pick now forces merge.conflictStyle=merge so the hand-rolled
  conflict-marker parser isn't at the mercy of the caller's git config.
- A post-resolution invariant check guards against ever writing a
  manifest.yml with zero or more than one "version:" line, as defense in
  depth against the auto-resolve logic being wrong.
- Diagnostic logging distinguishes "manifest.yml untouched", "version-only
  conflict auto-resolved", and "conflict needs manual resolution".
- manifest.yml version reads are consolidated onto dev/citools.ReadPackageManifest
  (removing a duplicate yaml.v3-based parser), and the repeated
  `git reset --hard HEAD` cleanup across error paths is factored into a
  single abortCherryPick helper.
- The auto-opened backport PR's author checklist now asks reviewers to
  compare the resulting manifest.yml against the original PR.

WHY:
manifest.yml's version field is independently owned by each backport
branch, so a version-line conflict during cherry-pick is expected on
almost every backport. The previous code exploited this by wholesale
discarding manifest.yml back to HEAD any time cherry-pick reported a
conflict on it, which also silently dropped any other, unrelated content
the source commit legitimately changed — a data-loss bug for any backport
touching manifest.yml beyond its version bump.

Author's Checklist

  • Revert the local debugging change (commit 47b0768d89, "Test changes locally") that disables git push/gh pr create and adds a duplicate .backports.yml test entry
  • go test ./dev/backports/apply/... passes (unit + integration tests, including the new manifest-preservation and missing-manifest regression tests)
  • go vet ./dev/backports/... is clean

How to test this PR locally

go test ./dev/backports/apply/...

Or exercise the pipeline directly against a scratch repo:

dev/scripts/backport_apply.sh \
  --sha <commit-sha> \
  --package <package-name> \
  --target <version-or-branch> \
  --dry-run

Related issues


This PR was generated with the assistance of Claude (claude-sonnet-5).

mrodm and others added 13 commits July 2, 2026 16:24
…cherry-pick

Previously the apply script always reset manifest.yml to HEAD after a
cherry-pick, discarding any legitimate content changes (e.g. new fields)
alongside the version bump. Now only a version-line-only conflict is
auto-resolved, letting the rest of the file's cherry-picked content merge
normally; genuine conflicts elsewhere in the file are still reported.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resolveManifestVersionConflict parses git's rendered conflict-marker
text directly. Under diff3/zdiff3 conflict styles the extra base
section made every conflict block look unresolvable, silently
degrading auto-resolution with no diagnostic. Forcing the style on the
cherry-pick invocation removes the dependency on the caller's ambient
git config.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ld during conflict resolution

resolveManifestVersionConflict now requires the merged manifest.yml to
contain exactly one "version:" line before accepting it as resolved,
falling back to a reported conflict otherwise. This protects against a
version-only conflict block whose "theirs" side is empty (e.g. the
source commit's hunk deleted the field alongside unrelated content),
which previously produced a manifest.yml missing its mandatory version
field and surfaced as an opaque error instead of a normal conflict.

Test scenarios were also reworked to reflect the realistic backport
shape: the branch is the older side, advancing only through its own
small independent bumps, while the cherry-picked commit's version
(carried on main) is normally ahead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…st.yml disappears

A cherry-picked commit deleting or renaming a package's manifest.yml
left the apply pipeline with an opaque hard error instead of a normal
conflict report — whether the file vanished via a delete/modify
conflict, or a clean merge that removes it outright with no conflict
markers at all. cherryPickOrConflict now checks for this explicitly
right after the cherry-pick runs and reports Result{Status:"conflict"}
in either case, before ever calling resolveManifestVersionConflict or
setManifestVersion on a file that no longer exists.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resolveManifestVersionConflict now returns whether manifest.yml had
any conflict markers at all, not just whether it ended up resolved —
those were previously conflated, since it also reports "resolved" when
manifest.yml never conflicted in the first place (some other file
caused the cherry-pick failure). cherryPickOrConflict uses this to log
a note precisely when a real version-only conflict was silently
resolved by keeping the incoming change, so operators tailing logs can
tell that apart from an ordinary clean cherry-pick.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ersion

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t a realistic scenario

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fore the cherry-pick even starts

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tVersion, drop the duplicate yaml.v3 parser

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hard HEAD calls

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hes the original PR

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mrodm mrodm self-assigned this Jul 3, 2026
// git config: resolveManifestVersionConflict below parses "<<<<<<<"/"======="/
// ">>>>>>>" text directly, and a diff3/zdiff3 style would make every block
// look unresolvable.
cherryErr := a.git.Run("-c", "merge.conflictStyle=merge", "cherry-pick", "-n", sha)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It changes the conflict style markers in this single call/process. So, this call does not change any configuration the developers could have in their own setups or laptops.

@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

cc @mrodm

Comment on lines +455 to +458
if len(ours) == 1 && strings.HasPrefix(ours[0], "version:") {
out = append(out, theirs...)
continue
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop just accepts the conflicts related to version line.

Other conflicts in the manifest.yml are kept. So if any conflicts remains after this loop, this file is going to be reported as a conflicting file.

@mrodm mrodm marked this pull request as ready for review July 3, 2026 14:47
@mrodm mrodm requested a review from a team as a code owner July 3, 2026 14:47

b.WriteString("\n## Author's checklist\n\n")
b.WriteString("- [ ] Review the version set in `manifest.yml` and `changelog.yml`\n")
b.WriteString("- [ ] Compare the `manifest.yml` changes here against the original PR (linked above under Origin) and confirm they match — a conflict limited to the version line is auto-resolved by keeping the incoming change as-is, with no compatibility check against this branch\n")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new checklist item in the PR description to try to emphasize that developers should check the manifest.yml file, if it makes sense or no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

backport: add backport_apply.sh to apply a fix to a backport branch as a single command

1 participant