From 08431ca9e79415ac1ba398b2d8b7bcd9c59baffe Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 22 Sep 2026 11:06:27 -0700 Subject: [PATCH 1/2] feat(ci): enable GitHub merge queue for master Adds a `github.rulesets` entry to .asf.yaml turning on GitHub's merge queue for master, alongside the existing classic `protected_branches` config (the two mechanisms coexist; classic protection still gates PR-entry-into-queue readiness, the new ruleset adds the merge_queue rule and its own required_status_checks that must pass against the queue's synthetic merge-group ref before a batch actually merges). Wires `merge_group:` support into every workflow that produces one of those required checks, since GitHub does not run a workflow against a merge-group ref unless it explicitly listens for that event -- without this, every queued PR would time out waiting for statuses that never get reported: - enforce-single-migration-head.yml: the file-list-based skip optimization can't work for merge_group (no pull_request context to list files from), so it now always runs the real check there, matching the file's own existing fail-closed philosophy -- this is also exactly the scenario the check exists for (two migration heads that each looked fine alone, combined by the queue). - pre-commit.yml: the changed-files diff base for merge_group is read from the event's own explicit `merge_group.base_sha` rather than assuming its commit has the same two-parent shape pull_request's synthetic merge commit does. - scripts/change_detector.py (used by superset-frontend.yml, superset-e2e.yml, and all three python test workflows): the change detector's event-type dispatch previously raised on anything but pull_request/push/workflow_dispatch/schedule; added a merge_group branch that diffs against `merge_group.base_sha` via a new shared `fetch_changed_files_compare()` helper (also now used by the existing push path, no behavior change there). Two required checks are deliberately left without merge_group support and excluded from the new ruleset's required_status_checks, staying required only via the existing classic config (i.e. still gate a PR before a committer can add it to the queue, just don't re-run once it's in one): - lint-check: validates the PR's own title, which doesn't mean anything once re-run against a merge-group ref. - dependency-review: actions/dependency-review-action has a confirmed, currently-unfixed bug on merge_group events (destructures github.event.pull_request, which doesn't exist there, and crashes -- see actions/dependency-review-action#843). Uses the raw ruleset payload syntax rather than the nicer `merge_queue` convenience key, since that key is still pending upstream in apache/infrastructure-asfyaml#119 (not yet merged/deployed as of this writing) -- .asf.yaml can move to the convenience syntax once that lands. - `pytest tests/unit_tests/scripts/change_detector_test.py` -- 13 passed (8 existing + 5 new: compare-helper delegation, push still delegates to it, merge_group happy path, merge_group fails closed without MERGE_GROUP_BASE_SHA). - `ruff check` / `ruff format --check` on the two touched Python files -- clean. - `zizmor` (the same GHA security linter this repo's `validate-all-ghas` check runs) against every touched workflow/action file -- no findings. - `python3 -c "import yaml; yaml.safe_load(...)"` on .asf.yaml and every touched workflow file -- all parse. - Parsed .asf.yaml back with PyYAML to confirm `rulesets` landed as a sibling of `protected_branches`, not nested inside it, and printed the parsed ruleset structure to confirm the exact shape GitHub's Rulesets API expects. - Could not run pre-commit itself end-to-end -- its git-diffing is broken in this environment on an unrelated host xcrun/libxcrun architecture mismatch (not something this commit introduces); the checks above cover what pre-commit would have run for these file types (ruff, YAML validity, zizmor). - Not yet verified against a live merge-group run (only checkable once this actually merges and a real queue entry exercises it). - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] Introduces new feature or API - [ ] Removes existing feature or API Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5 --- .asf.yaml | 89 +++++++++++++++++++ .github/actions/change-detector/action.yml | 4 + .../enforce-single-migration-head.yml | 10 ++- .github/workflows/pre-commit.yml | 9 ++ .github/workflows/superset-e2e.yml | 2 + .github/workflows/superset-frontend.yml | 2 + .../superset-python-integrationtest.yml | 2 + .../workflows/superset-python-presto-hive.yml | 2 + .../workflows/superset-python-unittest.yml | 2 + scripts/change_detector.py | 30 ++++++- .../scripts/change_detector_test.py | 72 +++++++++++++++ 11 files changed, 220 insertions(+), 4 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index b5bacfca1fed..c44eb875e0af 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -103,3 +103,92 @@ github: required_approving_review_count: 1 required_signatures: false + + # GitHub's merge queue rule type only exists on rulesets, not on the classic + # `protected_branches` API above -- the two mechanisms coexist (GitHub + # evaluates both), so this adds a merge queue on `master` without touching + # the existing classic config, which still gates PR-entry-into-queue + # readiness (review count, code-owner approval, etc). + # + # Raw payload syntax, not the newer `merge_queue` convenience key: that key + # is still pending in apache/infrastructure-asfyaml#119 as of this writing + # and isn't deployed yet, so using it here would fail INFRA-bot validation. + # Switch this entry to the convenience syntax once that PR merges. + # + # `required_status_checks` below intentionally omits two checks that are + # required in the classic config above: `lint-check` (validates the PR's + # own title -- meaningless once re-run against the queue's synthetic + # merge-group ref) and `dependency-review` (actions/dependency-review-action + # has a confirmed, unfixed bug against merge_group events as of this + # writing -- see actions/dependency-review-action#843 -- it destructures + # `github.event.pull_request`, which merge_group doesn't have, and crashes). + # Both still gate a PR before a committer can add it to the queue via the + # classic `required_status_checks` above; they just don't re-run once it's + # in the queue. + rulesets: + - name: "Merge Queue" + target: branch + enforcement: active + conditions: + ref_name: + include: + - "~DEFAULT_BRANCH" + exclude: [] + rules: + - type: required_status_checks + parameters: + strict_required_status_checks_policy: false + required_status_checks: + - context: enforce-single-migration-head + integration_id: -1 + - context: pre-commit (current) + integration_id: -1 + - context: frontend-build + integration_id: -1 + - context: cypress-matrix-required + integration_id: -1 + - context: playwright-tests-required + integration_id: -1 + - context: test-mysql + integration_id: -1 + - context: test-postgres-required + integration_id: -1 + - context: test-postgres-hive + integration_id: -1 + - context: test-postgres-presto + integration_id: -1 + - context: test-sqlite + integration_id: -1 + - context: unit-tests-required + integration_id: -1 + - type: merge_queue + parameters: + # How long a queued entry waits for its required checks to report + # a conclusion before GitHub gives up on it and removes it from + # the queue. This CI matrix runs full E2E/integration suites + # (Playwright, Cypress, MySQL/Postgres/Hive/Presto), which can + # legitimately take 40+ minutes; 60 leaves headroom without + # letting a genuinely hung run block the queue indefinitely. + check_response_timeout_minutes: 60 + # ALLGREEN (not HEADGREEN): every entry in a batch must pass + # before any of them merge, so a batch never ships a change on + # the strength of a check result computed against a different + # commit than the one landing. Slower under failure, but this + # matrix is expensive enough that re-running it per-entry on a + # HEADGREEN downgrade would be costly too. + grouping_strategy: ALLGREEN + # How many entries the queue will build/merge together at once. + # Kept modest given how large and slow this required-check matrix + # already is (Playwright, Cypress, and 4 backend DB flavors) -- + # a bigger batch multiplies the cost of any single entry's + # unrelated failure evicting the whole group. + max_entries_to_build: 5 + max_entries_to_merge: 5 + # Matches enabled_merge_buttons above: squash is the only merge + # method this repo has enabled. + merge_method: SQUASH + min_entries_to_merge: 1 + # How long the queue waits to see if more PRs arrive to batch + # together before proceeding with whatever's queued so far. Short + # enough that a lone PR isn't stuck waiting on nothing. + min_entries_to_merge_wait_minutes: 5 diff --git a/.github/actions/change-detector/action.yml b/.github/actions/change-detector/action.yml index 192a70ba3b00..81c0bb794933 100644 --- a/.github/actions/change-detector/action.yml +++ b/.github/actions/change-detector/action.yml @@ -40,3 +40,7 @@ runs: env: GITHUB_TOKEN: ${{ inputs.token }} GITHUB_OUTPUT: ${{ github.output }} + # Only set (non-empty) on a merge_group event; scripts/change_detector.py + # uses it to diff the merge-group's synthetic commit, which has no + # `pull_request` context to read a PR number from instead. + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} diff --git a/.github/workflows/enforce-single-migration-head.yml b/.github/workflows/enforce-single-migration-head.yml index b01284c910b6..6c5cc8c68856 100644 --- a/.github/workflows/enforce-single-migration-head.yml +++ b/.github/workflows/enforce-single-migration-head.yml @@ -22,6 +22,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] # No `paths:` filter on purpose: this job is a required status check, and a # required check that never runs for a given PR blocks that PR from merging @@ -53,7 +55,13 @@ jobs: with: github-token: ${{ github.token }} script: | - if (context.eventName === 'push') { + // A merge-group ref has no pull_request context to list files + // from (context.issue.number is undefined there), and this check + // exists specifically to catch two migration heads that each + // looked fine alone but collide once combined by the queue -- so + // always run the real check rather than skipping via the + // file-list optimization below. + if (context.eventName === 'push' || context.eventName === 'merge_group') { core.setOutput('changed', 'true'); return; } diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index b79ffcbb9a87..b4487277f599 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -7,6 +7,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] # Nightly full-tree sweep. Per-PR runs only lint changed files, so a change # that invalidates an untouched file (e.g. a type change that breaks an # importing test) can pass every PR yet leave master red. This catches that. @@ -92,6 +94,7 @@ jobs: env: EVENT_NAME: ${{ github.event_name }} BEFORE_SHA: ${{ github.event.before }} + MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} run: | set -euo pipefail @@ -113,6 +116,12 @@ jobs: # kernel's per-variable size limit and kill the step with # "Argument list too long" before bash even starts. base="$(git rev-parse HEAD^1 2>/dev/null || true)" + elif [ "${EVENT_NAME}" = "merge_group" ]; then + # The merge-group commit isn't guaranteed to be a two-parent merge + # commit the same shape as pull_request's, so don't assume HEAD^1 + # means the same thing here. merge_group.base_sha is the event's + # own explicit, documented base pointer -- use that directly. + base="${MERGE_GROUP_BASE_SHA:-}" elif [ -n "${BEFORE_SHA:-}" ] && \ [ "${BEFORE_SHA}" != "0000000000000000000000000000000000000000" ]; then base="${BEFORE_SHA}" diff --git a/.github/workflows/superset-e2e.yml b/.github/workflows/superset-e2e.yml index c22f68c1a2c8..32e1a20e3333 100644 --- a/.github/workflows/superset-e2e.yml +++ b/.github/workflows/superset-e2e.yml @@ -7,6 +7,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] workflow_dispatch: inputs: ref: diff --git a/.github/workflows/superset-frontend.yml b/.github/workflows/superset-frontend.yml index e8def54b0616..468ae8a89d6b 100644 --- a/.github/workflows/superset-frontend.yml +++ b/.github/workflows/superset-frontend.yml @@ -7,6 +7,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] # cancel previous workflow jobs for PRs concurrency: diff --git a/.github/workflows/superset-python-integrationtest.yml b/.github/workflows/superset-python-integrationtest.yml index 665e430d238e..2e4b3865eeab 100644 --- a/.github/workflows/superset-python-integrationtest.yml +++ b/.github/workflows/superset-python-integrationtest.yml @@ -13,6 +13,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} diff --git a/.github/workflows/superset-python-presto-hive.yml b/.github/workflows/superset-python-presto-hive.yml index 5797f37bb984..fa1ab401ef81 100644 --- a/.github/workflows/superset-python-presto-hive.yml +++ b/.github/workflows/superset-python-presto-hive.yml @@ -8,6 +8,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] # cancel previous workflow jobs for PRs concurrency: diff --git a/.github/workflows/superset-python-unittest.yml b/.github/workflows/superset-python-unittest.yml index 6fcd9ecde52e..7d76896faa81 100644 --- a/.github/workflows/superset-python-unittest.yml +++ b/.github/workflows/superset-python-unittest.yml @@ -13,6 +13,8 @@ on: - "[0-9].[0-9]*" pull_request: types: [synchronize, opened, reopened, ready_for_review] + merge_group: + types: [checks_requested] # cancel previous workflow jobs for PRs concurrency: diff --git a/scripts/change_detector.py b/scripts/change_detector.py index 41eb46a2141d..46d8bc2e7ca8 100755 --- a/scripts/change_detector.py +++ b/scripts/change_detector.py @@ -223,6 +223,13 @@ def fetch_changed_files_pr(repo: str, pr_number: str) -> List[str]: return [file_info["filename"] for file_info in files] +def fetch_changed_files_compare(repo: str, base_sha: str, head_sha: str) -> List[str]: + """Fetches files changed between two commits using the GitHub compare API.""" + compare_url = f"https://api.github.com/repos/{repo}/compare/{base_sha}...{head_sha}" + comparison_data = fetch_files_github_api(compare_url) + return [file["filename"] for file in comparison_data["files"]] + + def fetch_changed_files_push(repo: str, sha: str) -> List[str]: """Fetches files changed in the last commit for push events using GitHub API.""" # Fetch commit details to get the parent SHA @@ -232,9 +239,7 @@ def fetch_changed_files_push(repo: str, sha: str) -> List[str]: raise RuntimeError("No parent commit found for comparison.") parent_sha = commit_data["parents"][0]["sha"] # Compare the current commit against its parent - compare_url = f"https://api.github.com/repos/{repo}/compare/{parent_sha}...{sha}" - comparison_data = fetch_files_github_api(compare_url) - return [file["filename"] for file in comparison_data["files"]] + return fetch_changed_files_compare(repo, parent_sha, sha) def detect_changes(files: List[str], check_patterns: List) -> bool: # type: ignore @@ -271,6 +276,25 @@ def main(event_type: str, sha: str, repo: str) -> None: print("Files touched since previous commit:") print_files(files) + elif event_type == "merge_group": + # The merge queue's synthetic commit has no `pull_request` context to + # read a PR number from, and its parent-commit shape isn't guaranteed + # to match `push`'s. `merge_group.base_sha` is the event's own + # explicit, documented base pointer -- the workflow wires it through + # as MERGE_GROUP_BASE_SHA. `sha` here is already GITHUB_SHA, which for + # a merge_group event is documented as "SHA of the merge group" (the + # head we want). + base_sha = os.environ.get("MERGE_GROUP_BASE_SHA", "") + if not base_sha: + raise RuntimeError( + "merge_group event missing MERGE_GROUP_BASE_SHA; the workflow " + "must pass github.event.merge_group.base_sha through as that " + "env var." + ) + files = fetch_changed_files_compare(repo, base_sha, sha) + print("Files touched in this merge group:") + print_files(files) + elif event_type in ("workflow_dispatch", "schedule"): # Manual or cron-triggered runs aren't tied to a specific diff, so # treat every group as changed. `files = None` makes the loop below diff --git a/tests/unit_tests/scripts/change_detector_test.py b/tests/unit_tests/scripts/change_detector_test.py index f543a338d057..3bd180b6cdd6 100644 --- a/tests/unit_tests/scripts/change_detector_test.py +++ b/tests/unit_tests/scripts/change_detector_test.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. import io +import os from unittest import mock from urllib.error import HTTPError, URLError @@ -303,3 +304,74 @@ def test_composite_action_changes_trigger_tests(group: str) -> None: def test_sdk_changes_trigger_python_unit_workflow(path: str) -> None: """SDK-only changes must run the workflow that contains the SDK test step.""" assert change_detector.detect_changes([path], change_detector.PATTERNS["python"]) + + +def test_fetch_changed_files_compare_returns_filenames() -> None: + with mock.patch.object( + change_detector, + "fetch_files_github_api", + return_value={"files": [{"filename": "superset/foo.py"}]}, + ) as api_mock: + result = change_detector.fetch_changed_files_compare( + "apache/superset", "base-sha", "head-sha" + ) + + assert result == ["superset/foo.py"] + api_mock.assert_called_once_with( + "https://api.github.com/repos/apache/superset/compare/base-sha...head-sha" + ) + + +def test_fetch_changed_files_push_delegates_to_compare() -> None: + """`push` still resolves its own base (the parent commit) before comparing, + but the actual diff call is the same shared compare helper `merge_group` + uses.""" + with ( + mock.patch.object( + change_detector, + "fetch_files_github_api", + return_value={"parents": [{"sha": "parent-sha"}]}, + ) as commit_mock, + mock.patch.object( + change_detector, "fetch_changed_files_compare", return_value=["a.py"] + ) as compare_mock, + ): + result = change_detector.fetch_changed_files_push("apache/superset", "head-sha") + + assert result == ["a.py"] + commit_mock.assert_called_once_with( + "https://api.github.com/repos/apache/superset/commits/head-sha" + ) + compare_mock.assert_called_once_with("apache/superset", "parent-sha", "head-sha") + + +def test_main_merge_group_diffs_against_base_sha(tmp_path) -> None: + """A queue entry has no PR number to read; it must diff against the + event's own `merge_group.base_sha` instead.""" + output_file = tmp_path / "github_output" + with ( + mock.patch.dict( + os.environ, + { + "MERGE_GROUP_BASE_SHA": "base-sha", + "GITHUB_OUTPUT": str(output_file), + }, + ), + mock.patch.object( + change_detector, + "fetch_changed_files_compare", + return_value=["superset/foo.py"], + ) as compare_mock, + ): + change_detector.main("merge_group", "head-sha", "apache/superset") + + compare_mock.assert_called_once_with("apache/superset", "base-sha", "head-sha") + assert "python=true" in output_file.read_text() + + +def test_main_merge_group_without_base_sha_fails_closed() -> None: + """A missing MERGE_GROUP_BASE_SHA means the workflow wiring is broken -- + fail loudly rather than silently skip the diff.""" + with mock.patch.dict(os.environ, {"MERGE_GROUP_BASE_SHA": ""}): + with pytest.raises(RuntimeError, match="MERGE_GROUP_BASE_SHA"): + change_detector.main("merge_group", "head-sha", "apache/superset") From a5ae2ef541e1c06114431815eaf2919fd9a26c3a Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 23 Sep 2026 01:58:47 -0700 Subject: [PATCH 2/2] docs(ci): note the ruleset/classic required-checks lists must stay in sync The merge queue ruleset's required_status_checks list is a deliberate, documented subset of the classic protected_branches list -- but the overlap itself wasn't called out, so a check added to one and not the other would silently let the queue merge without it. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5 --- .asf.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.asf.yaml b/.asf.yaml index c44eb875e0af..2e60b8d04a08 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -125,6 +125,11 @@ github: # Both still gate a PR before a committer can add it to the queue via the # classic `required_status_checks` above; they just don't re-run once it's # in the queue. + # + # SYNC REQUIRED: aside from the two deliberate omissions above, this list + # must mirror `protected_branches.master.required_status_checks.contexts`. + # A check added there but not here would let the merge queue land a change + # without it. rulesets: - name: "Merge Queue" target: branch