Skip to content

maintainer-approval: count only each reviewer's latest review state - #6297

Open
TangoEnSkai wants to merge 1 commit into
databricks:mainfrom
TangoEnSkai:fix/maintainer-approval-latest-review-state
Open

maintainer-approval: count only each reviewer's latest review state#6297
TangoEnSkai wants to merge 1 commit into
databricks:mainfrom
TangoEnSkai:fix/maintainer-approval-latest-review-state

Conversation

@TangoEnSkai

Copy link
Copy Markdown

Changes

maintainer-approval treated approval as "has this reviewer ever left an
APPROVED review" rather than "is this reviewer's current review state
APPROVED". An approval that the same reviewer later replaced with
CHANGES_REQUESTED, or that was dismissed, kept satisfying the check.

This collapses the review history to each reviewer's most recent review
before the three places that evaluate it — maintainer approval,
maintainer-authored PR approval, and the per-path approverLogins list.
All three read the same reviews binding, so the fix is one helper plus
one reassignment.

COMMENTED reviews are skipped rather than counted as the latest state.
They carry no approval state, so a reviewer who approves and then leaves a
plain comment keeps their approval — the same way GitHub resolves its own
reviewers list. Treating them as the latest review would silently revoke
approvals instead.

Fixes #5322.

Why

The gate is supposed to reflect current approval state. Today a reviewer
who approves, spots a problem, and switches to CHANGES_REQUESTED cannot
actually withdraw the approval — the workflow still finds the historical
APPROVED record and reports success. Dismissing the review does not help
either, for the same reason.

The behaviour dates back to the original workflow in #4912 and was carried
over when per-path approval was added in #4918. #4918 added a test that
CHANGES_REQUESTED does not count as approval, but it covers only a PR
with a single CHANGES_REQUESTED review, which passes either way. The
approve-then-request-changes sequence was never covered.

Tests

node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js

Five cases added to maintainer-approval.test.js. Three fail against the
current code and pass with this change:

  • owner approves, then requests changes -> no longer approved
  • maintainer approves, then requests changes -> no longer approved
  • approval later dismissed -> no longer approved

Two guard the opposite direction and pass both before and after, so the
fix does not over-correct:

  • owner requests changes, then approves -> approved
  • owner approves, then leaves a COMMENTED review -> still approved

No changelog fragment: this changes repository CI tooling, not CLI
behaviour.

The workflow scanned the full review history for any APPROVED record, so an
approval that a reviewer later replaced with CHANGES_REQUESTED, or that was
dismissed, kept satisfying the check.

Collapse the history to each reviewer's most recent review before evaluating
maintainer approval, maintainer-authored PR approval, and per-path owner
approval. COMMENTED reviews are skipped: they carry no approval state and
leave the reviewer's previous standing intact, the same way GitHub resolves
its own reviewers list.

Closes databricks#5322
@github-actions

Copy link
Copy Markdown
Contributor

Waiting for approval

Based on git history, these people are best suited to review:

  • @simonfaltum -- recent work in .github/workflows/

Eligible reviewers: @andrewnester, @anton-107, @denik, @janniklasrose, @lennartkats-db, @pietern, @rclarey, @renaudhartert-db, @rugpanov, @shreyas-goenka

Suggestions based on git history. See OWNERS for ownership rules.

@TangoEnSkai

Copy link
Copy Markdown
Author

@simonfaltum when you have a moment — this touches the approval gate itself, so I kept the change as small as I could and led with tests that pin down the current behaviour.

Three of the five added cases fail against main today (an approval that was later replaced by CHANGES_REQUESTED, or dismissed, still reports success). The other two exist to show the fix does not over-correct: a re-approval after CHANGES_REQUESTED still counts, and a plain COMMENTED review left after an approval does not silently revoke it.

The workflow runs are action_required since this is my first PR here, so Test OWNERS scripts has not executed yet. Locally, node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js is green (42 + 27).

Happy to split the COMMENTED handling out or adjust the approach if you would rather shape this differently.

@github-actions

Copy link
Copy Markdown
Contributor

An authorized user can trigger integration tests manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 6297
  • Commit SHA: e317a9bcc5ea8321ba3b78d6e62194c6ade5cf53

Checks will be approved automatically on success.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

maintainer-approval uses historical APPROVED reviews instead of latest review state per reviewer

1 participant