ci: Setting up conditional CI runs depending on PR comment trigger #3580
Conversation
michalChrobot
requested review from
a team,
EmandM and
NoelStephensUnity
as code owners
There was a problem hiding this comment.
I know I'm a bit late to the conversation, but I think I'd prefer to have everything run by default, and the /ci ignore can ignore all the checks, and then /ci ngo can re-enable the checks. Having to remember to make an extra comment on every PR sounds like it'll frustrate me over time.
@michalChrobot |
|
hmm, ok, let me see what I can do |
|
FYI I corrected the trigger to run ONLY when package or testproject code gets changed. If we change for example a github action then there is also no reason to run CI tests but PR description (Testing section) should ensure we do some validation on those (which should anyway differ from usual CI run) |
|
At the end I would argue for keeping the comment trigger flow because
What do you think? Maybe we can try with the comments and see how annoying it will be, if it will be too much on annoyance then we can look into disabling the required check @EmandM @NoelStephensUnity Edit: actually I think (not 100% sure though) that the PR will be blocked from merging if any tests are still running but correct me here |
|
Ok @EmandM @NoelStephensUnity at the end I had some more difficulty with correctly guarding PR comment triggers (public repo) so since PRs are actually blocked from merging if some tests are running I can be fine with this version being without comment triggers and just automatic as before (so exactly as in @EmandM comment) In the future let's see but for now I'm ok with just merging this version, let me know (I modified PR description) |
|
/ci ngo |
… trigger (#3581) This PR is a backport of #3580, see the description there ## Testing & QA I verified that 1. PR trigger gets executed correctly when we have package or project changes 2. I verified that the fact of tests running blocks PR from merging ## Documentation I added explanation in jobs comments ## Jira ticket MTT-12903 ## Backports This is a backport of #3580
I believe it should be singular comment like Noel did above but generally |
|
@NoelStephensUnity @EmandM Since PR should be blocked either way if tests are running, it should work correctly but let me know if you will notice some unusual behavior |
…m 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
…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
3 participants
This PR aims to address the issue of "overdoing" the CI tests which due to distribution times often lead to very long wait times.
First thing is that I failed to notice that
targetsanddraftsconditions are configured as separate items in a list (see PR job trigger changes). In our CI, basic triggers in a list are combined with an OR operator, meaning the job will run if any of the conditions are met. This lead to test being executed on draft PRs.What I'm changing is
This change should improve our workflow with CI as well as save some resources for others
NOTE THAT
By default the PR trigger it's triggered if
Note that in other cases you can trigger it by writing a comment
/ci ngoin the PR thread (if for example you want to test it on draft PR)Additional stuff
I also used that occasion to remove disable-burst file. We are not using it and I included more advanced version in #3557 so if we ever need it we can reuse that one. Because of that I will remove it to avoid any confusion
Steps after approval
I will remove the mandatory PR trigger job from branch settings since now it will not always run (eg. only DOCS changes) and would block PRs otherwise
Backports
#3581
Testing & QA
I verified that
Documentation
I added explanation in jobs comments
Jira ticket
MTT-12903