I created https://github.com/apache/infrastructure-asfyaml/pull/55
Long story short (after investigation): we have a specific
configuration on Polaris GitHub repo ("Require Last Push Approval")
that is coming from our previous GitHub repository (as reminder, when
Polaris has been accepted into the incubator, we "moved" our GitHub
repo to ASF, we didn't start from a fresh repo). As this configuration
was not "supported" by .asf.yaml, it was not "overwritten" by
.asf.yaml.
I added the support of require_last_push_approval to .asf.yaml to
"control" this configuration.
I gonna create a PR to test this configuration on Polaris ;)
Regards
JB
On Thu, Mar 27, 2025 at 2:10 PM Jean-Baptiste Onofré <j...@nanthrax.net> wrote:
>
> Quick update: I had a quick chat with Daniel (from Infra) and it seems
> that there's a new "configuration" added recently by GitHub.
> We are updating asf.yaml to support this new feature and I will test.
>
> I will keep you posted :)
>
> Regards
> JB
>
> On Thu, Mar 27, 2025 at 1:05 PM Jean-Baptiste Onofré <j...@nanthrax.net>
> wrote:
> >
> > 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.
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
