chore(ci): run ci checks conditionally

[blakepettersson]

This should prevent docs changes from having the need to run e2e tests etc, and prevent backend changes from needing to run ui tests, and vice versa.

This is similar to previous attempts (see #16706 and #13507), with the difference here that we add the if checks on each step rather than each job - the reason being that most of these jobs are required, and if we skip whole jobs any PR which does this will be left hanging indefinitely, so Github forces us to do this on a step level instead a composite job is added which checks the aggregated status of the matrix jobs, any of which can either be skipped, passed or failed. If they are skipped or passed, the composite job passes, otherwise it's marked as failed.

Once this is merged, a maintainer will need to switch the required check to the composite job for this to work.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[terrytangyuan]

Sounds great. We’ve taken a similar approach for Workflows as well

[agilgur5]

I commented on Slack too; as Terry said I had a PR for Workflows for this a couple months ago: argoproj/argo-workflows#12006

All the step-level ifs get pretty verbose and are easy to miss accidentally (or screw up if you need multiple conditions), so in my PR I used job-level skips and then for the E2E matrix added an aggregate check that is used as the required CI check on GitHub. See the PR for more implementation details on that.

Overall I think the skips have been a huge boon as lots of PRs no longer run all E2E tests, for instance, which means flakey tests don't affect them and the CI finishes much faster.
When it comes to possible misses in the changed file list, for an active contributor, it was easy to spot when a file was missing since you expect a job to run and then it doesn't -- first miss I found was in argoproj/argo-workflows#12557 last week and it took months for me to find something (i.e. the initial list was very accurate)

[todaywasawesome]

@blakepettersson According to the Github docs, if a job is skipped, it won't be required.

If status checks are required for a repository, the required status checks must pass before you can merge your branch into the protected branch. For more information, see "About protected branches."

Note: A job that is skipped will report its status as "Success". It will not prevent a pull request from merging, even if it is a required check.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks

So as long as we skip the jobs, they shouldn't block merging.

[blakepettersson]

@blakepettersson According to the Github docs, if a job is skipped, it won't be required.

If status checks are required for a repository, the required status checks must pass before you can merge your branch into the protected branch. For more information, see "About protected branches."

Note: A job that is skipped will report its status as "Success". It will not prevent a pull request from merging, even if it is a required check.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks

So as long as we skip the jobs, they shouldn't block merging.

@todaywasawesome it doesn't seem to be the case with c37ed7d 😞

[blakepettersson]

@todaywasawesome it seems like the tests which never completes are only the matrix tests - but the solution with @agilgur5 has seems to do the trick, assuming we modify the Required check to be set on test-e2e-composite-result rather than the test-e2e matrix tests (someone with at least @approvers-ci access would need to work their magic though, since I don't have that prerequisite access)

[agilgur5]

it seems like the tests which never completes are only the matrix tests

yea indeed matrices are an exception. In my PR, I referenced issues actions/runner#952 and https://github.com/orgs/community/discussions/9141 and the workaround from https://github.com/orgs/community/discussions/9141#discussioncomment-2296809 and https://github.com/orgs/community/discussions/26822#discussioncomment-3305794.

also jobs that depend on skipped jobs can have gotchas as well, which is why I modified needs in my PR of some jobs that were not strictly dependent (but rather may fail if another failed).

(someone with at least @approvers-ci access would need to work their magic though, since I don't have that prerequisite access)

confirming that is one of the requirements as well. I had to ask Terry to change the checks for Workflows since I didn't/don't have that access either.

also noting that due to the new checks, some PRs will also have to be rebased as well in order to run the new jobs. PRs with checks already passing I believe are fine, but any future runs will require those.
I figure that is particularly worth mentioning given CD's PR backlog -- it will cause some churn for folks to be aware of.

[gdsoumya]

LGTM

[ishitasequeira]

This looks great. LGTM!!

[blakepettersson]

@ishitasequeira (or @rbreeze or some other maintainer) - for this to be able to be merged we need to remove the required checks on the E2E test checks. Once this PR has been merged we'd then need to set the test-e2e-composite-result to required, which will become the new required check instead.

[ishitasequeira]

@blakepettersson , unfortunately I do not seem to have permissions to update the setting. Most likely someone with owner permissions might be able to update that.

@alexmt can you help with updating the setting?