Add --conflict-policy flag to detect upstream dropped content#78
Open
mpryc wants to merge 1 commit intoopenshift-eng:mainfrom
Open
Add --conflict-policy flag to detect upstream dropped content#78mpryc wants to merge 1 commit intoopenshift-eng:mainfrom
mpryc wants to merge 1 commit intoopenshift-eng:mainfrom
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
needs-ok-to-test
Contributor
|
Hi @mpryc. Thanks for your PR. I'm waiting for a openshift-eng member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
Author
|
Now with the strict mode rebasebot works as expected: $ rebasebot --source "https://github.com/vmware-tanzu/velero-plugin-for-aws:main" --dest "openshift/velero-plugin-for-aws:oadp-dev" --rebase "oadp-rebasebot/velero-plugin-for-aws:rebase-bot-oadp-dev" --git-username "oadp-team-rebase-bot" --git-email "oadp-maintainers@redhat.com" --github-app-id "1810299" --github-app-key "/home/migi/.rebasebot/secrets/oadp-rebasebot-app-key" --github-cloner-id "1810272" --github-cloner-key "/home/migi/.rebasebot/secrets/oadp-rebasebot-cloner-key" --conflict-policy strict --post-rebase-hook git:https://github.com/oadp-rebasebot/oadp-rebase/oadp-dev:rebasebot-hook-scripts/go-replace_velero_oadp-dev.sh git:https://github.com/oadp-rebasebot/oadp-rebase/oadp-dev:rebasebot-hook-scripts/go-mod-tidy-and-commit.sh --always-run-hooks --dry-run
INFO - Logging to GitHub as an Application for repository https://github.com/openshift/velero-plugin-for-aws
INFO - Logging to GitHub as an Application for repository https://github.com/oadp-rebasebot/velero-plugin-for-aws
INFO - Destination repository is https://github.com/openshift/velero-plugin-for-aws.git
INFO - rebase repository is https://github.com/oadp-rebasebot/velero-plugin-for-aws.git
INFO - source repository is https://github.com/vmware-tanzu/velero-plugin-for-aws.git
INFO - Fetching oadp-dev from dest
INFO - Fetching main from source
INFO - Fetching all tags from source
INFO - Fetching all branches from source
INFO - Checking out source/main
INFO - Checking for existing rebase branch rebase-bot-oadp-dev in https://github.com/oadp-rebasebot/velero-plugin-for-aws
INFO - Fetching existing rebase branch
INFO - Branches with commit:
rebase/rebase-bot-oadp-dev
source/HEAD -> source/main
source/main
INFO - Preparing rebase branch
INFO - Merging upstream/main into oadp-dev
INFO - Performing rebase
INFO - Merge base of source/main and dest/oadp-dev: e930be50d4fb45c2fadafee7189b692c7156992a
INFO - Merges on ancestry-path from merge_base=(e930be50d4fb45c2fadafee7189b692c7156992a) to dest/oadp-dev branch:
7174af15e9625e981400d061bf3feea1f24600e3 || Merge pull request #117 from oadp-rebasebot/rebase-bot-oadp-dev || 138787+weshayutin@users.noreply.github.com
eb1418862addee658eb15e4bf73d005218179a31 || merge upstream/main into oadp-dev || oadp-maintainers@redhat.com
INFO - Searching for merge commit from previous rebasebot run to identify downstream commits
INFO - Found merge commit from previous rebase: eb1418862addee658eb15e4bf73d005218179a31
INFO - Its parent e930be50d4fb45c2fadafee7189b692c7156992a is on an upstream branch
INFO - Cutoff commits: ['^db5a2803e74708b3756d2fe39e7318e0dbbc963d', '^e930be50d4fb45c2fadafee7189b692c7156992a']
INFO - Identified downstream commits:
1866f267d181927f001e815543f958aaab71869c || adding Dockerfile.ubi || sseago@redhat.com
057dd5fb9d084096d1002b263ee5688915e18715 || Wait for snapshot to be ready before restoring || sseago@redhat.com
414ecb301c2bfa6e2fa304d8351f8741da3c906e || Add BZ automation to repo || rayfordj@redhat.com
3dfa6400d5d4cef58f1c36e0a24a5be3080603b7 || swapping out the builder image to konveyor/builder || jdevmane@redhat.com
cf1beecdb95fa8e0fffb8e7859fb597fd0b82eec || Enable multiarch builds || jmontleo@redhat.com
897ccc970b05954ff1701197fe8a5f2488ea4d56 || Use arm64-graviton2 for arm builds || jmontleo@redhat.com
7390134812187fb2e50aa394e2ad44d39096512f || Add required keys for arm builds || jmontleo@redhat.com
73ce2fad58910749416de4fadf83a580573f58d1 || Use numeric non-root user for nonroot SCC compatibility || jmontleo@redhat.com
4844a31be3390a09e141939b5ee9eb508a2df024 || use full vm for arm builds in travis || sseago@redhat.com
5eec6785546e44fa15e39328e8835bf92634d5a0 || OADP-3586: Update to ubi9 builder and base images || weshayutin@gmail.com
ea13afe16232274877c0f48a6158b4ea228c37a6 || `snapshot.State == ""` typo || tkaovila@redhat.com
2c43efdf3cfd2260db2fce853f1978b1e6cf14d3 || fix: ARM images (#55) || msouzaol@redhat.com
e7f20ec6248bab29b048c14b633027147295f920 || ubi: BUILDPLATFORM to build stage to enable cross compile. (#57) || passawit.kaovilai@gmail.com
ab6d13c6f4a6b8414df3bda7c9d378cf3df4728c || add DOWNSTREAM_OWNERS || tkaovila@redhat.com
0b6b73b4079bcea82cfcb3369869804ff09a74ea || UPSTREAM: <drop>: Updating go modules || oadp-maintainers@redhat.com
INFO - Picking commit: 1866f267d181927f001e815543f958aaab71869c - adding Dockerfile.ubi
INFO - Picking commit: 057dd5fb9d084096d1002b263ee5688915e18715 - Wait for snapshot to be ready before restoring
WARNING - Upstream content may have been dropped from 'velero-plugin-for-aws/volume_snapshotter.go' by cherry-pick of: 057dd5fb9d084096d1002b263ee5688915e18715 - Wait for snapshot to be ready before restoring
WARNING - lost line: ebsCSIDriver = "ebs.csi.aws.com"
WARNING - lost line: ebsKmsKeyIDKey = "ebsKmsKeyId"
WARNING - lost line: ebsKmsKeyId string
WARNING - lost line: ec2 *ec2.Client
WARNING - lost line: if err := veleroplugin.ValidateVolumeSnapshotterConfigKeys(config, regionKey, credentialProfileKey, credentialsFileKey, enableSharedConfigKey, ebsKmsKeyIDKey); err != nil {
WARNING - lost line: log logrus.FieldLogger
WARNING - lost line: regionKey = "region"
ERROR - Manual intervention is needed to rebase https://github.com/vmware-tanzu/velero-plugin-for-aws:main into openshift/velero-plugin-for-aws:oadp-dev
$ echo $?Current behavior was silently missing those, so it was going straight to the go mod update: |
When cherry-picking downstream carry patches, -Xtheirs silently resolves content conflicts by favoring the downstream version, which can drop upstream additions without any warning. Add a new --conflict-policy flag with three modes: - auto: no detection (default, preserves existing behavior) - warn: log warnings when upstream content may have been lost - strict: fail when upstream content is lost The detection compares each cherry-picked file against the upstream version to find missing lines after -Xtheirs resolution. Fixes openshift-eng#77 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Michal Pryc <mpryc@redhat.com>
|
0/ little help? |
2 tasks
Contributor
Author
|
May I ask for review? It is required by the oadp projects. Without this OADP is missing important patches that were in downstream repos. @RadekManak ? |
Contributor
|
@RadekManak is out this week, he might be able to review when he's back next week |
Contributor
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
WalkthroughThis PR introduces a conflict-aware cherry-pick workflow that detects conflicting files before applying Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/User
participant Bot as Bot.run()
participant Rebase as _do_rebase()
participant SafePick as _safe_cherry_pick()
participant Detect as _detect_conflicting_files()
participant Check as _check_upstream_content_loss()
participant Git as Git Operations
CLI->>Bot: run(conflict_policy=policy)
Bot->>Rebase: _do_rebase(conflict_policy=policy)
loop For each commit to cherry-pick
alt policy != "auto"
Rebase->>Detect: Probe for conflicts without -Xtheirs
Detect->>Git: Attempt cherry-pick
Detect-->>Rebase: Return conflicting files
end
Rebase->>SafePick: _safe_cherry_pick(sha, conflict_policy)
SafePick->>Git: Execute cherry-pick with -Xtheirs
alt Conflicting files exist
SafePick->>Check: Check for upstream content loss
Check->>Git: Compare upstream vs. current content
Check-->>SafePick: Report lost lines per file
alt policy == "strict"
SafePick-->>Rebase: Raise RepoException
else policy == "warn"
SafePick-->>Rebase: Log warning
else policy == "auto"
SafePick-->>Rebase: Silent (no-op)
end
end
SafePick-->>Rebase: Pick complete
end
Rebase-->>Bot: Rebase finished
Bot-->>CLI: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
✨ 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 |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rebasebot/bot.py (1)
542-570:⚠️ Potential issue | 🟠 MajorUse the upstream branch, not
dest.branch, for ART conflict checks.
_safe_cherry_pick()interpretssource_branchas the branch under thesource/remote. Passingdest.branchhere means ART PR checks look upsource/<dest.branch>. When source and dest branch names differ, every lookup falls into theGitCommandErrorpath and detection is effectively disabled for ART commits.Possible fix
-def _cherrypick_art_pull_request(gitwd: git.Repo, dest_repo: Repository, dest: GitHubBranch, - conflict_policy: str = "auto") -> None: +def _cherrypick_art_pull_request( + gitwd: git.Repo, + dest_repo: Repository, + dest: GitHubBranch, + source_branch: str, + conflict_policy: str = "auto", +) -> None: @@ _safe_cherry_pick( gitwd=gitwd, sha=commit.sha, - source_branch=dest.branch, + source_branch=source_branch, conflict_policy=conflict_policy, commit_description=f"ART PR commit {commit.sha}" )- _cherrypick_art_pull_request(gitwd, dest_repo, dest, conflict_policy) + _cherrypick_art_pull_request( + gitwd, dest_repo, dest, source.branch, conflict_policy + )Also applies to: 963-963
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/bot.py` around lines 542 - 570, The ART cherry-pick is passing dest.branch into _safe_cherry_pick so conflict checks look for source/<dest.branch>; change the call in _cherrypick_art_pull_request to pass the PR's source/upstream branch (use pull_request.head.ref) as source_branch instead of dest.branch so _safe_cherry_pick inspects source/<pr-branch>. Also update the other occurrence referenced (around line 963) to the same pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rebasebot/bot.py`:
- Around line 287-292: The current try/except around
git.show(f"{source_ref}:{f}") and git.show(f"HEAD:{f}") swallows GitCommandError
for either side; change it to call the two git.show calls separately so you can
detect the case where upstream_content exists but current_content (HEAD:{f})
raises GitCommandError — treat that as content-loss (e.g., set current_content =
"" or mark the file as lost so downstream delete/rename that won the merge is
considered content loss) instead of continuing; update the logic that uses
upstream_content/current_content (the code handling f after this block) to
handle the empty-current or “lost” flag and trigger warn/strict accordingly.
---
Outside diff comments:
In `@rebasebot/bot.py`:
- Around line 542-570: The ART cherry-pick is passing dest.branch into
_safe_cherry_pick so conflict checks look for source/<dest.branch>; change the
call in _cherrypick_art_pull_request to pass the PR's source/upstream branch
(use pull_request.head.ref) as source_branch instead of dest.branch so
_safe_cherry_pick inspects source/<pr-branch>. Also update the other occurrence
referenced (around line 963) to the same pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a684f2b3-9a8e-4cc1-bbdd-871071849c51
📒 Files selected for processing (4)
rebasebot/bot.pyrebasebot/cli.pytests/test_conflict_policy.pytests/test_rebases.py
Comment on lines
+287
to
+292
| try: | ||
| upstream_content = gitwd.git.show(f"{source_ref}:{f}") | ||
| current_content = gitwd.git.show(f"HEAD:{f}") | ||
| except git.GitCommandError: | ||
| # File doesn't exist on one side — not a content loss scenario | ||
| continue |
There was a problem hiding this comment.
Handle deleted conflicted files as content loss.
If HEAD:{file} no longer exists after -Xtheirs, this currently gets skipped as “not a content loss scenario”. That misses the exact case where a downstream delete/rename wins a conflict and drops all upstream content from the file, so warn/strict will silently pass.
Possible fix
for f in files_to_check:
try:
upstream_content = gitwd.git.show(f"{source_ref}:{f}")
- current_content = gitwd.git.show(f"HEAD:{f}")
except git.GitCommandError:
- # File doesn't exist on one side — not a content loss scenario
continue
+
+ try:
+ current_content = gitwd.git.show(f"HEAD:{f}")
+ except git.GitCommandError:
+ # The file existed upstream but not after the cherry-pick.
+ results.append((f, upstream_content.splitlines()))
+ continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rebasebot/bot.py` around lines 287 - 292, The current try/except around
git.show(f"{source_ref}:{f}") and git.show(f"HEAD:{f}") swallows GitCommandError
for either side; change it to call the two git.show calls separately so you can
detect the case where upstream_content exists but current_content (HEAD:{f})
raises GitCommandError — treat that as content-loss (e.g., set current_content =
"" or mark the file as lost so downstream delete/rename that won the merge is
considered content loss) instead of continuing; update the logic that uses
upstream_content/current_content (the code handling f after this block) to
handle the empty-current or “lost” flag and trigger warn/strict accordingly.
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
4 participants
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.
Fixes #77
When cherry-picking downstream carry patches, -Xtheirs silently resolves content conflicts by favoring the downstream version, which can drop upstream additions without any warning.
Add a new --conflict-policy flag with three modes:
The detection compares each cherry-picked file against the upstream version to find missing lines after -Xtheirs resolution.