You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/08/28 17:28:11 UTC
[PR] chore: require an approving review before merging [datafusion-ballista]
andygrove opened a new pull request, #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394
# Which issue does this PR close?
N/A
# Rationale for this change
`main` in this repo can currently be merged into without any approving review, which is different from `apache/datafusion` and `apache/datafusion-comet`, where both require one approval.
The reason is that Ballista never had review requirements configured. The only protection on `main` today comes from the ruleset that ASF Infra added in c4336a83d ("Set up default protection ruleset for default and release branches"), which is a fleet-wide default that only sets `restrict_deletion` and `restrict_force_push`. Checking the live repository settings confirms this:
- `GET /repos/apache/datafusion-ballista/rulesets/16498601` lists exactly two rules, `deletion` and `non_fast_forward`
- `GET /repos/apache/datafusion-ballista/branches/main/protection` returns 404, so there is no classic branch protection at all
DataFusion and Comet get their approval requirement from a `protected_branches` block in `.asf.yaml`, which Ballista does not have.
# What changes are included in this PR?
Adds a `protected_branches` block to `.asf.yaml` setting `required_approving_review_count: 1` on `main`, `branch-53` and `branch-54`. This is the same mechanism `apache/datafusion-comet` uses.
A few notes for reviewers:
- The existing Infra-managed ruleset is left in place. Legacy `protected_branches` and the newer `rulesets` are separate mechanisms and apply cumulatively, so deletion and force-push stay restricted. `apache/airflow` runs both side by side today.
- `.asf.yaml` does not support wildcards in `protected_branches`, so release branches have to be listed by name. I only listed the two active ones rather than every historical `branch-*`. Happy to add more if people would prefer to match Comet's approach of listing all of them.
- No `required_status_checks` are added here, so this only requires an approval, not a green CI run. Comet does the same. That can be a follow-up if there is appetite for it.
- This does mean committers can no longer merge their own PRs unreviewed, including small dependency bumps, so it is worth a quick sanity check that people are happy with that tradeoff. `allow_auto_merge` is already enabled, which takes some of the sting out of it.
# Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: require an approving review before merging [datafusion-ballista]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394#issuecomment-5462924886
> Although I do not disagree with this PR I would like to raise a concern
>
> I don't think we can compare ballista and datafusion / comet, yet. If we had this rule enabled few years back ballista would probably die off, as there was just not enough interest for reviews. It is great to see renewed interest lately but I believe we still have too few committers for such hard requirement, and as review capacity is still left to be desired approvals from involved contributors are still valuable way to get the code merged (which this PR blocks)
>
> This PR is not just a config change, this is a WOW style change and should have been left open for a bit more than 6 minutes. Urgency this has been done left me wondering why. Since we got back somewhat working community I don't recall "merge-than-review" privilege (ab)used, to remove it so promptly, correct me if I'm wrong
>
> Please do not get me wrong @andygrove and @avantgardnerio there is nobody happier to see ballista alive again and all of you back.
Sorry @milenkovicm. You are right, this deserved more opportunity for discussion. I'm happy to revert (or approve your revert PR if you want to create one). I created this PR after accidentally merging one of my own PRs that had no approvals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: require an approving review before merging [datafusion-ballista]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on PR #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394#issuecomment-5482802063
I'm late to this party, but I'll stick with the 24 hour rule too :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: require an approving review before merging [datafusion-ballista]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: require an approving review before merging [datafusion-ballista]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394#issuecomment-5462944332
> > Although I do not disagree with this PR I would like to raise a concern
> > I don't think we can compare ballista and datafusion / comet, yet. If we had this rule enabled few years back ballista would probably die off, as there was just not enough interest for reviews. It is great to see renewed interest lately but I believe we still have too few committers for such hard requirement, and as review capacity is still left to be desired approvals from involved contributors are still valuable way to get the code merged (which this PR blocks)
> > This PR is not just a config change, this is a WOW style change and should have been left open for a bit more than 6 minutes. Urgency this has been done left me wondering why. Since we got back somewhat working community I don't recall "merge-than-review" privilege (ab)used, to remove it so promptly, correct me if I'm wrong
> > Please do not get me wrong @andygrove and @avantgardnerio there is nobody happier to see ballista alive again and all of you back.
>
> Sorry @milenkovicm. You are right, this deserved more opportunity for discussion. I'm happy to revert (or approve your revert PR if you want to create one). I created this PR after accidentally merging one of my own PRs that had no approvals.
DataFusion contribution guide suggests leaving major PRs open for at least 24 hours. I'll follow that going forward (I think one working day would be a better definition)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: require an approving review before merging [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394#issuecomment-5461146486
Although I do not disagree with this PR I would like to raise a concern
I don't think we can compare ballista and datafusion / comet, yet. If we had this rule enabled few years back ballista would probably die off, as there was just not enough interest for reviews. It is great to see renewed interest lately but I believe we still have too few committers for such hard requirement, and as review capacity is still left to be desired approvals from involved contributors are still valuable way to get the code merged (which this PR blocks)
This PR is not just a config change, this is a WOW style change and should have been left open for a bit more than 6 minutes. Urgency this has been done left me wondering why. Since we got back somewhat working community I don't recall "merge-than-review" privilege (ab)used, to remove it so promptly, correct me if I'm wrong
Please do not get me wrong @andygrove and @avantgardnerio there is nobody happier to see ballista alive again and all of you back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: require an approving review before merging [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #2394:
URL: https://github.com/apache/datafusion-ballista/pull/2394#issuecomment-5462982255
Thanks for understanding @andygrove. let's give it a try and see how it goes, we can always revert it later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org