By the way, I propose to set dismiss_stale_review to false. It should keep the approval and not in stale mode. https://github.com/apache/infrastructure-asfyaml?tab=readme-ov-file#branchpro
I will submit a PR for that. Regards JB On Thu, Mar 27, 2025 at 10:43 AM Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > >
