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

feat: add option to choose between force and force-with-lease #1187

Closed
wants to merge 1 commit into from

Conversation

leeopop
Copy link
Contributor

@leeopop leeopop commented May 18, 2022

When using this plugin with a build bot, the build bot (e.g. bors) creates another commit that references the PR branch.
This forbids updating the PR branch through --force-with-lease push option.

This PR adds a boolean option to choose between force-with-lease and force.

Before

image

After

image

@leeopop leeopop force-pushed the feature/push-force-option branch from 527839b to e4c0013 Compare May 18, 2022 06:46
@peter-evans
Copy link
Owner

Hi @leeopop

Thanks for the effort to contribute.

I would like to understand the problem better before deciding on the solution. Could you explain in detail exactly what is happening in this scenario:

When using this plugin with a build bot, the build bot (e.g. bors) creates another commit that references the PR branch.

  • Where does it make the commit?
  • How does it make it? (example workflow?)
  • Do you have an example with git commands?

@leeopop
Copy link
Contributor Author

leeopop commented May 25, 2022

I think my previous comment was unrelated.

@leeopop
Copy link
Contributor Author

leeopop commented May 25, 2022

  • Where does it make the commit?

A temporal branch (default name: staging). It refers to merge (latest master) + PR. This snapshot is created just before the PR is scheduled to be merged.

  • How does it make it? (example workflow?)

When several opened & approved PRs are being merged, we mark the PRs by commenting bors r+.
Then the bot will schedule the PRs and ensure that one PR is being merged at the same time.

PR3 may be merged as either master + PR1 + PR3 or master + PR4 + PR5 + PR3, depending on the scheduling result.

  • Do you have an example with git commands?

I'll prepare this in the following comment.

@leeopop
Copy link
Contributor Author

leeopop commented May 25, 2022

@peter-evans While preparing the git example, I just realized that I put the "push" trigger to this plugin, and multiple workers tried to update the same branch.

The automated build bot created 2 to 3 consequent pushes at the same time, thus the short time (10 to 20 sec) while this plugin is trying to update the branch, there were two plugins running at the same time.

Force push does not seem to be a solution to this case.

BTW, here is the example scenario I was trying to demonstrate.
This was expected to be fail, but I found that it works actually.

# Init remote repo
mkdir test
cd test
git init --bare

# Clone
cd ..
git clone test test2
cd test2
touch README.md
git add .
git commit -m "init"

# Create AUTO PR
git branch PR-auto
git checkout PR-auto
echo "auto PRv1" > AUTO.md
git add .
git commit -m "auto PRv1"
git push --force-with-lease -u origin PR-auto
git checkout master
git branch -D PR-auto

# Create a "trying" branch via a build bot
MERGE_BRANCH="PR-auto"
git branch bors-trying master
git checkout bors-trying
git fetch
git merge --no-ff -m "auto merge" "origin/$MERGE_BRANCH"
git push --force-with-lease -u origin bors-trying
git checkout master
git branch -D bors-trying

# Another manual PR is created...
git branch PR-manual
git checkout PR-manual
echo "manual PRv1" >> README.md
git add .
git commit -m "manual PRv1"
git push --force-with-lease -u origin PR-manual
git checkout master
git branch -D PR-manual

# ... and being merged by the build bot
MERGE_BRANCH="PR-manual"
git branch bors-staging master
git checkout bors-staging
git fetch
git merge --no-ff -m "auto merge" "origin/$MERGE_BRANCH"
git push --force-with-lease -u origin bors-staging
# Assume that test has succeeded
git branch -D master
git branch master
git push --force-with-lease -u origin master
git branch -D $MERGE_BRANCH
git push origin --delete $MERGE_BRANCH

# Auto PR checks whether the PR should be updated.
# Force update it if needed
git branch PR-auto master
git checkout PR-auto
echo "auto PRv1" > AUTO.md
git add .
git commit -m "auto PRv1"
git diff PR-auto origin/PR-auto
git push --force-with-lease -u origin PR-auto
git checkout master
git branch -D PR-auto

Few days ago, I removed the "push" trigger and only using the cron trigger.
I'll observe few days to see if this solves my problem. If it solves, then this PR will just cause more unexpected behavior.

However, I think this behavior should be documented in the front page.
(e.g. Do not run this plugin simultaneously unless your are using randomized branch names)

@leeopop leeopop closed this May 25, 2022
@stevelacey
Copy link

I am also running into this issue and a --force option would be great.

I am using a push hook to build out release notes in a changelog, and it doesn't matter if a simultaneous run is getting clobbered, they'll produce the same results.

@leeopop
Copy link
Contributor Author

leeopop commented Aug 21, 2022

@stevelacey In my case, #1189 was the root cause and solved my problem.
Would it work with yours?

@stevelacey
Copy link

stevelacey commented Aug 22, 2022

@leeopop I went for adjusting my workflow's concurrency settings instead:

concurrency: 
  group: ${{ github.workflow }}
  cancel-in-progress: true

Saves on action minutes too 🤘🏼

@peter-evans
Copy link
Owner

Glad you've both resolved the problem for your use cases.

Both of these cases are actually confirmation that using force-with-lease is correct and provides safety. Multiple workflows should not be trying to modify the same branch concurrently.

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.

None yet

3 participants