Skip to content

Commit c1725c2

Browse files
ci: Adding minimal required job set and validation preventing PRs from merging when tests are running (#3682)
## Purpose of this PR This PR aims to address 3 problems that we currently have 1. Quite often if no checks are required it's easy to undertest a PR which leads to simple whitespace errors and such. Because of that I created `pr_minimal_required_checks` which consists of PVP checks and standard check which I will require on all PRs as minimum 2. Emma noticed that PRs are allowed to be merged when jobs are running but none yet failed. This is an issue created by #3580 and the fact that we want to run CI conditionally (for example we don't want to run it when we only have DOCS changes) so we had to remove PR trigger as required job in branch protection rules. This creates a problem that without required check PR is allowed to be merged until a test fails 3. Another problem that I spotted is that Documentation~ folder is actually located inside package folder so this check implemented there won't work correctly either way The solution that this PR proposes is implementation of 2 independent checks (triggered by expression trigger) where 1. `pr_minimal_required_checks` will trigger on all PRs targetting develop or release branches. It consist of simplest PVP and standards checks 2. `pr_code_changes_checks` will trigger on PRs that are modifying files under relevant package or project related paths (see expression trigger) Those jobs WON'T be required by the branch protection rules but I introduced new GitHub action of Yamato PR Supervisor which WILL be required and how it works is that it monitors checks running on PRs and will fail if any of them fails or pass if all checks are green. In this way we can have a workflow that both runs only necessary tests but also gates PRs from merging too early. Note that you can still use `/ci ngo` comment trigger to run the check on draft branch or branches not targeting develop nor release branches After this is approved and merged I will add this check as required on PRs in branch protection rules ### Jira ticket N/A ## Documentation Comments were added to the jobs (soon I also plan to create doc describing our CI practices/learnings) ## Testing & QA (How your changes can be verified during release Playtest) I tested scenarios with code changes and without code changes and the workflow works as expected ## Backports Will do
1 parent c2fc8fe commit c1725c2

File tree

2 files changed

+114
-15
lines changed

2 files changed

+114
-15
lines changed
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# GitHub Actions workflow to monitor Yamato CI job state on pull requests
2+
# We are using https://cli.github.com/manual/gh_pr_checks
3+
# The aim is to ensure that conditionally triggered Yamato jobs are completed successfully before allowing merges
4+
5+
# This job will be required in branch protection rules for develop, develop-2.0.0, and release/* branches. It's only goal will be to ensure that Yamato jobs are completed successfully before allowing Pr to merge.
6+
# Note that conditional jobs will have 30s to show which is always the cas since they are showing up as soon as in distribution stage.
7+
8+
name: Yamato PR Supervisor
9+
10+
on:
11+
pull_request:
12+
types: [opened, synchronize, reopened]
13+
branches:
14+
- develop
15+
- develop-2.0.0
16+
- release/*
17+
18+
concurrency:
19+
group: pr-${{ github.event.pull_request.number }}
20+
cancel-in-progress: true
21+
22+
jobs:
23+
yamato-supervisor:
24+
runs-on: ubuntu-latest
25+
timeout-minutes: 720
26+
steps:
27+
- name: Checkout repository
28+
uses: actions/checkout@v4
29+
30+
- name: Wait and Verify Yamato Job Status
31+
env:
32+
GH_TOKEN: ${{ secrets.GH_TOKEN }}
33+
PR_NUMBER: ${{ github.event.pull_request.number }}
34+
run: |
35+
set -e
36+
37+
38+
MAX_ATTEMPTS=$((12*60))
39+
INTERVAL=60
40+
41+
sleep $INTERVAL
42+
for ((i=1;i<=MAX_ATTEMPTS;i++)); do
43+
echo "Polling PR checks (attempt $i/$MAX_ATTEMPTS)..."
44+
45+
# We want to watch for pending checks beside this check
46+
checks=$(gh pr checks $PR_NUMBER --json name,state --jq '[ .[] | select(.name != "yamato-supervisor") ]')
47+
48+
pending=$(echo "$checks" | jq '[.[] | select(.state == "PENDING")] | length')
49+
skipping=$(echo "$checks" | jq '[.[] | select(.state == "SKIPPED")] | length')
50+
passed=$(echo "$checks" | jq '[.[] | select(.state == "SUCCESS")] | length')
51+
failed=$(echo "$checks" | jq '[.[] | select(.state == "FAILURE")] | length')
52+
53+
echo "Pending checks: $pending, Skipping checks: $skipping", Passed checks: $passed, Failed checks: $failed
54+
55+
if [[ "$failed" -gt 0 ]]; then
56+
echo "A check has failed! Failing fast."
57+
exit 1
58+
fi
59+
60+
if [[ "$pending" -eq 0 ]] && [[ "$passed" -gt 0 ]]; then
61+
echo "All non-supervisor checks are completed!"
62+
exit 0
63+
fi
64+
65+
sleep $INTERVAL
66+
done

.yamato/_triggers.yml

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,23 @@
44
# DESCRIPTION--------------------------------------------------------------------------
55
# This configuration defines three main CI trigger patterns:
66
# 1. Pull Request Validation: Validation performed on PR basis
7+
# - we have mandatory minimal checks that run on all PRs (even if only docs are changed)
8+
# - we have more extensive pr_code_changes_checks that run if code is changed
79
# 2. Nightly Development: Test set run nightly (validates most important test cases)
810
# 3. Weekly Full Validation: Test set run weekly (validates all test cases to prevent any surprises)
911
# Each pattern represents different balance between validation depth, execution time and CI resource usage
1012

1113
# TRIGGER PATTERNS-------------------------------------------------------------------
1214

1315
# Pull Request:
14-
# This test validates Standards, Package tests, Project tests and Desktop standalone tests to ensure that main platforms are covered
15-
# Focuses on critical validation paths that we should validate before merging PRs. It also cancels previous runs on new commits
16-
# By default it's triggered if
16+
# We have two PR triggers:
17+
# 1) Minimal PR checks that run on all PRs (even if only docs are changed)
18+
# 2) More extensive pr_code_changes_checks that run if code is changed. This test validates Standards, Package tests, Project tests and Desktop standalone tests to ensure that main platforms are covered
19+
# By default pr_minimal_required_checks it's triggered if
1720
# 1) PR targets develop, develop-2.0.0 or release branches
1821
# 2) PR is not a draft
19-
# 3) PR changes files in package or testproject folders (doesn't run on for example DOCS only changes)
22+
# Then pr_code_changes_checks it's triggered if the same conditions apply plus:
23+
# 1) PR changes code in com.unity.netcode.gameobjects package (Editor, Runtime or Tests folders) or in testproject folder or package.json file
2024

2125
# Note that in other cases you can trigger it by writing a comment "/ci ngo" in the PR thread
2226

@@ -40,16 +44,32 @@
4044
# It's important to ensure that all dependencies exist (this can be verified in Yamato) since a modification in parameters may result in a given job not being generated, and thus we will not be able to run such erroneous job.
4145

4246

43-
#-----------------------------------------------------------------------------------
47+
#-----------------------------------------------------------------------------------
48+
49+
# After some experimenting with CI setups we discovered that even though sometimes we don't need CI to run (no reason to run package tests if only Documentation is changed) there are some checks that devs may not realize but changes in seemingly unrelated files will cause their failures
50+
# This trigger was created to ensure that ALL PRs run this minimal check even when we don't need to run full tests
51+
pr_minimal_required_checks:
52+
name: Minimal PR checks
53+
dependencies:
54+
- .yamato/package-pack.yml#package_pack_-_ngo_win
55+
- .yamato/project-standards.yml#standards_ubuntu_testproject_trunk
56+
triggers:
57+
expression: |-
58+
(pull_request.comment eq "ngo" OR
59+
(pull_request.target eq "develop" OR
60+
pull_request.target eq "develop-2.0.0" OR
61+
pull_request.target match "release/*")) AND
62+
NOT pull_request.draft
63+
cancel_old_ci: true
4464

4565

4666
# Run all relevant tasks when a pull request targeting the develop or release branch is created or updated.
4767
# In order to have better coverage we run desktop standalone tests with different configurations which allows to mostly cover for different platforms, scripting backends and editor versions.
4868
# This job will FIRST run "run_quick_checks" jobs (defined in _run-all.yml) since it's the dependency of project pack jobs which is on the lowest dependency tier. This runs the fastest checks (like PVP or code standards) and ONLY IF those pass it will run the rest of the tests.
4969
# This optimization allows to speed up feedback look for any "obvious" errors and save resources.
5070
# Since standards job is a part of initial checks it's not present as direct dependency here!!!!!!!!!!!!!!!!!!!!
51-
pull_request_trigger:
52-
name: Pull Request Trigger (develop, develop-2.0.0, & release branches)
71+
pr_code_changes_checks:
72+
name: Code changes PR checks
5373
# Run the following tests on a selection of different desktop platforms
5474
dependencies:
5575
# Run package EditMode and Playmode package tests on trunk and an older supported editor (6000.0)
@@ -65,18 +85,31 @@ pull_request_trigger:
6585
- .yamato/desktop-standalone-tests.yml#desktop_standalone_test_testproject_win_il2cpp_6000.0
6686
- .yamato/cmb-service-standalone-tests.yml#cmb_service_standalone_test_testproject_ubuntu_il2cpp_trunk
6787
triggers:
68-
# Note that PR tests will run ONLY if we are changing package/sample code. If changes are let's say docs only no tests will be triggered
69-
# TODO: consider setting up and running tests from Examples/
70-
# TODO: or docs only changes are spelling/link check
7188
expression: |-
72-
pull_request.comment eq "ngo" OR
73-
((pull_request.target eq "develop" OR
89+
(pull_request.comment eq "ngo" OR
90+
(pull_request.target eq "develop" OR
7491
pull_request.target eq "develop-2.0.0" OR
75-
pull_request.target match "release/*") AND
92+
pull_request.target match "release/*")) AND
7693
NOT pull_request.draft AND
77-
(pull_request.changes.any match "com.unity.netcode.gameobjects/**" OR
78-
pull_request.changes.any match "testproject/**"))
94+
pull_request.changes.any match [
95+
"com.unity.netcode.gameobjects/Editor/**",
96+
"com.unity.netcode.gameobjects/Runtime/**",
97+
"com.unity.netcode.gameobjects/Tests/**",
98+
"testproject/**",
99+
"com.unity.netcode.gameobjects/package.json"
100+
] AND
101+
NOT pull_request.changes.all match [
102+
"**/Documentation~/**",
103+
"**/InternalDocs~/**",
104+
"**/*.md"
105+
]
79106
cancel_old_ci: true
107+
108+
109+
110+
111+
112+
80113

81114
# Run all tests on trunk on nightly basis.
82115
# Same subset as pull_request_trigger with addition of mobile/desktop/console tests and webgl builds

0 commit comments

Comments
 (0)