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

.github/workflows: Add workflow to address merge conflicts #2958

Closed
wants to merge 7 commits into from

Conversation

jxs
Copy link
Member

@jxs jxs commented Sep 29, 2022

Description

uses https://github.com/eps1lon/actions-label-merge-conflict

Links to any relevant issues

addresses #2948

Open Questions

I did not test the ${{ github.event.pull_request.user.login } is there an easier way to test the workflow than activating actions on my fork and replicating a real merge conflict situation?

Thanks!

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger
Copy link
Contributor

I did not test the ${{ github.event.pull_request.user.login } is there an easier way to test the workflow than activating actions on my fork and replicating a real merge conflict situation?

Not that I know but doing things on my fork is how I usually test them. By default there are no branch protections so you can just (force) push to master which is typically a fairly quick way of testing these kinds of things :)

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I'd like to get @mxinden's and @elenaf9's opinion on whether or not we actually want this. I am definitely in favor of not having to baby-sit PRs myself but being able to rely on my notification inbox for this :)

Can you test whether this will created repeated comments on subsequent runs?

@@ -0,0 +1,16 @@
name: "Check merge conflicts"
on:
push:
Copy link
Contributor

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?

Copy link
Contributor

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_request for push events!

Copy link
Member Author

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?

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

There won't be an github.event.pull_request for push events!

Yeah found out here 😅

Copy link
Contributor

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 synchronize happen 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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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!

Copy link
Contributor

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 - master/.github/workflows/stale.yml It's currently closing only those that need author input and have no activity.

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.

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 in favor of this. Thanks @jxs!

.github/workflows/check-merge-conflicts.yml Outdated Show resolved Hide resolved
@jxs
Copy link
Member Author

jxs commented Sep 30, 2022

Thanks!

I'd like to get @mxinden's and @elenaf9's opinion on whether or not we actually want this. I am definitely in favor of not having to baby-sit PRs myself but being able to rely on my notification inbox for this :)

Can you test whether this will created repeated comments on subsequent runs?

yeah sure :)
It seems so, everytime there's a push to a branch the action runs and checks the merge conflicts,. If PR's have merge conflicts it comments there and adds the label. Whenever there's a new push it checks again and if merge conflicts are resolved it removes the label, and keeps doing that for the open PR's.
See here it added the label and the comment, then removed the label, then added it again and a new comment.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of this in general and the specific solution proposed in particular. Thanks @jxs for working on it.

Let me know once this is ready to merge @jxs and @thomaseizinger.

//CC @galargh in case you have opinions or want us to be consistent with solutions used in other repositories.

@galargh
Copy link
Contributor

galargh commented Oct 12, 2022

Thanks for the tag @mxinden. Nothing to add on my side really. I don't know of any other repositories using it. Seems like a fair way of getting an extra notif on merge conflicts.

Would be great if we could build up a library of solutions to common problems like this. For lack of a better place for now, I added it as an entry here: https://www.notion.so/pl-strflt/IPDX-122073392dce454e9ca4b87231034483#cfcfcfca010f4029bfb0d992cd44bf06

it's redundant since we have pull_request_target
@jxs jxs force-pushed the add-merge-conflict-action branch 2 times, most recently from f3f9e2b to bf13fe1 Compare October 12, 2022 11:42
@jxs jxs force-pushed the add-merge-conflict-action branch from 8df09e3 to 1fcdd17 Compare October 12, 2022 15:55
@thomaseizinger
Copy link
Contributor

If we end up doing #3010, then we could also use mergify for this: https://docs.mergify.com/actions/comment/#request-for-action

@jxs
Copy link
Member Author

jxs commented Oct 14, 2022

closed in favor of #3026

@jxs jxs closed this Oct 14, 2022
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.

5 participants