Skip to content

feat: ping PR author on merge conflicts #1027

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 29, 2024
Merged

feat: ping PR author on merge conflicts #1027

merged 1 commit into from
Apr 29, 2024

Conversation

leseb
Copy link
Contributor

@leseb leseb commented Apr 29, 2024

From now on, if a pull request has merge conflicts and needs a rebase, the author will be pinged.

Signed-off-by: Sébastien Han seb@redhat.com

@leseb leseb force-pushed the mergify-conflict branch 3 times, most recently from 75474f3 to ea823b9 Compare April 29, 2024 09:29
@leseb
Copy link
Contributor Author

leseb commented Apr 29, 2024

@russellb PTAL

russellb

This comment was marked as duplicate.

Sign in to view

russellb
russellb requested changes Apr 29, 2024
View reviewed changes
Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

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

I like this, thanks. A couple things I’d like to add

  • add a needs-rebase label
  • Another rule that removes that label when a PR is not in conflict

I have config for this in another repo I can add here later, but if you’d like to add that’s even better!

@leseb
Copy link
Contributor Author

leseb commented Apr 29, 2024

I like this, thanks. A couple things I’d like to add

  • add a needs-rebase label
  • Another rule that removes that label when a PR is not in conflict

I have config for this in another repo I can add here later, but if you’d like to add that’s even better!

Can do!

@leseb leseb force-pushed the mergify-conflict branch from ea823b9 to 4b0aa7a Compare April 29, 2024 11:27
@leseb
Copy link
Contributor Author

leseb commented Apr 29, 2024

@russellb the 'needs-rebase' label needs to be created. Additionally, should we remove 'DRAFT - DO NOT MERGE'?

@leseb leseb requested a review from russellb April 29, 2024 11:28
@leseb
feat: ping PR author on merge conflicts
4b0aa7a 
From now on, if a pull request has merge conflicts and needs a rebase,
  * the author will be pinged
  * a 'need-rebase' label will be added

When conflicts are resolved the label will be removed.

Signed-off-by: Sébastien Han <seb@redhat.com>
russellb
russellb reviewed Apr 29, 2024
View reviewed changes
.github/mergify.yml
@@ -8,6 +30,7 @@ pull_request_rules:
- base=main
- label!=hold
- label!=do-not-merge
- label!=needs-rebase
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't hurt, but I don't think it's necessary. Or is there a case in which you think this would prevent a merge that otherwise would happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about the case where the PR has been approved and something creating a conflict gets merged? I don't think merge queues are enabled in the current configuration so this scenario might happen right?

Or perhaps we just rely on GitHub's configuration to block merge on conflict?

@russellb
Copy link
Member

@russellb the 'needs-rebase' label needs to be created.

Done.

Additionally, should we remove 'DRAFT - DO NOT MERGE'?

I'm not sure what this is referring to. Can you clarify?

@leseb
Copy link
Contributor Author

leseb commented Apr 29, 2024

@russellb the 'needs-rebase' label needs to be created.

Done.

Additionally, should we remove 'DRAFT - DO NOT MERGE'?

I'm not sure what this is referring to. Can you clarify?

I saw that label, and I was wondering if we should keep it or not.

@russellb
Copy link
Member

@russellb the 'needs-rebase' label needs to be created.

Done.

Additionally, should we remove 'DRAFT - DO NOT MERGE'?

I'm not sure what this is referring to. Can you clarify?

I saw that label, and I was wondering if we should keep it or not.

Oh, yeah, that's probably not necessary. There's also 'hold'. It looks like someone renamed do-not-merge to that, which breaks the mergify configuration.

@leseb leseb requested a review from russellb April 29, 2024 13:24
@russellb russellb merged commit 522f4c8 into instructlab:main Apr 29, 2024
@leseb leseb deleted the mergify-conflict branch April 30, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russellb russellb russellb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@leseb @russellb