ci: [Backport] Adding minimal required job set and validation preventing PRs from merging when tests are running #3691
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…m merging when tests are running (#3682) 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 N/A Comments were added to the jobs (soon I also plan to create doc describing our CI practices/learnings) Playtest) I tested scenarios with code changes and without code changes and the workflow works as expected Will do
michalChrobot
requested review from
NoelStephensUnity,
EmandM and
a team
as code owners
michalChrobot
deleted the
test-for-all-checks-pass-before-merging-backport
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
This is a backport of #3682. Same description apply
Jira ticket
N/A
Documentation
Same as original PR
Testing & QA (How your changes can be verified during release Playtest)
Same as original PR
Backports
This is a backport of #3682