By the way, I got some questions from some contributors. "If I approve a PR, and the author is pushing new changes, should I re-approve the PR ?"
The answer is no, your previous approval is still there. For instance, on this PR https://github.com/apache/polaris/pull/1259, Eric approved, after that I pushed a new change, Eric's approval is still there. Optionally, we can choose to dismiss stale pull request approvals when commits are pushed that affect the diff in the pull request. GitHub records the state of the diff at the point when a pull request is approved. This state represents the set of changes that the reviewer approved. If the diff changes from this state (for example, because a contributor pushes new changes to the pull request branch or clicks Update branch, or because a related pull request is merged into the target branch), the approving review is dismissed as stale, and the pull request cannot be merged until someone approves the work again. If we want to "force" re-approval on change, we should use dismiss_stale_reviews: true in .asf.yaml. See https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-pull-request-reviews-before-merging for details. I can change the repo configuration if needed. Regards JB On Thu, Mar 27, 2025 at 8:28 AM Eric Maynard <eric.w.mayn...@gmail.com> wrote: > > Sounds good to me! > > On Wed, Mar 26, 2025 at 11:42 PM Jean-Baptiste Onofré <j...@nanthrax.net> > wrote: > > > Hi Eric > > > > I agree. I propose: > > > > 1. To update the contributor good practices on the website to clearly state > > the keywords nit and minor. > > 2. For request change, we can use it. For the record, we can bypass request > > change flag if a given number of reviewers approve the PR anyway. > > > > Thoughts ? > > > > Regards > > JB > > > > Le mar. 25 mars 2025 à 19:54, Eric Maynard <eric.w.mayn...@gmail.com> a > > écrit : > > > > > In this case I think rather than a mistake, there could simply be some > > > ambiguity around what comments are considered blocking. For example, I'm > > > not sure if a comment prefixed "minor" should be considered blocking -- I > > > would say probably not, but someone else could interpret it differently. > > > > > > To that end, I think putting these things in the guidelines could be wise > > > even if there's not a strict enforcement of the guidelines as such. I > > agree > > > that it's hard to "codify a spirit", but it could be useful to have > > > explicit expectations and guidelines written down somewhere. It could be > > as > > > simple as asking reviewers to "Request Changes" if they want to block a > > PR, > > > or advising authors to wait a couple of days before merging a PR they > > view > > > as complex. > > > > > > --EM > > > > > > On Mon, Mar 24, 2025 at 11:13 AM Yufei Gu <flyrain...@gmail.com> wrote: > > > > > > > I'm particularly concerned about what happened in PR #1230( > > > > https://github.com/apache/polaris/pull/1230). The PR was merged > > despite > > > > multiple unresolved comments from a committer. > > > > > > > > To maintain a healthy review process, we’d like to encourage PR authors > > > to > > > > address open comments and work toward consensus. If necessary, relying > > on > > > > lazy consensus is fine. > > > > > > > > Let’s all try to be mindful of this going forward to ensure smooth > > > > collaboration. > > > > > > > > Yufei > > > > > > > > > > > > On Mon, Mar 24, 2025 at 1:55 AM Jean-Baptiste Onofré <j...@nanthrax.net> > > > > wrote: > > > > > > > > > Hi Eric, > > > > > > > > > > Thanks for pointing this out. > > > > > > > > > > I think that's unfortunate, especially for #1230 (I don't see issues > > > > > on #1226 and #1220 as they have been approved by 3 different > > > > > committers). > > > > > > > > > > I strongly believe that, as a community, we do a great work all > > > > > together, and we highly consider comments from each other. > > > > > If again, I think it's unfortunate for #1230, it's certainly a > > > > > "mistake" from the author/merger and we should still give a change to > > > > > our soft rules (before being stricter). > > > > > > > > > > So, I propose to continue with our soft rules and good intentions, > > and > > > > > if it doesn't work, then we can discuss about a stricter approach. > > > > > > > > > > Thoughts ? > > > > > > > > > > Regards > > > > > JB > > > > > > > > > > On Sun, Mar 23, 2025 at 8:05 AM Eric Maynard < > > eric.w.mayn...@gmail.com > > > > > > > > > wrote: > > > > > > > > > > > > Revisiting this thread, I wonder if the "soft rules" are working. I > > > > have > > > > > > noticed quite a few PRs merged recently with outstanding comments. > > > The > > > > > most > > > > > > recent of these that I personally reviewed are #1220 > > > > > > <https://github.com/apache/polaris/pull/1220>, #1226 > > > > > > <https://github.com/apache/polaris/pull/1226>, and #1230 > > > > > > <https://github.com/apache/polaris/pull/1230> but there are > > > doubtless > > > > > other > > > > > > examples. > > > > > > > > > > > > If, indeed, the guidelines are not working perhaps stricter > > > enforcement > > > > > > more consistent with JB's initial proposal would be effective. > > > > > > > > > > > > On Fri, Jan 24, 2025 at 8:21 AM Dmitri Bourlatchkov > > > > > > <dmitri.bourlatch...@dremio.com.invalid> wrote: > > > > > > > > > > > > > On Thu, Jan 23, 2025 at 5:15 AM Jean-Baptiste Onofré < > > > > j...@nanthrax.net> > > > > > > > wrote: > > > > > > > > > > > > > > > Let's rename from "guidelines" to "good practices & advices" :) > > > > > > > > > > > > > > > > > > > > > > +1 to that. I'm not sure it worth trying to "codify a spirit" > > (from > > > > > > > previous emails), but > > > > > > > I think having good advice is helpful to instill the spirit of > > > > > goodwill and > > > > > > > collaboration > > > > > > > into the community. > > > > > > > > > > > > > > In this regard: > > > > > > > > - I propose to create a PR to update the contributing guide > > with > > > > this > > > > > > > > good practices discussed in this thread > > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > - I propose to close https://github.com/apache/polaris/pull/840 > > in > > > > > > > > favor of "soft rule" (we quickly discuss about owner review > > with > > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > CODEOWNERS in this PR, but let's keep aside for now) > > > > > > > > - I propose to update > > https://github.com/apache/polaris/pull/839 > > > . > > > > We > > > > > > > > have a consensus about the label, but no consensus about the > > > > schedule > > > > > > > > for now. In order to move forward, I propose to update the PR > > > with > > > > > > > > just the label for now (as we have a consensus) and see if it's > > > > > > > > enough. If it's still a concern, I will create a new PR to > > update > > > > the > > > > > > > > schedule > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > Cheers, > > > > > > > Dmitri. > > > > > > > > > > > > > > > > > > > > >
