You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/03 16:04:21 UTC
[PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
rusackas opened a new pull request, #40719:
URL: https://github.com/apache/superset/pull/40719
> **DRAFT — for evaluation of the mechanism.** This builds on #40718
> (the `changes` job). Please review the tradeoffs below before considering it.
### SUMMARY
Today the E2E (Cypress + Playwright) workflow runs **in parallel** with
`pre-commit checks`. When a PR has a formatting/lint/type error that pre-commit
catches in ~4 minutes, the Cypress shards and Playwright jobs (~15–20 min each,
full Python+Node+asset setup) have already spun up and burned runner minutes for
nothing.
This makes E2E run **only after `pre-commit checks` completes successfully**.
**Mechanism:**
- Trigger switches from `push`/`pull_request` to `workflow_run` on
`["pre-commit checks"]`. pre-commit itself runs on push (master/release) +
pull_request, so event coverage is preserved.
- The entry `changes` job gates on
`github.event.workflow_run.conclusion == 'success'`. On a pre-commit failure
it — and every downstream `needs: changes` job — is **skipped**, so no
Cypress/Playwright runners are provisioned.
- `scripts/change_detector.py` gains a `workflow_run` path: it recovers the
originating event / head SHA / PR number from `WF_RUN_*` env vars (fork PRs
don't carry PR context in the payload, so they conservatively
assume-all-changed and run the full suite).
- Checkouts and the push-only `/app/prefix` matrix switch now read
`github.event.workflow_run.*` instead of the live event context.
- A new `report-status` job posts an aggregate **`E2E / required`** commit
status back onto the PR head SHA, because `workflow_run` checks don't attach
to the PR automatically.
### TRADEOFFS TO EVALUATE (why this is a draft)
1. **Default-branch workflow file.** GitHub always uses the **master** copy of a
`workflow_run`-triggered workflow. Edits to `superset-e2e.yml` on a PR branch
won't take effect until merged — this very PR's changes can't be exercised on
the PR itself.
2. **Branch protection must change.** The individual `cypress-matrix` /
`playwright-tests` checks no longer appear on PRs; **`E2E / required`** (the
posted commit status) must become the required check instead.
3. **Fork PRs.** `workflow_run.pull_requests` is empty for forks, so they fall
back to running the full suite (no change-detection narrowing).
### TESTING INSTRUCTIONS
Because of tradeoff #1 this can only be fully validated once on master (behind
the branch-protection change). The `change_detector.py` `workflow_run` routing
is unit-verifiable locally:
```
GITHUB_EVENT_NAME=workflow_run WF_RUN_EVENT=pull_request WF_RUN_PR_NUMBER="" \
python scripts/change_detector.py --event-type workflow_run --sha x --repo apache/superset
# -> "assuming all changed" (fork/no-PR fallback)
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3351724874
##########
scripts/change_detector.py:
##########
@@ -109,6 +109,37 @@ def is_int(s: str) -> bool:
return bool(re.match(r"^-?\d+$", s))
+def resolve_workflow_run_files(repo: str, sha: str) -> Optional[List[str]]:
+ """Resolve changed files for a workflow_run-triggered run.
+
+ When a workflow is gated behind another (e.g. running only after
+ pre-commit succeeds), GitHub re-dispatches it as a `workflow_run` event
+ whose context points at the default branch rather than the originating
+ diff. Recover the original event and head SHA from the workflow_run
+ payload, exposed via the WF_RUN_* env vars. Returns ``None`` (meaning
+ "assume all changed") when the diff can't be resolved.
+ """
+ original_event = os.getenv("WF_RUN_EVENT") or "push"
+ print("ORIGINAL_EVENT", original_event)
+ if original_event == "pull_request":
+ pr_number = os.getenv("WF_RUN_PR_NUMBER", "")
+ if not is_int(pr_number):
+ # Fork PRs don't populate workflow_run.pull_requests, so we can't
+ # resolve the diff -> assume all changed (run everything).
+ print("workflow_run without PR context, assuming all changed")
+ return None
+ files = fetch_changed_files_pr(repo, pr_number)
+ print("PR files:")
+ print_files(files)
+ return files
+
+ head_sha = os.getenv("WF_RUN_HEAD_SHA") or sha
+ files = fetch_changed_files_push(repo, head_sha)
+ print("Files touched since previous commit:")
+ print_files(files)
+ return files
Review Comment:
Good call — added unit tests in `tests/unit_tests/scripts/change_detector_test.py` covering `resolve_workflow_run_files`: the pull_request and push success paths (asserting the right fetch function is called and its return value propagates), the head-SHA-vs-fallback-SHA behavior, and the missing/invalid `WF_RUN_PR_NUMBER` cases that return `None` (assume all changed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4615860108
On the "gate behind unit + frontend too" idea — totally agree on the principle (E2E is the priciest, so ideally it only runs once the cheap gates are green). The catch is GitHub's `workflow_run` fires on *each* listed workflow completing and has no native "run only if **all** of [pre-commit, unit, frontend] succeeded" — you'd have to list them all and then query the others' latest status via the API on every trigger, which is fiddly and race-prone. The clean way to express "A and B and C must pass before E2E" is to put them in one workflow with `needs:` — but they're separate workflows today, so that's a bigger consolidation.
I deliberately started with **pre-commit** as the single gate because it's the fastest (~4 min) and catches the most common failures (lint/format/type), so it captures most of the "don't waste E2E on a broken PR" value at minimal complexity. Gating additionally on unit + frontend has real diminishing returns for a lot of added cross-workflow glue.
Your auto-cancel alternative is viable but similar in spirit — it needs a job that polls the other workflows' status and self-cancels via the API. Happy to prototype either the multi-gate or a consolidated-CI approach as a follow-up if folks want to invest in the coordination — but I'd suggest landing the pre-commit gate first to prove the mechanism, then layering more gates on top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3351234846
##########
scripts/change_detector.py:
##########
@@ -109,6 +109,37 @@ def is_int(s: str) -> bool:
return bool(re.match(r"^-?\d+$", s))
+def resolve_workflow_run_files(repo: str, sha: str) -> Optional[List[str]]:
+ """Resolve changed files for a workflow_run-triggered run.
+
+ When a workflow is gated behind another (e.g. running only after
+ pre-commit succeeds), GitHub re-dispatches it as a `workflow_run` event
+ whose context points at the default branch rather than the originating
+ diff. Recover the original event and head SHA from the workflow_run
+ payload, exposed via the WF_RUN_* env vars. Returns ``None`` (meaning
+ "assume all changed") when the diff can't be resolved.
+ """
+ original_event = os.getenv("WF_RUN_EVENT") or "push"
+ print("ORIGINAL_EVENT", original_event)
+ if original_event == "pull_request":
+ pr_number = os.getenv("WF_RUN_PR_NUMBER", "")
+ if not is_int(pr_number):
+ # Fork PRs don't populate workflow_run.pull_requests, so we can't
+ # resolve the diff -> assume all changed (run everything).
+ print("workflow_run without PR context, assuming all changed")
+ return None
+ files = fetch_changed_files_pr(repo, pr_number)
+ print("PR files:")
+ print_files(files)
+ return files
+
+ head_sha = os.getenv("WF_RUN_HEAD_SHA") or sha
+ files = fetch_changed_files_push(repo, head_sha)
+ print("Files touched since previous commit:")
+ print_files(files)
+ return files
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new function</b></div>
<div id="fix">
New `resolve_workflow_run_files` function lacks unit tests per project guidelines. Add tests in `tests/scripts/test_change_detector.py` covering success paths for PR and push events, plus error scenarios (missing/invalid WF_RUN_PR_NUMBER). Tests should verify the correct file-fetching functions are called and return values propagate correctly.
</div>
</div>
<small><i>Code Review Run #ae5f27</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3351728231
##########
.github/workflows/superset-e2e.yml:
##########
@@ -269,3 +287,34 @@ jobs:
${{ github.workspace }}/superset-frontend/playwright-results/
${{ github.workspace }}/superset-frontend/test-results/
name: playwright-artifact-${{ github.run_id }}-${{ github.job }}-${{ matrix.browser }}--${{ steps.set-safe-app-root.outputs.safe_app_root }}
+
+ # workflow_run runs don't attach their checks to the originating PR, so post
+ # an aggregate commit status back onto the PR head SHA. Make THIS the required
+ # status check in branch protection (in place of the individual E2E jobs).
+ report-status:
+ needs: [cypress-matrix, playwright-tests]
+ if: always() && github.event_name == 'workflow_run'
+ runs-on: ubuntu-24.04
+ permissions:
+ statuses: write
+ steps:
+ - name: Report aggregate E2E status to PR commit
+ uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
+ with:
+ script: |
+ // 'skipped' is acceptable: the change-detector legitimately skips
+ // jobs when no relevant files changed. Only real failures fail.
+ const results = [
+ '${{ needs.cypress-matrix.result }}',
+ '${{ needs.playwright-tests.result }}',
+ ];
+ const ok = results.every((r) => r === 'success' || r === 'skipped');
+ await github.rest.repos.createCommitStatus({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ sha: context.payload.workflow_run.head_sha,
+ state: ok ? 'success' : 'failure',
+ context: 'E2E / required',
+ description: ok ? 'E2E passed (or skipped)' : 'E2E failed',
+ target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`,
+ });
Review Comment:
Right — that is the intended behavior, and it is safe because the merge gate is the `pre-commit checks` workflow itself, not the E2E/Cypress/Playwright workflows. Those expensive workflows are deliberately not in the required-status-check set; they only fan out via `workflow_run` after pre-commit succeeds. So if pre-commit fails, the required pre-commit check is red and blocks the merge directly — the gated E2E jobs being skipped (and therefore never reporting) does not create a false green, because they were never the thing branch protection waits on. The only way to land is a green pre-commit run, which is exactly what then unlocks the E2E fan-out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4615856850
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #40719: ci: gate Cypress/Playwright behind pre-commit via workflow_run
URL: https://github.com/apache/superset/pull/40719
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4615362332
@rusackas I might make this even more extreme but it may add to the CI time, but only run post unit tests and frontend CI passes. Since e2e is the most expensive, we should only run if all other gates have passed. Imagine frontend build CI fails, someone will have to fix and would require another run. Or if we don't like that, could we auto-cancel if any steps fail before e2e is finished running?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3352147966
##########
.github/workflows/superset-e2e.yml:
##########
@@ -88,13 +106,13 @@ jobs:
steps:
# -------------------------------------------------------
# Conditional checkout based on context
- - name: Checkout for push or pull_request event
- if: github.event_name == 'push' || github.event_name == 'pull_request'
+ - name: Checkout (gated by pre-commit via workflow_run)
+ if: github.event_name == 'workflow_run'
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
with:
persist-credentials: false
submodules: recursive
- ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}
+ ref: ${{ github.event.workflow_run.head_sha }}
Review Comment:
By changing this from `pull_request` to `workflow_run` do we drop the implicit fork-PR secrets block?
This checkout pulls the fork's SHA, then `cypress-run-all` runs with `secrets.CYPRESS_RECORD_KEY` in env (L163) — a fork PR can exfiltrate the key once pre-commit passes. Same shape on `playwright-tests` (L222). Both jobs need `if: github.event.workflow_run.head_repository.full_name == github.repository` to skip forks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4614536490
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40719?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 63.28%. Comparing base ([`cb2a56d`](https://app.codecov.io/gh/apache/superset/commit/cb2a56d16e0bccf0e6d8cb878b90ca3474b1a7b4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`de92cc0`](https://app.codecov.io/gh/apache/superset/commit/de92cc067af45ebaedb6497193b1775587a65ae1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40719 +/- ##
==========================================
- Coverage 64.07% 63.28% -0.80%
==========================================
Files 2663 2663
Lines 143505 143505
Branches 32993 32993
==========================================
- Hits 91951 90814 -1137
- Misses 49952 51083 +1131
- Partials 1602 1608 +6
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.72% <ø> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.31% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.32% <ø> (-1.70%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.16% <ø> (ø)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40719/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40719?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3350093860
##########
.github/workflows/superset-playwright.yml:
##########
@@ -23,9 +23,30 @@
cancel-in-progress: true
jobs:
+ changes:
+ runs-on: ubuntu-24.04
+ permissions:
+ contents: read
+ pull-requests: read
+ outputs:
+ python: ${{ steps.check.outputs.python }}
+ frontend: ${{ steps.check.outputs.frontend }}
+ steps:
+ - name: Checkout
+ uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
Review Comment:
## zizmor /
action's hash pin has mismatched or missing version comment: points to commit df4cb1c069e1
[Show more details](https://github.com/apache/superset/security/code-scanning/2517)
##########
.github/workflows/superset-python-presto-hive.yml:
##########
@@ -15,7 +15,27 @@
cancel-in-progress: true
jobs:
+ changes:
+ runs-on: ubuntu-24.04
+ permissions:
+ contents: read
+ pull-requests: read
+ outputs:
+ python: ${{ steps.check.outputs.python }}
+ steps:
+ - name: Checkout
+ uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
Review Comment:
## zizmor /
action's hash pin has mismatched or missing version comment: points to commit df4cb1c069e1
[Show more details](https://github.com/apache/superset/security/code-scanning/2519)
##########
.github/workflows/superset-python-integrationtest.yml:
##########
@@ -14,7 +14,27 @@
cancel-in-progress: true
jobs:
+ changes:
+ runs-on: ubuntu-24.04
+ permissions:
+ contents: read
+ pull-requests: read
+ outputs:
+ python: ${{ steps.check.outputs.python }}
+ steps:
+ - name: Checkout
+ uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
Review Comment:
## zizmor /
action's hash pin has mismatched or missing version comment: points to commit df4cb1c069e1
[Show more details](https://github.com/apache/superset/security/code-scanning/2518)
##########
.github/workflows/superset-python-unittest.yml:
##########
@@ -15,7 +15,27 @@
cancel-in-progress: true
jobs:
+ changes:
+ runs-on: ubuntu-24.04
+ permissions:
+ contents: read
+ pull-requests: read
+ outputs:
+ python: ${{ steps.check.outputs.python }}
+ steps:
+ - name: Checkout
+ uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
Review Comment:
## zizmor /
action's hash pin has mismatched or missing version comment: points to commit df4cb1c069e1
[Show more details](https://github.com/apache/superset/security/code-scanning/2520)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3351657922
##########
.github/workflows/superset-e2e.yml:
##########
@@ -269,3 +287,34 @@ jobs:
${{ github.workspace }}/superset-frontend/playwright-results/
${{ github.workspace }}/superset-frontend/test-results/
name: playwright-artifact-${{ github.run_id }}-${{ github.job }}-${{ matrix.browser }}--${{ steps.set-safe-app-root.outputs.safe_app_root }}
+
+ # workflow_run runs don't attach their checks to the originating PR, so post
+ # an aggregate commit status back onto the PR head SHA. Make THIS the required
+ # status check in branch protection (in place of the individual E2E jobs).
+ report-status:
+ needs: [cypress-matrix, playwright-tests]
+ if: always() && github.event_name == 'workflow_run'
+ runs-on: ubuntu-24.04
+ permissions:
+ statuses: write
+ steps:
+ - name: Report aggregate E2E status to PR commit
+ uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
+ with:
+ script: |
+ // 'skipped' is acceptable: the change-detector legitimately skips
+ // jobs when no relevant files changed. Only real failures fail.
+ const results = [
+ '${{ needs.cypress-matrix.result }}',
+ '${{ needs.playwright-tests.result }}',
+ ];
+ const ok = results.every((r) => r === 'success' || r === 'skipped');
+ await github.rest.repos.createCommitStatus({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ sha: context.payload.workflow_run.head_sha,
+ state: ok ? 'success' : 'failure',
+ context: 'E2E / required',
+ description: ok ? 'E2E passed (or skipped)' : 'E2E failed',
+ target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`,
+ });
Review Comment:
Right now when tests are skipped because pre-commit fails we'll land a success. Is that what we want? Shouldn't be an issue since pre-commit is required but just wondering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40719:
URL: https://github.com/apache/superset/pull/40719#discussion_r3351726254
##########
scripts/change_detector.py:
##########
@@ -109,6 +109,37 @@ def is_int(s: str) -> bool:
return bool(re.match(r"^-?\d+$", s))
+def resolve_workflow_run_files(repo: str, sha: str) -> Optional[List[str]]:
+ """Resolve changed files for a workflow_run-triggered run.
+
+ When a workflow is gated behind another (e.g. running only after
+ pre-commit succeeds), GitHub re-dispatches it as a `workflow_run` event
+ whose context points at the default branch rather than the originating
+ diff. Recover the original event and head SHA from the workflow_run
+ payload, exposed via the WF_RUN_* env vars. Returns ``None`` (meaning
+ "assume all changed") when the diff can't be resolved.
+ """
+ original_event = os.getenv("WF_RUN_EVENT") or "push"
+ print("ORIGINAL_EVENT", original_event)
+ if original_event == "pull_request":
+ pr_number = os.getenv("WF_RUN_PR_NUMBER", "")
+ if not is_int(pr_number):
+ # Fork PRs don't populate workflow_run.pull_requests, so we can't
+ # resolve the diff -> assume all changed (run everything).
+ print("workflow_run without PR context, assuming all changed")
+ return None
+ files = fetch_changed_files_pr(repo, pr_number)
+ print("PR files:")
+ print_files(files)
+ return files
+
+ head_sha = os.getenv("WF_RUN_HEAD_SHA") or sha
+ files = fetch_changed_files_push(repo, head_sha)
+ print("Files touched since previous commit:")
+ print_files(files)
+ return files
Review Comment:
<!-- Bito Reply -->
The addition of unit tests in `tests/unit_tests/scripts/change_detector_test.py` effectively addresses the reviewer's suggestion to cover the success paths for PR and push events, as well as the specified error scenarios. This implementation ensures that the `resolve_workflow_run_files` function is properly validated according to project guidelines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4617263544
Great catch, and you're right — this is a real fork-PR exfiltration vector, not a hypothetical. Switching `pull_request` → `workflow_run` drops the implicit fork secrets block: the gated run executes in the base-repo context with full secrets **and** a read-write `GITHUB_TOKEN`, and since we check out and run the fork's code (`cypress-run-all` at L166), a fork PR could lift `CYPRESS_RECORD_KEY` *and* the token once pre-commit passes. Broader than just the record key.
Your `head_repository.full_name == github.repository` guard is the correct fix, but it skips forks entirely — which means fork PRs (most of our external contributions) would lose E2E coverage they have today. That tradeoff, plus the other `workflow_run` downsides (default-branch-only workflow file, branch-protection rework, forks losing change-detection), tips the cost/benefit against this approach.
Since #40717 (shard cut) and #40718 (job-level change-gating) already capture the bulk of the E2E savings with no security change, I'm going to **close this one** rather than bolt on guards that negate its value. Thanks for the careful review — this was the right call to flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4614368868
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] ci: gate Cypress/Playwright behind pre-commit via workflow_run [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40719:
URL: https://github.com/apache/superset/pull/40719#issuecomment-4617186671
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org