Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: only run jobs when relevant files have been changed [actions testing PR] #1

Closed
wants to merge 23 commits into from

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Oct 14, 2023

Fixes argoproj#10156
Fixes argoproj#10265

  • Note: these can be further segmented and improved as well, see "Future Work" below

Motivation

Make CI run faster for certain kinds of PRs, use less CI minutes (and just reduce unnecessary compute in general), and limit the blast radius of flaky tests as well

  • e.g. don't run E2E tests if only UI or only Docs have changed
    • or don't run UI CI when UI has not chaged

Modifications

Changed Files checks

Changes to needs for codegen and lint

  • both can run independently of each other and independently of tests

    • they should fail fast if the other one fails though, I suppose
  • otherwise this breaks some of the checks, since if tests are skipped, then codegen and lint are skipped too

Changes to Status Checks

Note that due to lack of GH features such as actions/runner#952 and https://github.com/orgs/community/discussions/9141, there is a workaround embedded here for the E2E test matrix

  • skipping the overall e2e-tests job will cause status checks for individual E2E tests to just hang indefinitely
    • so instead we can change the required status checks to check the new composite result
      • that or we'd have to put an if for every single step of the E2E tests and also check for changed files within each E2E test
  • the new e2e-tests-composite-result job aggregates all of the results as a workaround

Verification

This is the PR in my own fork that I am using for testing purposes!

Notes to Reviewers

  • See the "Changes to Status Checks" section above that we will need to implement
  • We will also need to be a bit careful in making sure that any new code is properly listed in the files lists

Future Work

  1. Further segment E2E tests -- not all tests necessarily need to run for all back-end changes
  2. Add to other GH Workflows, like the Docs Workflow?
    • would not be a huge efficiency increase, but would limit some unnecessary runs
  3. Add a check for argoexec-image job as well
    • this one may require some more effort to get everything necessary, particularly with repetition from .dockerignore etc

- e.g. don't run E2E tests if only UI or only Docs have changed
  - or don't run UI CI when UI has not chaged

- use [`tj-actions/changed-files`](https://github.com/tj-actions/changed-files) action for this
  - the most popular and full featured one I could find

- run `changed-files` in its own job that must run before all other jobs
  - have it `output` booleans for specific changes -- limit all the `changed_files` nuances, naming, syntax, etc to that one job
    - job `outputs` are also needed for skipping other jobs, as step outputs can't be directly accessed
      - see https://docs.github.com/en/actions/using-jobs/defining-outputs-for-jobs and https://docs.github.com/en/actions/learn-github-actions/contexts#needs-context
- have other jobs specify it in their `needs` and then skip if not needed with `if`

- use multi-output variant of `changed-files` YAML directive
  - so can check e2e vs docs vs UI etc
- use `any_modified` as that includes added, copied, modified, renamed, and deleted (ACMRD)
  - `any_changed` does not include deletions

NOTE: I realized after that `docs` isn't a job, `lint` is, so there's gonna be some follow-up commits
- and well need to test anyway too
- will also include the `all` list _after_ testing, since it would make everything run

Signed-off-by: Anton Gilgur <[email protected]>
- also tiny renames

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 changed the title [testing] ci: only run jobs when relevant files have been changed ci: only run jobs when relevant files have been changed [actions testing PR] Oct 14, 2023
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
- list merging is not supported in YAML natively, but `changed-files` appears to support it? https://github.com/tj-actions/changed-files/blob/2a10bef1b42044172f2e64d40beeb8fbad792438/test/changed-files.yml#L8-L11

Signed-off-by: Anton Gilgur <[email protected]>
- both can run independently of each other and independently of tests
  - they should fail fast if the other one fails though, I suppose

- otherwise this breaks some of the checks, since if tests are skipped, then codegen and lint are skipped too
  - or, well, that's my hypothesis at least -- will test by pushing this

Signed-off-by: Anton Gilgur <[email protected]>
- so now only lint should run...

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
This reverts commit 34671d7.

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5
Copy link
Owner Author

This was a testing PR; now that everything is confirmed working, I opened an upstream PR: argoproj#12006

@agilgur5 agilgur5 closed this Oct 14, 2023
Repository owner locked as resolved and limited conversation to collaborators Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better segmentation of tests Skip unnecessary CI workflows if only have documentary changes
1 participant