Refactor CI workflow with dynamic matrix configuration#124
Conversation
- Add configure job for dynamic matrix (full on main/tags, reduced on PRs) - Merge build-wasm into build-ubuntu using type matrix dimension - Add concurrency group to cancel in-progress runs - Add pull_request and workflow_dispatch triggers with paths-ignore - Move PACKAGE_NAME to global env - Simplify macOS coverage flags and guard steps with contains-code-coverage - Make build-windows and build-android conditional on full-matrix - Update Android API levels to 28/33/36 and use swift object matrix - Keep Xcode 15.4/macos-14 and Swift 6.1 in Android - Remove per-job env blocks and old ci-skip conditions - Keep Mint-based lint and swift-source-compat-suite jobs https://claude.ai/code/session_01V8SH8T2AKK9KzvKFAF1vaK
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Code Review: Refactor CI workflow with dynamic matrix configurationOverviewThis PR restructures the CI workflow to use a dynamic matrix based on trigger context, consolidates the Ubuntu + WASM jobs, adds concurrency control, and simplifies coverage reporting. The intent is sound and the overall direction is good, but there are several issues worth addressing before merging. Bugs / Potential Breakage1. The original workflow passed # Removed from build-ubuntu, build-windows, build-android:
# scheme: ${{ env.PACKAGE_NAME }}2. Coverage files not passed to Codecov in The # Missing id: coverage-files
- name: Process Coverage
if: steps.build.outputs.contains-code-coverage == 'true'
uses: sersoft-gmbh/swift-coverage-action@v5
with:
fail-on-empty-output: true
# Codecov then has no 'files:' input3. The original referenced Missing Functionality4. Nightly Swift builds silently dropped The original # Original:
container: ${{ matrix.swift.nightly && format('swiftlang/swift:nightly-{0}-noble', ...) || format('swift:{0}', ...) }}The new version hardcodes 5.
needs: [configure]
# and conditionally reducing the matrix, or at least documenting why macOS always runs full.6. The original had Minor Issues7. These were present in the original for all platforms. Their removal means Codecov failures won't block CI and debugging info disappears from logs. This may be intentional to reduce noise, but it also makes silent coverage failures harder to detect. 8. The 9. Android API level 34 removed, 36 added The change from 10. Changed from blocking on Improvements Worth Noting
SummaryThe architecture of the dynamic matrix approach is solid. The main risks before merging are:
🤖 Generated with Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dependabot/swift/github.com/coreoffice/xmlcoder-0.18.1 #124 +/- ##
=======================================================================================
Coverage 77.65% 77.65%
=======================================================================================
Files 74 74
Lines 1553 1553
=======================================================================================
Hits 1206 1206
Misses 347 347
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review: Refactor CI workflow with dynamic matrix configurationOverall, this is a well-structured refactor that improves CI efficiency. The dynamic matrix approach is sound and the consolidation of 🚨 Critical Issues1. Hardcoded feature branch in elif [[ "$REF" == "refs/heads/claude/update-workflow-matrix-bnyqx" ]]; then
FULL=trueThis development branch name must be removed before merging. It's dead weight in production and reveals implementation details. The surrounding logic already handles the correct cases (
|
For push events on feature branches, look up the PR's base branch via gh CLI rather than hardcoding the branch name. Full matrix runs when the PR targets main or a version tag branch. https://claude.ai/code/session_01V8SH8T2AKK9KzvKFAF1vaK
Code Review: PR #124 — Refactor CI workflow with dynamic matrix configurationOverall: The PR's goal is solid — reducing CI costs with dynamic matrix selection and better organization. The direction is good, but there are several correctness issues that should be addressed before merging. Bugs / Correctness Issues1. Dead code in The # This branch is dead — push only fires on main/version branches,
# which are caught by the first two conditions above
elif [[ "$EVENT" == "push" ]]; then
PR_BASE=$(gh pr view "${{ github.ref_name }}" ...)
...
fi2. Missing The original jobs passed # build-ubuntu (original)
- scheme: ${{ env.PACKAGE_NAME }}
# build-windows (original)
- scheme: ${{ env.PACKAGE_NAME }}3.
4. The original Coverage Concerns5. Removed explicit The original piped the output of 6. Coverage flag naming convention changed Flags changed from structured names ( Minor Issues7. - name: Process Coverage
if: steps.build.outputs.contains-code-coverage == 'true'
uses: sersoft-gmbh/swift-coverage-action@v5
with:
fail-on-empty-output: true
# No id: set, so output cannot be referencedThe 8. Android The comment explaining why API 34 was used ( 9. Codecov flags concatenation without separator flags: ${{ matrix.type && format('{0}{1}', matrix.type, matrix.osVersion) || 'spm' }}
# Produces: 'ios18.5', 'tvos18.5', 'visionos2.5' — hard to parseConsider using a separator: Positive Changes
SummaryThe three issues that should block merge:
The coverage concerns (#5, #6) are worth addressing to avoid silent regressions in coverage reporting. |
Code Review: PR #124 — Refactor CI workflow with dynamic matrix configurationOverviewThis PR introduces a smart dynamic matrix strategy for CI, reducing build costs on non-critical branches while preserving full coverage on 🚨 Critical Issues1. Hardcoded development branch in
|
Code Review: CI Workflow Refactor (#124)OverviewThis PR restructures the GitHub Actions CI workflow to use a dynamic matrix driven by a 🔴 Critical Issues1. Hardcoded PR number in
|
Remove specific pull request checks for FULL variable.
leogdion
merged commit 6af2681
into
dependabot/swift/github.com/coreoffice/xmlcoder-0.18.1
Code Review — PR #124: Refactor CI Workflow with Dynamic Matrix ConfigurationOverviewThis PR restructures the GitHub Actions workflow to use a dynamic matrix determined at runtime, consolidates the Ubuntu and WASM builds, adds concurrency control, and updates simulator device names. The overall direction is good — dynamic matrices reduce CI spend on non-critical branches. A few issues need addressing before merging. Base Branch ConcernThe PR targets Issues🔴
|
2 participants
Summary
Restructured the SyndiKit GitHub Actions workflow to use dynamic matrix configuration based on the trigger context, improving CI efficiency and maintainability.
Key Changes
Added
configurejob: Introduces a new job that determines the scope of testing based on the event type and branch. Full matrix testing runs onmain, version branches, manual triggers, and PRs targeting these branches. Other PRs use a minimal matrix.Consolidated Ubuntu builds: Merged the separate
build-ubuntuandbuild-wasmjobs into a singlebuild-ubuntujob that uses matrix exclusions to handle WASM variants only for Swift 6.2+.Simplified trigger conditions: Replaced
branches-ignorewith explicitbranchesfilter formainand version tags. Addedpaths-ignoreto skip CI for documentation and metadata changes. Addedpull_requestandworkflow_dispatchtriggers.Added concurrency control: Implemented concurrency groups to cancel in-progress runs when new commits are pushed, preventing redundant CI executions.
Streamlined coverage reporting: Simplified coverage upload logic by removing complex flag generation scripts and using simpler, more direct flag values. Removed coverage uploads from Windows and Android builds.
Updated action versions: Bumped
actions/checkoutfrom v6 to v4 across all jobs.Refined macOS test matrix: Updated device names and OS versions for iOS, tvOS, and visionOS simulators to match current Xcode releases. Fixed comment labels from "SPM Build Matrix" to "macOS Build Matrix".
Simplified Windows/Android builds: Converted Windows matrix from
includeto standard matrix format. Removed coverage reporting from these platforms. Made Windows and Android jobs conditional on full matrix configuration.Updated Android API levels: Changed test API levels from [28, 33, 34] to [28, 33, 36].
Implementation Details
The dynamic matrix approach uses GitHub Actions outputs from the
configurejob to conditionally expand or reduce the test matrix. This reduces CI costs for non-critical branches while maintaining comprehensive testing for releases and main branch changes.https://claude.ai/code/session_01V8SH8T2AKK9KzvKFAF1vaK
Perform an AI-assisted review on