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

Auto-merge PRs after approve at the end of Functional Tests #16078

Open
Piedone opened this issue May 16, 2024 · 10 comments
Open

Auto-merge PRs after approve at the end of Functional Tests #16078

Piedone opened this issue May 16, 2024 · 10 comments

Comments

@Piedone
Copy link
Member

Piedone commented May 16, 2024

Is your feature request related to a problem? Please describe.

Functional Tests run after PR feedback submissions. In case you as a reviewer approve a PR, and you don't need a second opinion, you need to wait around for the tests to finish before merging the PR.

Describe the solution you'd like

Change the functional_all_db workflow to merge the PR if all of the following are true:

  • The trigger was pull_request_review.
  • All reviewers approved (i.e. there are no reviews with "changed requested" open).
  • There's no "don't merge" label on the PR.
  • The PR is not a draft one.

Describe alternatives you've considered

GitHub's auto-merge can do this, but for that, Functional Tests would need to be required checks, i.e. mandatorily run for every PR (they currently don't run for some paths).

@MikeAlhayek
Copy link
Member

I rather not see this. Sometimes we approve something but looking for a second approval.

@Piedone
Copy link
Member Author

Piedone commented May 16, 2024

The PR should then have the "don't merge" label to make this explicit.

@MikeAlhayek
Copy link
Member

don't merge to prevent merging. we don't want to start adding "don't merge" just to would around this automation. If one thinks the PR looks good, they should approve it and or merge it. If they don't merge it, they are likely waiting a second approval. so if a second person wants, they can merge it.

@Piedone
Copy link
Member Author

Piedone commented May 16, 2024

My problem is, also regardless of this issue, that if you're waiting for a second approval, you should make that explicit. I asked around under PRs sitting there with approvals, gathering merge conflicts, before too, due to the lack of this.

@MikeAlhayek
Copy link
Member

I understand. But I think auto merging is a bad idea and may solve an issue but introduce another.

@Piedone
Copy link
Member Author

Piedone commented May 16, 2024

That very well be, of course.

@Piedone
Copy link
Member Author

Piedone commented May 17, 2024

How about having a "merge when functional tests succeeded" label to ask for auto-merge explicitly?

@MikeAlhayek
Copy link
Member

No need for that. We already have to "Auto-merge" which will merge the PR once everything passes.

@Piedone
Copy link
Member Author

Piedone commented May 17, 2024

As mentioned above, Functional Tests would need to be required checks, i.e. mandatorily run for every PR for this (they currently don't run for some paths).

Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants