🐛 fix: checkout PR head SHA for issue_comment triggered workflows#832
🐛 fix: checkout PR head SHA for issue_comment triggered workflows#832
Conversation
When /trigger-e2e-full is used on a PR, the workflow is triggered via issue_comment event. For this event type, github.sha points to the default branch (main), not the PR head. This caused e2e-tests and lint-and-test jobs to build/test main instead of the PR changes. Fix: - Export pr_head_sha from check-code-changes job (already computed) - e2e-tests: use pr_head_sha output in checkout (already depends on check-code-changes) - lint-and-test: add PR info step to resolve head SHA for issue_comment events - When ref is empty string, actions/checkout falls back to default behavior (correct for pull_request events) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
/trigger-e2e-full |
|
🚀 Full E2E tests triggered by /trigger-e2e-full |
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
There was a problem hiding this comment.
Pull request overview
Fixes CI correctness for issue_comment-triggered runs by ensuring jobs checkout the PR head commit (instead of main, which github.sha points to for issue_comment events).
Changes:
- Export
pr_head_shafromcheck-code-changesas a job output. - Update
e2e-teststo checkoutneeds.check-code-changes.outputs.pr_head_sha. - Update
lint-and-testto resolve the PR head SHA onissue_commentevents and checkout that ref.
| - name: Checkout source | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ needs.check-code-changes.outputs.pr_head_sha || '' }} |
There was a problem hiding this comment.
e2e-tests now checks out needs.check-code-changes.outputs.pr_head_sha (PR code) even on issue_comment runs, but the job-level if: condition still allows the job to run when has_code_changes == 'true' (which can be true for ordinary comments). This enables untrusted commenters to trigger execution of PR code (and expensive Kind E2E runs). Tighten the e2e-tests if: logic so that issue_comment runs only proceed when check-full-tests has validated an approved trigger comment from a trusted collaborator.
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
- Skip lint-and-test for issue_comment events (already runs on pull_request) - Remove duplicated PR head SHA resolution from lint-and-test - Tighten e2e-tests gate: issue_comment only runs when check-full-tests validates an approved trigger from a trusted collaborator - Explicitly handle workflow_dispatch and pull_request event types Signed-off-by: Andrew Anderson <andy@clubanderson.com>
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
| if: >- | ||
| always() && ( | ||
| (github.event_name == 'issue_comment' && needs.check-full-tests.outputs.run_full == 'true') || | ||
| (github.event_name == 'workflow_dispatch' && needs.check-full-tests.outputs.run_full == 'true') || |
There was a problem hiding this comment.
The updated if: condition removes the previous fallback that allowed smoke tests to run on workflow_dispatch when run_full_tests is false. With this change, a manual dispatch with default inputs will run no e2e tests, which contradicts the workflow_dispatch input description (“default: smoke tests only”) and the comment above e2e-tests (“smoke tests run automatically”). Consider restoring the smoke-test path for workflow_dispatch (e.g., reuse the has_code_changes/default-true behavior) or update the input/inline documentation to match the new behavior.
| (github.event_name == 'workflow_dispatch' && needs.check-full-tests.outputs.run_full == 'true') || | |
| (github.event_name == 'workflow_dispatch' && ( | |
| needs.check-full-tests.outputs.run_full == 'true' || | |
| (needs.check-code-changes.result == 'success' && needs.check-code-changes.outputs.has_code_changes == 'true') | |
| )) || |
| if: >- | ||
| always() && ( | ||
| (github.event_name == 'issue_comment' && needs.check-full-tests.outputs.run_full == 'true') || | ||
| (github.event_name == 'workflow_dispatch' && needs.check-full-tests.outputs.run_full == 'true') || | ||
| (github.event_name == 'pull_request' && needs.check-code-changes.result == 'success' && needs.check-code-changes.outputs.has_code_changes == 'true') | ||
| ) | ||
| timeout-minutes: 60 | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - name: Checkout source | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ needs.check-code-changes.outputs.pr_head_sha || '' }} |
There was a problem hiding this comment.
For issue_comment triggers, e2e-tests can still run even if check-code-changes fails (because of always() and no needs.check-code-changes.result == 'success' guard). In that failure mode needs.check-code-changes.outputs.pr_head_sha will be empty, so the checkout falls back to the event default SHA (which is main for issue_comment) and reintroduces the original problem. Tighten the if: for the issue_comment branch to require check-code-changes success (and/or a non-empty pr_head_sha), or explicitly fail early when pr_head_sha is empty.
| # lint-and-test already runs on pull_request events; skip for issue_comment | ||
| # to avoid untrusted commenters triggering execution of PR code | ||
| lint-and-test: | ||
| if: github.event_name != 'issue_comment' | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
This change makes lint-and-test skip entirely for issue_comment events, but the PR description says the fix is to resolve PR info and check out the PR head SHA in lint-and-test. Please reconcile the implementation with the PR description (either implement the described checkout/ref behavior for trusted comment triggers, or update the PR description to reflect that lint is intentionally not run for issue_comment).
- Restore workflow_dispatch smoke-test path (run_full OR has_code_changes) - Require check-code-changes success for issue_comment e2e runs - Add explicit validation step to fail fast if pr_head_sha is empty Signed-off-by: Andrew Anderson <andy@clubanderson.com>
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
|
@clubanderson can we also make the full tests required before pr merge? Thanks. |
|
Is there an example of another test or action you would like to follow in wva? |
|
For example the CI openshift tests and the smoke tests are required right? Same |
|
@mamy-CS Making full e2e tests a required check is separate scope from this PR (which fixes the checkout ref bug for issue_comment triggers). Could you approve this one and open a new issue to track adding a required full e2e check? Happy to help with that next. Thanks! |
Problem
Fixes #831
When
/trigger-e2e-fullis used on a PR, the workflow triggers viaissue_commentevent. For this event type,github.shapoints tomain(the default branch), not the PR head. This caused thee2e-testsjob to build and test code frommaininstead of the PR changes.Root Cause
The
check-code-changesjob already correctly resolves the PR head SHA and checks it out, bute2e-testsdid a bareactions/checkout@v4with noref:, defaulting togithub.sha(main for issue_comment events).Fix
pr_head_shaas a job output (already computed but not exposed)needs.check-code-changes.outputs.pr_head_shain checkout ref, with a validation step that fails fast if the SHA is empty (prevents silent fallback to main)issue_commentevents — it already runs onpull_requesttriggers, so re-running on comments is unnecessary and would allow untrusted commenters to execute PR codeissue_commentevents only run whencheck-full-testsvalidates an approved trigger from a trusted collaborator ANDcheck-code-changessucceedsrun_full_tests=trueOR when code changes are detected (matching existing behavior)