Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue with updating PR when the stack is reordered (#395) #436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wahyudibo
Copy link

@wahyudibo wahyudibo commented Oct 23, 2024

This PR addresses issue #395

Fixing Steps

  1. Handle the error returned when updating a PR before the local branch is pushed to GitHub by marking the reRunPRUpdate in GitHubPushModel as true and continue to the next iteration.
  2. After runGitPush(), check reRunPRUpdate value. If true, then re-run updatePRs again to fix the PR base branch

Before

Previously, when the branches were reordered and we ran av stack sync it would return There are no new commits between base branch 'base' and head branch 'head', and the process is stopped

After

The previous issue did not happen and the PR order is now in sync with the local machine's re-ordered stack.

Image 2024-10-23-11-28-33

@wahyudibo wahyudibo requested a review from a team as a code owner October 23, 2024 04:29
Copy link
Contributor

aviator-app bot commented Oct 23, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@aviator-app aviator-app bot requested review from draftcode and removed request for a team October 23, 2024 04:29
Copy link
Contributor

aviator-app bot commented Oct 23, 2024

FlexReview Team Status

Code Review Health

To get a faster review, consider keeping the PR under 200 lines.

  • @aviator-co/engineering: Expected response time: a dayMax modified lines: 200

Pending Team Reviews

  • @aviator-co/engineering: Assigned to @draftcode

@draftcode
Copy link
Contributor

It seems there is a bug.

set -ex

git checkout main
av stack branch branch-1
git commit --allow-empty -m "Commit on branch-1"
av pr create --title "PR on branch-1" --body "PR on branch-1"

git checkout main
av stack branch branch-2
git merge --ff-only branch-1
av pr create --title "PR on branch-2" --body "PR on branch-2"

git checkout branch-2
av stack reparent --parent branch-1

av stack sync --all --push yes

With this change, the last av stack sync exits with 0 saying it's "pushed", but it's actually failing since there's no new commit in branch-2. As a result, av stack sync becomes no-op and repeatedly running it will just trying to push, and it says it's "pushed", but it didn't. Prior to this change, it correctly errors out with there's no new commit.

@wahyudibo
Copy link
Author

Thank you for checking @draftcode , i'll try to reproduce the issue on my end and fix it

@wahyudibo
Copy link
Author

Hi @draftcode, i tried to reproduce the issue on my end, and here's what I've got:

Image 2024-10-24-16-09-41

my understanding of the issue #395 is, we have this error There are no new commits between base branch 'base' and head branch 'head' which prevents us from updating the PR when the stack is re-ordered. So my steps to fix this issue are:

  1. catch the above error into a conditional statement (if)
  2. inside the if block, remark this session to re-run pr update after git push
  3. continue to the next iteration of pr checks

but from the example above, seems you're expecting the error to appear and stop the process. So what is the condition that is required to differentiate between this case and the case from #395? because both have the same there's no new commit error but we want the process to be continued in #395 but stopped in this case. Please kindly suggest.

@draftcode
Copy link
Contributor

#436 (comment) truely do not have any new commit between the parent and its child, so it should be errored out. #395 is that depending on the order of the operations, it appears as there's no new commit between the parent and its child, but it's just a transient case.

Locally, it should be able to tell if there is no new commit or not. Traversing or doing something like #395 (comment).

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.

2 participants