fix(ci): Improve workflow efficiency by optimizing runner usage and eliminating duplicate runs#1824
fix(ci): Improve workflow efficiency by optimizing runner usage and eliminating duplicate runs#1824jackluo923 wants to merge 8 commits intomainfrom
Conversation
The `filter-relevant-changes` job only performs git operations to detect changed paths. Using a GitHub-hosted runner avoids consuming self-hosted runner resources needed for heavier build jobs.
WalkthroughAdded a new reusable workflow Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-10-22T21:14:12.225ZApplied to files:
📚 Learning: 2025-10-22T21:02:31.113ZApplied to files:
📚 Learning: 2025-05-26T16:03:05.519ZApplied to files:
📚 Learning: 2025-07-01T14:52:02.418ZApplied to files:
📚 Learning: 2025-08-09T04:07:27.083ZApplied to files:
📚 Learning: 2025-10-22T21:01:31.391ZApplied to files:
📚 Learning: 2025-07-23T09:54:45.185ZApplied to files:
📚 Learning: 2025-05-27T20:04:51.498ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/clp-artifact-build.yaml (1)
45-47: Consider removing this workaround step.Now that the
filter-relevant-changesjob runs on a GitHub-hosted runner, this workaround for permission issues (typically needed for self-hosted or Docker-based workflows) is likely unnecessary. The step is harmless but represents technical debt.🔎 Proposed change
- - name: "Work around actions/runner-images/issues/6775" - run: "chown $(id -u):$(id -g) -R ." - shell: "bash" -
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/clp-artifact-build.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-05-27T20:04:51.498Z
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Applied to files:
.github/workflows/clp-artifact-build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: check-generated
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: check-generated
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (3)
.github/workflows/clp-artifact-build.yaml (3)
31-33: LGTM! Appropriate use of GitHub-hosted runner for lightweight job.The rationale is clear: this job only performs git operations for path filtering and doesn't require Docker or intensive resources, making it suitable for GitHub-hosted runners while preserving self-hosted runner capacity for heavier build jobs.
106-111: Conditional runner selection looks correct.The logic properly falls back to
ubuntu-24.04for forks whilst using the self-hosted runner array for the y-scope organisation. The YAML anchor approach cleanly eliminates duplication across all jobs requiring Docker resources.
492-492: The cache key concern is not applicable here. Theubuntu-jammy-lintjob runs clang-tidy tasks inside a Docker container (therun-on-imageaction), not on the host runner OS. Since both the y-scope self-hosted ubuntu-noble runner and fork ubuntu-24.04 runners use the same ubuntu-jammy container, the cache artifacts are compatible regardless of which runner executes the job. The hardcoded cache key is stable and appropriate for this use case.Likely an incorrect or invalid review comment.
jackluo923
changed the title
Prevents duplicate workflow runs when pushing to PR branches. PRs are tested via the pull_request event; the push event is only needed for testing commits merged to main.
| branches: | ||
| - "main" |
There was a problem hiding this comment.
Does this mean workflows won't run on non-main branches in forks unless a pull request is opened? Though, i guess users can manually workflow_dispatch?
There was a problem hiding this comment.
@kirkrodrigues is this something you mentioned that we want to avoid? though i vaguely remember seeing something similar in our workflows previously
There was a problem hiding this comment.
assuming the triggering condition change is ok, we may prefer
| branches: | |
| - "main" | |
| branches: ["main"] |
There was a problem hiding this comment.
Yah. CI only triggers when pushed to their fork's main branch after this change if they don't submit a PR. Otherwise, if they PR via their own branch into our repo, our CI would be triggered. I can revert this if that's not something we want.
There was a problem hiding this comment.
@kirkrodrigues is this something you mentioned that we want to avoid? though i vaguely remember seeing something similar in our workflows previously
Correct, I would prefer we didn't disable workflow runs on pushes to branches. I feel like if we want to get rid of the duplication, we should employ a different solution of some sort.
Replace the push branch restriction with step-security/skip-duplicate-actions to handle duplicate runs when both push and pull_request events trigger. This action is: - Used by 9,234+ repositories including haskell-language-server, CNCF TOC, and MegaLinter - A security-hardened drop-in replacement for fkirc/skip-duplicate-actions - More flexible than limiting push to main branch only
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/clp-artifact-build.yaml (1)
21-26: Clarify interaction between workflow concurrency and skip-duplicate-actions.The workflow has both:
- Workflow-level
concurrency(Lines 21-26) which cancels in-progress runsskip-duplicate-actions(Lines 29-42) which skips starting duplicate runsThese mechanisms are complementary but could lead to subtle interactions. For example, if a push triggers first and then a PR event arrives for the same commit,
skip-duplicate-actionswill skip the PR run. However, if the PR triggers first, the push event might be cancelled mid-flight by concurrency controls.Consider adding a brief comment near the concurrency block explaining how these two mechanisms work together to prevent redundant work.
Also applies to: 29-42
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/clp-artifact-build.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-artifact-build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: check-generated
- GitHub Check: check-generated
🔇 Additional comments (2)
.github/workflows/clp-artifact-build.yaml (2)
44-50: Verify downstream job behaviour whenfilter-relevant-changesis skipped.When
skip-duplicate-checksetsshould_skipto'true',filter-relevant-changeswill be skipped and its outputs will be empty strings. Downstream jobs likecentos-stream-9-deps-image(Line 119) checkneeds.filter-relevant-changes.outputs.centos_stream_9_image_changed == 'true', which will evaluate tofalsewhen the output is empty—causing those jobs to be skipped as expected.However, jobs like
centos-stream-9-binaries(Line 230-232) have conditions that checkneeds.filter-relevant-changes.outputs.clp_changed == 'true'as a fallback. Whenfilter-relevant-changesis skipped,clp_changedwill be empty, so this fallback won't trigger—which appears to be the intended behaviour (skip the entire workflow on duplicate).Please confirm this cascading skip behaviour aligns with the PR's intent.
121-128: Good use of YAML anchor for DRY runner configuration.The anchor
&runnerwith conditional logic for repository owner elegantly handles both self-hosted (y-scope) and GitHub-hosted (forks) scenarios. This pattern is clean and consistent with the repository's preference for YAML anchors to avoid duplication (based on learnings).
step-security version requires a paid subscription.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/clp-artifact-build.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-05-27T20:04:51.498Z
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
.github/workflows/clp-artifact-build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: check-generated
- GitHub Check: rust-checks
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: check-generated
🔇 Additional comments (1)
.github/workflows/clp-artifact-build.yaml (1)
123-130: LGTM: Runner configuration enables fork support.The conditional runner configuration is well-designed. It allows the y-scope organization to continue using self-hosted runners for resource-intensive Docker builds while enabling forks to run on GitHub-hosted runners. The YAML anchor pattern aligns with the repository's preference for reducing duplication.
Based on learnings, YAML anchors/aliases are acceptable and preferred in this repository to avoid duplication.
| skip-duplicate-check: | ||
| name: "skip-duplicate-check" | ||
| runs-on: "ubuntu-24.04" | ||
| outputs: | ||
| should_skip: "${{steps.skip_check.outputs.should_skip}}" | ||
| steps: | ||
| - id: "skip_check" | ||
| uses: "fkirc/skip-duplicate-actions@v5" | ||
| 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\"]" |
There was a problem hiding this comment.
Pin the action to a commit SHA instead of a version tag.
The action references @v5, but this repository pins GitHub Actions to commit SHAs for supply-chain security and to prevent unexpected changes when tags are moved.
🤖 Prompt for AI Agents
.github/workflows/clp-artifact-build.yaml lines 29-44: the workflow uses
fkirc/skip-duplicate-actions@v5 which should be pinned to a commit SHA for
supply-chain security; replace the tag ref with the exact commit SHA for the v5
release (e.g., fkirc/skip-duplicate-actions@<commit-sha>) by finding the commit
SHA on the action's GitHub repository that corresponds to v5 and update the
"uses" line accordingly, keeping all existing inputs the same.
| 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" |
There was a problem hiding this comment.
🧩 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 -20Repository: 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.yamlRepository: 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.yamlRepository: 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:
- Adding
if: always() && (result checks)to jobs that must handle skipped upstream jobs, OR - Adding explicit status checks like
needs.filter-relevant-changes.result == 'skipped'in conditions, OR - 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.
Create a reusable workflow for skip-duplicate-check that can be used by other workflows in the future.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm. mostly just nitpicking except that one request about clarification on concurrency: conflicts
| # 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\"]" |
There was a problem hiding this comment.
| do_not_skip: "[\"schedule\", \"workflow_dispatch\"]" | |
| do_not_skip: >- | |
| ["schedule", "workflow_dispatch"] |
| @@ -0,0 +1,40 @@ | |||
| name: "skip-duplicate-check" | |||
|
|
|||
| # Reusable workflow to skip duplicate runs when both push and pull_request events trigger on the | |||
There was a problem hiding this comment.
in this docstring, can we also explain how this workflow interacts / may conflict with the caller workflow's concurrency.cancel-in-progress settings?
| should_skip: "${{steps.skip_check.outputs.should_skip}}" | ||
| steps: | ||
| - id: "skip_check" | ||
| uses: "fkirc/skip-duplicate-actions@v5" |
There was a problem hiding this comment.
| 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
| outputs: | ||
| should_skip: "${{steps.skip_check.outputs.should_skip}}" | ||
| steps: | ||
| - id: "skip_check" |
There was a problem hiding this comment.
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
| cancel-in-progress: "${{github.ref != 'refs/heads/main'}}" | ||
|
|
||
| jobs: | ||
| skip-duplicate-check: |
There was a problem hiding this comment.
maybe renaming this as check-duplicate-run? like other steps place the verb at the beginning of the ids
3 participants
Description
Improve CI workflow efficiency with two changes:
1. Use GitHub-hosted runner for lightweight path filtering job
The
filter-relevant-changesjob only performs git operations to detect changed paths. Moving it to a GitHub-hosted runner avoids consuming self-hosted runner resources needed for heavier build jobs.2. Use skip-duplicate-actions to prevent duplicate workflow runs
When pushing to a PR branch, both
pushandpull_requestevents trigger, causing duplicate workflow runs. This PR adds fkirc/skip-duplicate-actions to detect and skip duplicate runs.How it works:
pushandpull_requesttrigger on the same commit, the duplicate is skippeddo_not_skip)filter-relevant-changesis skippedWhy this action:
Validation performed
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.