-
Notifications
You must be signed in to change notification settings - Fork 999
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
.github/workflows: Add workflow to address merge conflicts #2958
Closed
Closed
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b912eaf
.github/workflows: Add workflow to address merge conflicts
jxs d7dfa3b
.github/workflows: remove event user, which is not available
jxs bf13fe1
.github/workflows: remove on push event,
jxs f426cce
.github/workflows: add push event back
jxs 57c86bb
.github/workflows: refer to eps1lon/actions-label-merge-conflict via …
jxs 1fcdd17
Merge branch 'master' of github.com:libp2p/rust-libp2p into add-merge…
jxs 1a132e1
Merge branch 'master' into add-merge-conflict-action
jxs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| name: "Check merge conflicts" | ||
| on: | ||
| push: | ||
| pull_request_target: | ||
| types: [synchronize] | ||
|
|
||
| jobs: | ||
| main: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: check if prs are dirty | ||
| uses: eps1lon/actions-label-merge-conflict@releases/2.x | ||
| with: | ||
| dirtyLabel: "merge-conflicts" | ||
| repoToken: "${{ secrets.GITHUB_TOKEN }}" | ||
| commentOnDirty: "Hey ${{ github.event.pull_request.user.login }} This pull request has conflicts, please resolve the merge conflicts with the base branch before we can continue reviewing. Thanks!" | ||
elenaf9 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need
push?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There won't be an
github.event.pull_requestforpushevents!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This action runs on every push and checks all pending PR's for merge conflicts, see https://github.com/jxs/rust-libp2p/actions/runs/3158064326/jobs/5139673243
Yeah found out here 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do we need to run it on
push?Shouldn't
synchronizehappen every time HEAD or base changes? I guess one is always subscribed to a PR that you created so we don't necessarily need to tag the author, I'd be kinda nice though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an open point for me @jxs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that is a bit of a concern, we usually have lots of PRs open. @elenaf9 @mxinden @jxs Could I get your opinion on #3010? Happy for you to also suggest other tools but mergify could give us quite a bit of automation like closing stale PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, you already have a workflow for closing up stale PRs - https://github.com/libp2p/rust-libp2p/blob/master/.github/workflows/stale.yml It's currently closing only those that need author input and have no activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also checked https://github.com/eps1lon/actions-label-merge-conflict/blob/main/sources/main.ts. It doesn't check out each PR branch. It uses the API to check on the mergeability status of all PRs. So the approach they're taking seems correct and it should work just fine for this repo. We can investigate further and contribute back to the project if it turns out it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nice then, thanks @galargh!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! haha
I wasn't aware at all and should probably go buy some glasses as I've completely overlooked that one all this time when I was working on the workflows!
We also don't really seem to use it / the label. We should probably keep that one then and just remove the label requirement so we don't have to explicitly tag PRs for it to run.