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] Reduce runners load for pulls that affect only doc or cpp changes #17541

Merged
merged 10 commits into from
Sep 8, 2022

Conversation

nicoloboschi
Copy link
Contributor

Motivation

  1. For doc or cpp changes now the Pulsar CI workflow requires runners to just skip steps.
  2. CPP builds run on different workflows and this can be optimized in a single workflow
  3. MacOS build runner is created even if there are only doc changes
  4. GO functions check can reuse the same runner

Modifications

  • Moved the check for doc changes to jobs level instead of steps. In case the condition of if returns false, no new runners will be spawned
  • Moved all CPP builds to the same workflow
  • Moved go functions checks to the same runner
  • Moved MacOs build as step after Pulsar CI workflow
  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 8, 2022
@lhotari
Copy link
Member

lhotari commented Sep 8, 2022

Moved the check for doc changes to jobs level instead of steps. In case the condition of if returns false, no new runners will be spawned

That solution is possible only when the check produced by the skipped build job isn't required That's why the skipping solution is needed at the build step level. It shouldn't be done at such a fine grain level. A workflow where the last step is the required check would be a way to solve this so that the builds wouldn't have to be scheduled at all. This would also simplify the workflows since we could remove the step level skipping.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please modify the workflows in a way so that you add a new fan-in build job called "{workflow name} - Checks completed" from all build jobs that previously used to be listed in .asf.yaml in the required checks. When we re-add required checks to .asf.yaml, we would only add the "{workflow name} - Checks completed" check as required. This way we can do the nice optimization that you have performed in this PR.

with:
action: wait

flaky-system-tests:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the exact reason for adding this? I would assume that it's possible to add matrix variables to cover this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but then the new step pulsar-ci-checks-completed needs these tests passing. Currently in the .asf.yaml they are not listed. So now it depends only on system-tests job and the result of this one doesn't impact the mergeablity of the pull

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes you are right.

@lhotari lhotari merged commit 7e86968 into apache:master Sep 8, 2022
nicoloboschi added a commit that referenced this pull request Sep 9, 2022
#17541)

* [ci] Reduce runners load for pulls that affects only doc or cpp changes

* add final jobs to mark required workflows as completed even if they are skipped

* test parametrized job id

* fix latest step

(cherry picked from commit 7e86968)
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.

2 participants