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

[service] Automatically Label Pull Requests with Megre Conflicts #4725

Closed
wants to merge 18 commits into from

Conversation

prince-chrismc
Copy link
Contributor

@prince-chrismc prince-chrismc commented Feb 28, 2021

🎁 For CCI... this is probably the first step in making the comment below possible. Demo prince-chrismc#12

We cannot afford building after merging to master. When I have time, one of my priorities is to implement something that will force to update the PR before considering the merge if there are changes to the same recipe already in master... probably it is something easy for a Github Action: whenever there is a push to master, it can check the recipe being modified and label the PRs accordingly 😉

Originally posted by @jgsogo in #4577 (comment)


⚠️ Action required: Create a label

Optionally you may wish to use the @conan-center-bot PAT for the action so the changes will have that name.

Lastly there is https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-status-checks-before-merging but I am not sure how helpful it is with the CCI bot doing all the work.

@prince-chrismc
Copy link
Contributor Author

Don't mind all my test commits.

@conan-center-bot
Copy link
Collaborator

Changes not allowed in build 1:

[.github/workflows/label_conflicts.yml]

Split them in several pull requests if you are making changes in more than one folder.

@SSE4 SSE4 added the infrastructure Waiting on tools or services belonging to the infra label Mar 1, 2021
@jgsogo
Copy link
Contributor

jgsogo commented Mar 2, 2021

Hi, first of all, thanks! 🤟

This is not targeting exactly the specific problem we'd like to avoid in conan-center-index. We already require status checks to pass before merging and Github fails if you try to merge a PR with conflicts.... so, what is exactly our problem?

The problem arises when a PR can be merged (it has no conflicts), but it will create a new recipe revision when merged to master because it was not rebased. We cannot force a rebase before merging because there are so many changes in master that doesn't affect the PR and we don't want to trigger the CI for all the PRs after every merge. Github doesn't notify about this situation, the PR is perfectly mergeable and this is why we use Git, to be able to merge unrelated changes.

Our problem is that the merge might create an invalid recipe (and also the time it takes to build) even if it mergeable. To avoid these situations, usually, you can require a rebase/update before merging, and also the CI systems provide a branch indexing feature (Jenkins terminology) to trigger again the PR merging to master when the base branch changes. Again, we cannot trigger all the PRs for every commit in master and that's the reason why we need a custom solution.

The implementation should check the repository files and see if, after merging to master (at the moment just before clicking the merge button), the recipe revision computed in the PR is the same. If not, it should warn the user, or label the PR, or force-push a commit to the PR... wait for the build and try the merge again.

A Github action could do this job, adding labels like "This PR needs to be updated", our CI can check the label and ignore the PR... o trigger it again. This is different from the Conflicted label proposed here.


About this action, it is useful to add the label, but probably posting a comment in the PR that will trigger a notification to the user is also needed. wdyt?

@prince-chrismc
Copy link
Contributor Author

Ahh thank you for the detailed description, I think I better understood the problem before I had done this small project 😀 but it's more clear now. It's really eric's bot but comparing PRs to master.
Regardless thank you very much for the idea, I am happy I got to learn/practice my TypeScript and GitHub.

About this action, it is useful to add the label, but probably posting a comment in the PR that will trigger a notification to the user is also needed. wdyt?

Very first though I had. prince-chrismc/label-merge-conflicts-action#13

The labels are most helpful for bots.


I'll need time to digest this but my first reaction is this might not obtainable/practical in a GitHub action. Actions are webhooks called after something has occurred. What CCI needs is before.

The implementation should check the repository files and see if, after merging to master (at the moment just before clicking the merge button)

Since there's an unknown time that each PR is open, 1 day to 1 year, with only ~20 (0.2 to 0.25) active PRs open at any one time there would be a lot of waste calculation time given there's 5 to 10 merges a day. Not sure which organization plan you are on but that might be pricey.

The technical challenge is whether running after each change the action can detect is sufficient to keep the PRs up to date. I've flipped from not to yes to maybe trying to write this.

Would it be possible to have the bot, when considering to merge, calculate the merge_commit's recipe_revision and verify that against its last known one? This could reduce the workload since the GitHub merge strategy is not trivial to recreate.

This is a very interesting problem, I might come back with a second proposal now that the requirements are clear in my head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Failed infrastructure Waiting on tools or services belonging to the infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants