Skip to content

Commit e707575

Browse files
joeyparrishjoeyparrish
authoredJan 17, 2025
ci: Switch pull_request_target to pull_request (shaka-project#66)
With this change, GitHub will start to enforce "require approval" settings for the repo, which `pull_request_target` allows PR authors to get around. To stop using `pull_request_target`, we also have to stop using `vars`, which are not available on `pull_request` triggers. Instead, we use the "environments" trick used by Shaka Packager's workflows before `vars` was introduced to GitHub Actions. This is a safer system.
1 parent 0f6154f commit e707575

File tree

3 files changed

+94
-24
lines changed

3 files changed

+94
-24
lines changed
 

‎.github/workflows/build.yaml

+29-15
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,30 @@ on:
2525
ref:
2626
required: true
2727
type: string
28+
# If true, start a debug SSH server on failures.
29+
debug:
30+
required: false
31+
type: boolean
32+
default: false
33+
# If true, enable self-hosted runners in the build matrix.
34+
self_hosted:
35+
required: false
36+
type: boolean
37+
default: false
2838

2939
# Runs on manual trigger.
3040
workflow_dispatch:
41+
inputs:
42+
# If true, start a debug SSH server on failures.
43+
debug:
44+
required: false
45+
type: boolean
46+
default: false
47+
# If true, enable self-hosted runners in the build matrix.
48+
self_hosted:
49+
required: false
50+
type: boolean
51+
default: false
3152

3253

3354
# NOTE: The versions of the software we build are stored in versions.txt.
@@ -41,11 +62,9 @@ defaults:
4162
shell: bash
4263

4364
jobs:
44-
# Configure the build matrix based on repo variables. The list of objects in
45-
# the build matrix contents can't be changed by conditionals, but it can be
46-
# computed by another job and deserialized. This uses
47-
# vars.ENABLE_SELF_HOSTED to determine the build matrix, based on the
48-
# metadata in build-matrix.json.
65+
# Configure the build matrix based on inputs. The list of objects in the
66+
# build matrix contents can't be changed by conditionals, but it can be
67+
# computed by another job and deserialized.
4968
matrix_config:
5069
runs-on: ubuntu-latest
5170
outputs:
@@ -62,11 +81,11 @@ jobs:
6281
shell: node {0}
6382
run: |
6483
const fs = require('fs');
65-
const enableDebug = "${{ vars.ENABLE_DEBUG }}" != '';
66-
const enableSelfHosted = "${{ vars.ENABLE_SELF_HOSTED }}" != '';
84+
const enableDebug = ${{ inputs.debug }};
85+
const enableSelfHosted = ${{ inputs.self_hosted }};
6786
68-
// Use ENABLE_SELF_HOSTED to decide what the build matrix below
69-
// should include.
87+
// Use enableSelfHosted to decide what the build matrix below should
88+
// include.
7089
const buildMatrix = JSON.parse(fs.readFileSync("${{ github.workspace }}/repo-src/build-matrix.json"));
7190
const {hosted, selfHosted} = buildMatrix;
7291
const matrix = enableSelfHosted ? hosted.concat(selfHosted) : hosted;
@@ -76,11 +95,6 @@ jobs:
7695
process.env['GITHUB_OUTPUT'],
7796
`MATRIX=${ JSON.stringify(matrix) }\n`);
7897
79-
// Output the debug flag directly.
80-
fs.appendFileSync(
81-
process.env['GITHUB_OUTPUT'],
82-
`ENABLE_DEBUG=${ enableDebug }\n`);
83-
8498
// Log the outputs, for the sake of debugging this script.
8599
console.log({enableDebug, enableSelfHosted, matrix});
86100
@@ -212,4 +226,4 @@ jobs:
212226
uses: mxschmitt/action-tmate@v3.6
213227
with:
214228
limit-access-to-actor: true
215-
if: failure() && vars.ENABLE_DEBUG != ''
229+
if: failure() && inputs.debug

‎.github/workflows/settings.yaml

+52
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# Copyright 2022 Google LLC
2+
#
3+
# Use of this source code is governed by a BSD-style
4+
# license that can be found in the LICENSE file or at
5+
# https://developers.google.com/open-source/licenses/bsd
6+
7+
# A reusable workflow to extract settings from a repository.
8+
# To enable a setting, create a "GitHub Environment" with the same name.
9+
#
10+
# This enables per-repo settings that aren't copied to a fork. This is better
11+
# than "vars" or "secrets", since those would require the use of
12+
# `pull_request_target` instead of `pull_request` triggers, which come with
13+
# additional risks such as the bypassing of "require approval" rules for
14+
# workflows.
15+
#
16+
# Without a setting for flags like "self_hosted", test workflows for a fork
17+
# would time out waiting for self-hosted runners that the fork doesn't have.
18+
name: Settings
19+
20+
# Runs when called from another workflow.
21+
on:
22+
workflow_call:
23+
outputs:
24+
self_hosted:
25+
description: "Enable jobs requiring a self-hosted runner."
26+
value: ${{ jobs.settings.outputs.self_hosted }}
27+
debug:
28+
description: "Enable SSH debugging when a workflow fails."
29+
value: ${{ jobs.settings.outputs.debug }}
30+
31+
jobs:
32+
settings:
33+
runs-on: ubuntu-latest
34+
outputs:
35+
self_hosted: ${{ steps.settings.outputs.self_hosted }}
36+
debug: ${{ steps.settings.outputs.debug }}
37+
env:
38+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
39+
steps:
40+
- id: settings
41+
run: |
42+
environments=$(gh api /repos/${{ github.repository }}/environments)
43+
for name in self_hosted debug; do
44+
exists=$(echo $environments | jq ".environments[] | select(.name == \"$name\")")
45+
if [[ "$exists" != "" ]]; then
46+
echo "$name=true" >> $GITHUB_OUTPUT
47+
echo "\"$name\" enabled."
48+
else
49+
echo "$name=" >> $GITHUB_OUTPUT
50+
echo "\"$name\" disabled."
51+
fi
52+
done

‎.github/workflows/test.yaml

+13-9
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,9 @@
1616
name: Test
1717

1818
# Runs when a PR is uploaded or revised. Builds ffmpeg and ffprobe on all OS &
19-
# CPU combinations. Uses pull_request_target rather than pull_request for
20-
# access to configuration variables. Since no secrets are accessed or passed
21-
# to the build workflow, this is safe. Only the release workflow uses secrets.
19+
# CPU combinations.
2220
on:
23-
pull_request_target:
21+
pull_request:
2422
types: [opened, synchronize, reopened]
2523

2624
# If another instance of this workflow is started for the same PR, cancel the
@@ -30,14 +28,20 @@ concurrency:
3028
group: ${{ github.workflow }}-${{ github.event.number }}
3129
cancel-in-progress: true
3230

33-
# NOTE: Set the repository variable ENABLE_DEBUG to enable debugging via tmate
34-
# on failure.
35-
# NOTE: Set the repository variable ENABLE_SELF_HOSTED to enable self-hosted
36-
# runners such as linux-arm64. This is set on the official repo, but forks
37-
# will have to opt in after setting up their own self-hosted runners.
31+
# NOTE: Create an environment called "debug" to enable debugging via tmate on
32+
# failure.
33+
# NOTE: Create an environment called "self_hosted" to enable self-hosted
34+
# runners from build-matrix.json.
3835

3936
jobs:
37+
settings:
38+
name: Settings
39+
uses: ./.github/workflows/settings.yaml
40+
4041
build:
42+
needs: settings
4143
uses: ./.github/workflows/build.yaml
4244
with:
4345
ref: refs/pull/${{ github.event.number }}/merge
46+
self_hosted: ${{ needs.settings.outputs.self_hosted != '' }}
47+
debug: ${{ needs.settings.outputs.debug != '' }}

0 commit comments

Comments
 (0)
Please sign in to comment.