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

[Bug]: when cleaning up PRs with is_pr_cleanup: true, switches to main on PR cleanup #529

Closed
1 task done
kanno41 opened this issue Jun 5, 2024 · 5 comments · Fixed by #534
Closed
1 task done
Assignees
Labels
bug Something isn't working

Comments

@kanno41
Copy link
Contributor

kanno41 commented Jun 5, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

I had an old PR existing and tried to have it clean up the old PR. However, when it cleaned up the PR, it switched the branch to main. I am using a GitHub App.
template-sync-bug

Expected Behavior

PR should be created from created chore branch

Current Behavior

See Description

Steps To Reproduce

No response

Possible Solution

unsure, theoretically is_keep_branch_on_pr_cleanup: true should work for me, but it is also has a bug (#528)

Additional Information/Context

I am using is_force_push_pr: true. I don't really want to force push, I want to delete the branch first in all cases (which is essentially a force push, I suppose).

Template sync version Version

2.2.0

@kanno41 kanno41 added the bug Something isn't working label Jun 5, 2024
@AndreasAugustin
Copy link
Owner

AndreasAugustin commented Jun 5, 2024

that is weird. Need to check it. Thanks for the report!

Update:
ok, issue seems clear related to docs https://cli.github.com/manual/gh_pr_close
gh pr close -d is also deleting the local branch. Seems I tried to be smart and failed.. 😸

Will try to provide a fix asap.

Remark
@kanno41 Can you give some more details why you are using is_force_push_pr: true? What is your use case? Actually the action won't push or create a new PR if there are no changes within the remote repository. For the force push you do not need to delete the branch first.

@AndreasAugustin
Copy link
Owner

@all-contributors please add @kanno41 for bug

Copy link
Contributor

@AndreasAugustin

I've put up a pull request to add @kanno41! 🎉

@AndreasAugustin
Copy link
Owner

@kanno41 I think I understand the issue now. It is somehow an edge case.

  • You have set is_force_push_pr: true
  • there is already a PR available with the same branch (this happens when you do not have changes within remote repository, when there are changes available, all should be fine because it will create a new branch which is not existing remote yet)
  • the gh pr close -d will delete the local branch because it has the same name like remote

As a intermediate mitigation action you can remove the is_force_push_pr: true parameter. Then at least the action does not fail anymore.

For a bug fix I need to think about some options:

  • if is_force_push_pr: true then exclude the current local branch from the deletion list
  • if is_force_push_pr: true and there are no changes within base repo -> remove the -d flag -> when there are changes available in any next run, the branch cleanup will work again
  • some other options

currently I need to think about the downsides of each option.

AndreasAugustin added a commit that referenced this issue Jun 9, 2024
Signed-off-by: Andy Augustin <dev@andreas-augustin.org>
@AndreasAugustin
Copy link
Owner

AndreasAugustin commented Jun 9, 2024

@kanno41 #534 should fix this issue. I need to test it.

You can try the solution with

      - name: actions-template-sync
        # use here the related branch name
        uses: AndreasAugustin/actions-template-sync@#529
        with:
          source_repo_path: <owner/repo>
          upstream_branch: <target_branch> # defaults to main
          pr_labels: <label1>,<label2>[,...] # defaults to template_sync
          is_force_push_pr: true
          is_pr_cleanup: true

It actually checks if the remote branch equals the local branch. If so, it skips the cleanup for that branch.

AndreasAugustin added a commit that referenced this issue Jun 16, 2024
Signed-off-by: Andy Augustin <dev@andreas-augustin.org>
AndreasAugustin added a commit that referenced this issue Jun 16, 2024
…e-sync into fix/#529

Signed-off-by: Andy Augustin <dev@andreas-augustin.org>
AndreasAugustin added a commit that referenced this issue Jun 16, 2024
* fix(#529): 🐛 edge case with branch cleanup and force push

---------

Signed-off-by: Andy Augustin <dev@andreas-augustin.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants