-
Notifications
You must be signed in to change notification settings - Fork 0
feat(#2096): add two-pass review strategy for large PRs #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
92e2d17
436a7f8
0a0561b
f19f1e3
32aaf9d
b5baa69
8a9681e
0b50f96
1f678e7
1881e3b
88ecef4
0013c75
893d1af
52dc9d2
b7b04f5
7d71e38
d330766
99ddc9d
fed552c
1d3da39
73dea45
63c27e4
ba99ae3
9a35c91
d4a394e
e492ac7
b2055cb
c48a832
3a44b0c
6f79d87
080368c
11bae49
e57f10a
602f75b
d1baca8
47e61b6
368890e
2e040b5
7c40a70
162dce2
80a414d
d2d2428
22c6e28
f126581
bbbb0b5
5fe6487
22be06d
7ecf899
61f467d
5e3d932
ecf5175
3305c1a
4c360c8
ac64c91
ded059b
78302ba
3c9f0db
7249b34
df020f5
3ae6f72
966abbf
65b155c
d988d32
e66f2d9
a24ffd1
d6988a9
515e49b
387968a
133ed6e
6832b14
32f73a4
b4d1c97
a9bd135
2b93fff
3fb219c
9241475
22d710d
25a286f
7905dfc
6f7ddf6
f322448
560ace4
7aef782
f902ef8
f4e19d5
f71504f
b04ecb3
ed8c416
b405b36
7993274
854d2e0
5c5e14d
c7ad026
6ac8e8f
d8c20b3
543d3ce
37ffc36
a4d5818
58c0e94
8dc0b93
33084f7
70ed5c1
2aaead0
2181382
3d54bc9
71601af
24fd33f
9806973
18846a2
25d4659
12b47a9
f01e246
e972b2c
fe94a21
6f20434
adba556
1dabdc6
14d5335
8b62249
fcd4101
ad57f0b
a39963f
a84bddf
773df28
874e0bb
2c94eab
241c5da
b73e233
72f1848
095039e
9ea24e8
57e807c
de9e17a
cf544d0
c8ea622
24f969e
bb406a3
1b69b0f
b4f6454
81848a5
5ce3e65
dce83dd
f77a94b
6cf0bb0
c5c248d
62926fc
6ce108d
ac47bf5
6371614
270ab1d
758c27d
3ed6080
c78c7d1
c30a531
e7f68c3
fee13a5
7077be2
d2856eb
b906210
71dc194
8e9ba31
5ee4955
36186df
a4a5008
8271187
1e985c9
47c8fdc
4d83c42
f112cad
6dbd59a
650261c
84a141c
9a7370e
f0532f9
ee8a90c
647da74
555511e
9888e31
e3bd02c
398ea67
689db50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #!/usr/bin/env bash | ||
| # Install the pinned OpenShell version via upstream install.sh. | ||
| # | ||
| # Sources openshell-version.sh for the version and commit SHA, then | ||
| # runs the upstream installer. Requires sudo for RPM installation. | ||
| # | ||
| # Usage: | ||
| # .github/scripts/install-openshell.sh | ||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| source "${SCRIPT_DIR}/openshell-version.sh" | ||
|
|
||
| echo "Installing OpenShell ${OPENSHELL_VERSION} (${OPENSHELL_SHA})" | ||
| curl -LsSf "https://raw.githubusercontent.com/NVIDIA/OpenShell/${OPENSHELL_SHA}/install.sh" \ | ||
| | OPENSHELL_VERSION="v${OPENSHELL_VERSION}" sh | ||
|
|
||
| openshell --version | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| #!/usr/bin/env bash | ||
| # Single source of truth for the pinned OpenShell version. | ||
| # | ||
| # Source this script to set OPENSHELL_VERSION and OPENSHELL_SHA in the | ||
| # current shell. In GitHub Actions it also exports them to GITHUB_ENV | ||
| # for downstream steps. | ||
| # | ||
| # Usage: | ||
| # source .github/scripts/openshell-version.sh | ||
|
|
||
| # renovate: datasource=github-tags depName=NVIDIA/OpenShell | ||
| OPENSHELL_VERSION=0.0.63 | ||
| OPENSHELL_SHA=ec197a43ef349e36c3fff04e9aaea9599fb83b31 | ||
|
|
||
| export OPENSHELL_VERSION OPENSHELL_SHA | ||
|
|
||
| if [[ -n "${GITHUB_ENV:-}" ]]; then | ||
| echo "OPENSHELL_VERSION=${OPENSHELL_VERSION}" >> "${GITHUB_ENV}" | ||
| echo "OPENSHELL_SHA=${OPENSHELL_SHA}" >> "${GITHUB_ENV}" | ||
| fi |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ permissions: {} | |
| on: | ||
| push: | ||
| branches: [main] | ||
| # SYNC-WITH: grep regex in "Check for e2e-relevant changes" step in the e2e job | ||
| paths: | ||
| - '**/*.go' | ||
| - 'go.mod' | ||
|
|
@@ -24,19 +25,6 @@ on: | |
| - 'scripts/check-e2e-authorization.sh' | ||
| pull_request_target: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] workflow-trigger-broadening Removed paths: filter from pull_request_target trigger. Every PR now triggers the e2e workflow. While the gate job enforces authorization, removing the path filter widens the trigger surface for pull_request_target which runs with base-repo privileges and secret access. Suggested fix: Keep paths: filter as defense-in-depth, or document the intentional trade-off. |
||
| types: [opened, synchronize, reopened, labeled] | ||
| paths: | ||
| - '**/*.go' | ||
| - 'go.mod' | ||
| - 'go.sum' | ||
| - 'e2e/**' | ||
| - 'internal/scaffold/fullsend-repo/**' | ||
| - 'internal/security/hooks/**' | ||
| - 'internal/dispatch/gcf/mintsrc/**' | ||
| - 'internal/sentencetoken/english.json' | ||
| - 'Makefile' | ||
| - '.github/workflows/e2e.yml' | ||
| - '.github/actions/check-e2e-authorization/**' | ||
| - 'scripts/check-e2e-authorization.sh' | ||
| merge_group: | ||
| workflow_dispatch: | ||
|
|
||
|
|
@@ -93,19 +81,44 @@ jobs: | |
| contents: read | ||
| id-token: write | ||
| steps: | ||
| - name: Check for e2e-relevant changes | ||
| id: changes | ||
| if: github.event_name == 'pull_request_target' | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| REPO: ${{ github.repository }} | ||
| # SYNC-WITH: push.paths filter above | ||
| run: | | ||
| FILES=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename') || { | ||
| echo "::warning::Failed to fetch PR files — running e2e tests as a precaution" | ||
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| } | ||
| if echo "$FILES" | grep -qE '\.go$|^go\.(mod|sum)$|^e2e/|^internal/scaffold/fullsend-repo/|^internal/security/hooks/|^internal/dispatch/gcf/mintsrc/|^internal/sentencetoken/english\.json$|^Makefile$|^\.github/workflows/e2e\.yml$|^\.github/actions/check-e2e-authorization/|^scripts/check-e2e-authorization\.sh$'; then | ||
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "::notice::No e2e-relevant files changed — skipping tests" | ||
| echo "relevant=false" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| - uses: actions/checkout@v4 | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| with: | ||
| ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || github.sha }} | ||
| persist-credentials: false | ||
|
|
||
| - uses: actions/setup-go@v5 | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| with: | ||
| go-version-file: go.mod | ||
|
|
||
| - name: Install Playwright system dependencies | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| run: npx playwright install-deps chromium | ||
|
|
||
| - name: Check for secrets | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| id: secrets-check | ||
| run: | | ||
| if [ -z "$E2E_GITHUB_SESSION_B64" ]; then | ||
|
|
@@ -118,7 +131,7 @@ jobs: | |
| E2E_GITHUB_SESSION_B64: ${{ secrets.E2E_GITHUB_SESSION }} | ||
|
|
||
| - name: Decode session | ||
| if: steps.secrets-check.outputs.available == 'true' | ||
| if: steps.changes.outputs.relevant != 'false' && steps.secrets-check.outputs.available == 'true' | ||
| run: | | ||
| SESSION_FILE="${RUNNER_TEMP}/github-session.json" | ||
| printf '%s' "$E2E_GITHUB_SESSION_B64" | base64 -d > "$SESSION_FILE" | ||
|
|
@@ -127,14 +140,14 @@ jobs: | |
| E2E_GITHUB_SESSION_B64: ${{ secrets.E2E_GITHUB_SESSION }} | ||
|
|
||
| - name: Authenticate to GCP | ||
| if: steps.secrets-check.outputs.available == 'true' | ||
| if: steps.changes.outputs.relevant != 'false' && steps.secrets-check.outputs.available == 'true' | ||
| uses: google-github-actions/auth@v2 | ||
| with: | ||
| workload_identity_provider: ${{ secrets.E2E_GCP_WIF_PROVIDER }} | ||
| service_account: ${{ secrets.E2E_GCP_SERVICE_ACCOUNT }} | ||
|
|
||
| - name: Run e2e tests | ||
| if: steps.secrets-check.outputs.available == 'true' | ||
| if: steps.changes.outputs.relevant != 'false' && steps.secrets-check.outputs.available == 'true' | ||
| run: make e2e-test | ||
| env: | ||
| E2E_SCREENSHOT_DIR: ${{ runner.temp }}/e2e-screenshots | ||
|
|
@@ -144,7 +157,7 @@ jobs: | |
| E2E_GCP_PROJECT_ID: ${{ secrets.E2E_GCP_PROJECT_ID }} | ||
|
|
||
| - name: Upload debug screenshots | ||
| if: always() && steps.secrets-check.outputs.available == 'true' | ||
| if: always() && steps.changes.outputs.relevant != 'false' && steps.secrets-check.outputs.available == 'true' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: e2e-screenshots-${{ github.event_name == 'pull_request_target' && github.event.pull_request.number || github.run_id }} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] curl-pipe-to-shell
New script downloads and executes code from GitHub via curl | sh, pinned to a commit SHA but without checksum verification of the downloaded content.
Suggested fix: Add SHA-256 checksum verification of the downloaded install.sh before piping to sh.