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: Schedule PR jobs based on commit specific changes #7132

Merged
merged 1 commit into from
Nov 17, 2022
Merged

CI: Schedule PR jobs based on commit specific changes #7132

merged 1 commit into from
Nov 17, 2022

Conversation

microdev1
Copy link
Collaborator

The goal here was to schedule PR jobs based on file changes specific to a commit instead of diff between base and head branch of the PR, this presented two (that I know of) pitfalls:

  1. As @jepler pointed out on discord: "suppose commit 1 breaks everything and commit 2 fixes 50% of the thing"
    • Fix: Include all the unsuccessful jobs from the most recent workflow run.
  2. Only scheduling based on changes in the latest commit and ignoring changes in the previous commits.
    • Fix: Schedule jobs based on diff between head commit and the most recent commit with a workflow run.

Process Flow:

  • The code utilizes GitHub's GraphQL API.
  • It fetches all the commits in the PR and iterates over them in the order of most to least recent.
  • The commit that triggered the current workflow is skipped.
  • A commit is selected based on workflow == "Build CI" and conclusion != "Success".
  • The checkSuite ID of the commit is then used to fetch the unsuccessful checkRuns.
  • tj-actions/changed-files is used to get changed files based on diff between head commit and the most recent commit with a workflow run. If no commit with a workflow run was found, the comparison is between base and head commit.
  • The unsuccessful jobs and changed files are then passed to the set-matrix job which schedules the PR jobs.

@microdev1
Copy link
Collaborator Author

To test locally: (Guide to get GITHUB_TOKEN)

REPO=adafruit/circuitpython PULL=7132 GITHUB_TOKEN=*** EXCLUDE_COMMIT=30f07fbf22bfb7e19e27f062d1ab39ab83e7b067 python tools/ci_changes_per_commit.py

@dhalbert
Copy link
Collaborator

dhalbert commented Oct 26, 2022

So, if I understand correctly, the idea is that a new commit to a PR would run only jobs that failed due to the previous commit(s)?

Suppose the new commit broke builds that passed in the previous commits? Or would the new commit still trigger a rebuild of all the affected builds based on the scope of some file change?

@microdev1
Copy link
Collaborator Author

So, if I understand correctly, the idea is that a new commit to a PR would run only jobs that failed due to the previous commit(s)? Suppose the new commit broke builds that passed in the previous commits? Or would the new commit still trigger a rebuild of all the affected builds based on the scope of some file change?

The jobs will be run based on the changed files between new commit and the most recent commit with a workflow run plus the jobs that failed in the previous workflow run will also be added if not already present.

@jepler jepler removed their request for review October 26, 2022 21:05
@jepler
Copy link
Member

jepler commented Oct 26, 2022

I'm removing myself as a reviewer. Thanks for taking into account my comment on the basic idea. But at least right this second I'm not able to think too hard about the specifics of this proposed change.

@dhalbert
Copy link
Collaborator

@microdev1 I think I will merge this after beta.4. I didn't see anything to change, but it's complicated, s let's test the workflow in action after the next release.

@tannewt
Copy link
Member

tannewt commented Nov 15, 2022

@dhalbert Still want this in? I'm concerned we'll miss broken but not fixed issues.

@dhalbert
Copy link
Collaborator

@dhalbert Still want this in? I'm concerned we'll miss broken but not fixed issues.

I do not understand this well enough to have an opinion right now.

@microdev1
Copy link
Collaborator Author

@tannewt Can you elaborate on "we'll miss broken but not fixed issues".

@tannewt
Copy link
Member

tannewt commented Nov 16, 2022

@tannewt Can you elaborate on "we'll miss broken but not fixed issues".

If commit A is run and fails on boards 1 and 2 then commit B is added to fix 2 (but not 1), then the CI view will show green even though 1 is still broken. I think you need to run tests based on all changes since the main branch.

@microdev1
Copy link
Collaborator Author

If commit A is run and fails on boards 1 and 2 then commit B is added to fix 2 (but not 1), then the CI view will show green even though 1 is still broken. I think you need to run tests based on all changes since the main branch.

This won't be the case as failed jobs from the most recent workflow run of the PR are also scheduled.

@tannewt
Copy link
Member

tannewt commented Nov 16, 2022

Ah, ok. Neat! I'm ok merging this in now.

@tannewt tannewt merged commit c525322 into adafruit:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants