Skip to content

fix(ci): Improve workflow efficiency by optimizing runner usage and eliminating duplicate runs #1824

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.

Sign up for GitHub

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

Open
jackluo923 wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(ci): Improve workflow efficiency by optimizing runner usage and eliminating duplicate runs #1824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
745a05f
fix(ci): use GitHub-hosted runner for lightweight path filtering job
jackluo923 Dec 19, 2025
702cbbe
fix(ci): limit push event trigger to main branch only
jackluo923 Dec 19, 2025
b59fbd5
fix(ci): use skip-duplicate-actions to prevent duplicate workflow runs
jackluo923 Dec 20, 2025
e4a44de
Merge branch 'main' into ci
jackluo923 Dec 20, 2025
e0e89b6
fix(ci): switch to fkirc/skip-duplicate-actions
jackluo923 Dec 20, 2025
ada1991
fix(ci): use double quotes for yamllint compliance
jackluo923 Dec 20, 2025
fce9337
Merge branch 'main' into ci
jackluo923 Dec 21, 2025
9f49be4
refactor(ci): extract skip-duplicate-check to reusable workflow
jackluo923 Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions .github/workflows/clp-artifact-build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ concurrency:
cancel-in-progress: "${{github.ref != 'refs/heads/main'}}"

jobs:
skip-duplicate-check:
Copy link
Member

@junhaoliao junhaoliao Dec 22, 2025

Choose a reason for hiding this comment

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

maybe renaming this as check-duplicate-run? like other steps place the verb at the beginning of the ids

uses: "./.github/workflows/skip-duplicate-check.yaml"

filter-relevant-changes:
name: "filter-relevant-changes"
runs-on: &runner >-
${{
github.repository_owner == 'y-scope'
&& fromJSON('["self-hosted", "x64", "ubuntu-noble"]')
|| 'ubuntu-24.04'
}}
needs: "skip-duplicate-check"
if: "needs.skip-duplicate-check.outputs.should_skip != 'true'"
# This job only performs git operations to detect changed paths, so we use a GitHub-hosted
# runner to avoid consuming self-hosted runner resources needed for heavier build jobs.
runs-on: "ubuntu-24.04"
Comment on lines 32 to +38
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 21, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for all references to needs.filter-relevant-changes.outputs in the workflow
rg -n "needs\.filter-relevant-changes\.outputs" .github/workflows/clp-artifact-build.yaml | head -20

Repository: y-scope/clp

Length of output: 1636

🏁 Script executed:

#!/bin/bash
# Also get the file size first to determine how to read it
wc -l .github/workflows/clp-artifact-build.yaml

Repository: y-scope/clp

Length of output: 100

🏁 Script executed:

#!/bin/bash
# Get the lines around 46-52 and also around 121 and 234 to see the downstream jobs
echo "=== Lines 46-52 (filter-relevant-changes job) ==="
sed -n '46,52p' .github/workflows/clp-artifact-build.yaml
echo ""
echo "=== Lines 115-135 (around centos-stream-9-deps-image) ==="
sed -n '115,135p' .github/workflows/clp-artifact-build.yaml
echo ""
echo "=== Lines 228-248 (around centos-stream-9-binaries) ==="
sed -n '228,248p' .github/workflows/clp-artifact-build.yaml

Repository: y-scope/clp

Length of output: 2120

Verify downstream job behavior when filter-relevant-changes is skipped.

When skip-duplicate-check determines a run should be skipped, filter-relevant-changes will also be skipped (line 49). Image build jobs (lines 121, 154, 180, 206) depend on filter-relevant-changes and reference its outputs in their conditions without if: always(), so they will be skipped. Additionally, jobs like centos-stream-9-binaries (line 234) use the pattern success() || (...needs.filter-relevant-changes.outputs...), which is documented as unreliable in GitHub Actions when dependent jobs are skipped.

Ensure downstream jobs handle the skipped status correctly by:

  1. Adding if: always() && (result checks) to jobs that must handle skipped upstream jobs, OR
  2. Adding explicit status checks like needs.filter-relevant-changes.result == 'skipped' in conditions, OR
  3. Verifying this cascading skip is intentional and documented
🤖 Prompt for AI Agents 
.github/workflows/clp-artifact-build.yaml lines 46-52: downstream jobs reference
outputs from the filter-relevant-changes job but do not guard for the case where
that job is skipped, which can cause unreliable condition evaluation and
unintended cascading skips; update downstream job conditions to explicitly
handle skipped upstream jobs — either prepend always() (e.g., if: always() &&
(<existing result checks>)) for jobs that must still run/evaluate when upstream
was skipped, or add explicit checks for needs.filter-relevant-changes.result ==
'skipped' where appropriate, or document and confirm that the cascading skip
behavior is intentional and acceptable.

outputs:
centos_stream_9_image_changed: "${{steps.filter.outputs.centos_stream_9_image}}"
manylinux_2_28_x86_64_image_changed: "${{steps.filter.outputs.manylinux_2_28_x86_64_image}}"
Expand Down Expand Up @@ -104,7 +106,14 @@ jobs:
name: "centos-stream-9-deps-image"
if: "needs.filter-relevant-changes.outputs.centos_stream_9_image_changed == 'true'"
needs: "filter-relevant-changes"
runs-on: *runner
# Define a reusable runner configuration for jobs that require Docker and benefit from
# self-hosted runner resources. Falls back to GitHub-hosted runners for non-y-scope forks.
runs-on: &runner >-
${{
github.repository_owner == 'y-scope'
&& fromJSON('["self-hosted", "x64", "ubuntu-noble"]')
|| 'ubuntu-24.04'
}}
steps:
- uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683"
with:
Expand Down
40 changes: 40 additions & 0 deletions .github/workflows/skip-duplicate-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
name: "skip-duplicate-check"

# Reusable workflow to skip duplicate runs when both push and pull_request events trigger on the
Copy link
Member

@junhaoliao junhaoliao Dec 22, 2025

Choose a reason for hiding this comment

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

in this docstring, can we also explain how this workflow interacts / may conflict with the caller workflow's concurrency.cancel-in-progress settings?

# same commit. Other workflows can call this and check the `should_skip` output to avoid redundant
# work.
#
# Usage:
# jobs:
# skip-check:
# uses: "./.github/workflows/skip-duplicate-check.yaml"
#
# my-job:
# needs: "skip-check"
# if: "needs.skip-check.outputs.should_skip != 'true'"
# ...

on:
workflow_call:
outputs:
should_skip:
description: "Whether the workflow run should be skipped"
value: "${{jobs.check.outputs.should_skip}}"

jobs:
check:
name: "skip-duplicate-check"
runs-on: "ubuntu-24.04"
outputs:
should_skip: "${{steps.skip_check.outputs.should_skip}}"
steps:
- id: "skip_check"
Copy link
Member

@junhaoliao junhaoliao Dec 22, 2025

Choose a reason for hiding this comment

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

Can we omit this id specification? If we really want to avoid showing "uses: "fkirc/skip-duplicate-actions" as a workflow step on the workflow status page, we could specify something like name: "Evaluate whether this workflow run should be skipped" instead

uses: "fkirc/skip-duplicate-actions@v5"
Copy link
Member

@junhaoliao junhaoliao Dec 22, 2025

Choose a reason for hiding this comment

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

Suggested change
uses: "fkirc/skip-duplicate-actions@v5"
uses: "fkirc/skip-duplicate-actions@f75f66ce1886f00957d99748a42c724f4330bdcf" # v5.3.1

for security / reproducibility concerns, in case the publisher republish the version with different contents

with:
# Skip duplicate runs for the same content (e.g., when both push and pull_request trigger)
concurrent_skipping: "same_content"
skip_after_successful_duplicate: "true"
# Always run for scheduled builds and manual triggers.
# NOTE: This is a JSON array string (required by the action), with escaped quotes for
# yamllint compliance.
do_not_skip: "[\"schedule\", \"workflow_dispatch\"]"
Copy link
Member

@junhaoliao junhaoliao Dec 22, 2025

Choose a reason for hiding this comment

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

Suggested change
do_not_skip: "[\"schedule\", \"workflow_dispatch\"]"
do_not_skip: >-
["schedule", "workflow_dispatch"]

Loading