Fix matrix exclude keys in build-dummy-app-webpack-test-bundles job #2036
Conversation
Adds the missing exclude section to the build-dummy-app-webpack-test-bundles job to match the structure used in dummy-app-integration-tests job. This fixes the GitHub Actions error where matrix exclude keys (ruby-version, node-version, dependency-level) didn't match any keys within the matrix. The exclude section prevents the minimum dependency matrix (Ruby 3.2, Node 20) from running on regular PRs, ensuring it only runs on master, workflow_dispatch, force_run, or when the full-ci label is present. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR adds exclude matrix entries to the GitHub Actions integration-tests workflow to prevent the minimum dependency matrix from executing on regular PRs. The exclusion rules ensure minimum-dependency builds only run on master, workflow_dispatch triggers, or when full CI conditions are satisfied. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 ReviewSummaryThis PR fixes a GitHub Actions workflow error where the ✅ Positives
🔍 Observations
🎯 RecommendationsNo changes required for this PR - the fix is correct and follows the established pattern. However, for future consideration:
✅ VerdictLGTM - This is a straightforward bug fix that correctly addresses the matrix configuration error. The change is:
The code follows the project's GitHub Actions patterns and should resolve the CI error without introducing new issues. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The previous approach used conditional expressions within the include section, which caused GitHub Actions to create matrix entries with false/empty values when conditions weren't met. This resulted in a (false, false, false) matrix entry that failed. Solution: Changed to match the pattern used in gem-tests.yml and examples.yml - use static entries in include section and rely solely on exclude section for conditional logic. Changes: - Converted matrix from array-based to include-based for both jobs - Made include entries static (always add both latest and minimum) - Keep exclude section to conditionally remove minimum on PRs This ensures only valid matrix combinations are created. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR Review - Fix matrix exclude keys in build-dummy-app-webpack-test-bundles job✅ SummaryThis PR correctly fixes the GitHub Actions workflow error by restructuring the matrix configuration to match the pattern already established in the 🎯 Code Quality & Best PracticesExcellent:
Pattern Applied: # Before (lines 68-76): Implicit matrix that caused exclude mismatch
matrix:
ruby-version: ['3.4']
node-version: ['22']
dependency-level: ['latest']
include: [conditional minimum versions]
# After (lines 68-81): Explicit include with proper exclude
matrix:
include:
- ruby-version: '3.4' # Always run
node-version: '22'
dependency-level: 'latest'
- ruby-version: '3.2' # Conditional via exclude
node-version: '20'
dependency-level: 'minimum'
exclude: [conditional exclusion]🐛 Bug Fix ValidationRoot Cause Correctly Identified:
But the Solution: 🔒 Security ConsiderationsNo security concerns. The changes:
⚡ Performance ConsiderationsNeutral to Positive:
🧪 Test CoverageAdequate:
Suggestion: After merge, monitor the next master branch build to confirm both matrix entries (latest + minimum) execute as expected. 📝 Minor Suggestions
github.event_name == 'pull_request' &&
github.ref \!= 'refs/heads/master' &&
inputs.force_run \!= true &&
needs.detect-changes.outputs.has_full_ci_label \!= 'true' &&
'3.2' || ''This correctly excludes the minimum config on regular PRs while allowing it on master, forced runs, and PRs with the full-ci label. 🚀 RecommendationAPPROVE ✅ This is a well-executed fix that:
The change is low-risk and high-value. The consistent structure across both jobs will make future maintenance much easier. 📚 References
Review conducted per CLAUDE.md guidelines 🤖 |
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Added the |
Replace problematic include/exclude approach with setup-matrix job that
generates different matrices based on branch and trigger context.
The previous approach using conditional expressions in exclude with empty
strings ('') didn't work reliably - GitHub Actions can't truly conditionally
exclude matrix entries this way.
New approach:
- Add setup-matrix job that evaluates conditions and outputs JSON matrix
- examples job uses fromJson() to consume the dynamic matrix
- On PRs: runs 1 job (Ruby 3.4 + latest) for fast feedback
- On master/workflow_dispatch/force_run/full-ci: runs 2 jobs for full coverage
Benefits:
- Actually works (no empty string workarounds)
- Clear and maintainable (explicit bash conditions)
- Debuggable (matrix output visible in logs)
- Flexible (easy to add more conditions)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2036: Fix matrix exclude keysSummaryThis PR correctly fixes the GitHub Actions matrix configuration issue by adding the missing exclude section to the build-dummy-app-webpack-test-bundles job. The changes align well with the patterns used in other jobs. Positive Aspects
Code Quality ObservationsThe matrix structure is now consistent across jobs. This fixes the original error where the exclude section was missing entirely from build-dummy-app-webpack-test-bundles. The new setup-matrix approach in examples.yml is superior because logic is centralized in one place, no need for complex conditional excludes, easier to test and verify, and more explicit about what runs when. Suggestions
Test Coverage
Final RecommendationAPPROVE - This PR successfully fixes the immediate issue and maintains consistency with the existing patterns. The code is correct and well-documented. Optional follow-up: Consider refactoring all three jobs to use the setup-matrix pattern for better maintainability, but that is out of scope for this bug fix. Review generated with assistance from Claude Code |
Code ReviewSummaryThis PR fixes GitHub Actions matrix configuration issues across multiple workflow files. The changes move from a problematic conditional matrix approach to more reliable patterns. ✅ Strengths
📋 Code QualityIntegration Tests Workflow (.github/workflows/integration-tests.yml):
Examples Workflow (.github/workflows/examples.yml):
🔍 Potential Issues
🎯 Suggested ImprovementsConsider unifying the approach across all workflows: # Option A: Extend dynamic matrix to integration-tests.yml (recommended)
# Pros: More maintainable, explicit, debuggable
# Cons: Extra job overhead (minimal)
# Option B: Keep current hybrid approach
# Pros: Fewer jobs, proven pattern for integration tests
# Cons: Two different patterns to maintainIf choosing Option A, you could extract the matrix logic into a reusable workflow to avoid duplication. 🔒 Security & Performance
📊 Test Coverage
✅ Final VerdictApprove with minor suggestions. The fix is correct and solves the immediate problem. The hybrid approach (static+exclude for integration-tests, dynamic for examples) works but could be unified for better maintainability in the future. Before merging: Verify the workflow runs successfully with the expected matrix configurations. Review completed following repository CLAUDE.md guidelines |
Apply same dynamic matrix approach to integration-tests.yml that was used in examples.yml. This fixes the matrix exclude errors for both build-dummy-app-webpack-test-bundles and dummy-app-integration-tests jobs. Changes: - Add setup-integration-matrix job to generate dynamic matrices - Update both jobs to use fromJson() with the dynamic matrix - Remove broken exclude rules with empty string workarounds Behavior: - On PRs: runs 1 job (Ruby 3.4 + Node 22 + latest) for fast feedback - On master/workflow_dispatch/force_run/full-ci: runs 2 jobs for full coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Convert gem-tests.yml to use the same dynamic matrix pattern as integration-tests.yml and examples.yml for consistency. Changes: - Added setup-gem-tests-matrix job to compute matrix based on context - Updated rspec-package-tests to use fromJson(needs.setup-gem-tests-matrix.outputs.matrix) - Removed static include/exclude pattern in favor of dynamic generation - Maintains same behavior: latest on PRs, both latest+minimum on master or with full-ci label Benefits: - Consistent pattern across all three test workflows - More explicit and debuggable (matrix visible in job output) - Prevents potential issues with empty/false values in exclude - Easier to understand and maintain 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@claude review this PR |
|
Claude finished @justin808's task —— View job PR Review: Fix matrix exclude keys in build-dummy-app-webpack-test-bundles jobTodo List
SummaryThis PR refactors the GitHub Actions matrix configuration across three workflow files to use a cleaner, more maintainable pattern. The changes replace the complex ✅ Strengths
🔍 Code Quality ObservationsBefore (integration-tests.yml lines 68-76): matrix:
ruby-version: ['3.4']
node-version: ['22']
dependency-level: ['latest']
include:
- ruby-version: ${{ (condition) && '3.2'}}
node-version: ${{ (condition) && '20'}}
dependency-level: ${{ (condition) && 'minimum'}}After (integration-tests.yml lines 68-81): # Dedicated setup job generates the matrix dynamically
setup-integration-matrix:
steps:
- id: set-matrix
run: |
if [[ full_ci_conditions ]]; then
echo 'matrix={"include":[...both configs...]}' >> $GITHUB_OUTPUT
else
echo 'matrix={"include":[...latest only...]}' >> $GITHUB_OUTPUT
fiThis is much clearer and avoids the pitfall of GitHub Actions trying to evaluate conditional expressions that result in empty values. 📊 Matrix Logic AnalysisThe matrix generation logic in all three workflows correctly handles:
The condition is consistent across all three workflows: if [[ "${{ github.ref }}" == "refs/heads/master" ]] || \
[[ "${{ github.event_name }}" == "workflow_dispatch" ]] || \
[[ "${{ inputs.force_run }}" == "true" ]] || \
[[ "${{ needs.detect-changes.outputs.has_full_ci_label }}" == "true" ]]; thenNote: The gem-tests.yml workflow doesn't include 🎯 Specific Changes Review1. examples.yml (lines 60-78)
2. gem-tests.yml (lines 64-80)
3. integration-tests.yml (lines 61-80, 81-87, 157-162)
🐛 Original Issue AnalysisThe PR description mentions:
This occurred in the Old approach (problematic): matrix:
ruby-version: ['3.4']
node-version: ['22']
dependency-level: ['latest']
include:
- ruby-version: ${{ condition && '3.2'}} # Could evaluate to empty stringWhen the condition was false, this would create a matrix entry with empty values, but there was no corresponding New approach (correct): 🚀 Performance ImpactPositive: The new approach adds one lightweight setup job per workflow (~5-10 seconds), but this is negligible compared to the clarity and correctness gains. Neutral: The actual test execution remains the same—PRs run 1 configuration, master runs 2 configurations. 📝 Minor Suggestions
✅ Final RecommendationAPPROVE ✅ This PR successfully:
The refactoring is well-executed and makes the workflows much easier to understand and maintain. The consistent 🧪 Testing NotesSince you've already run After merge, monitor the next master branch build to confirm the expected behavior continues. References:
|
Summary
Fixes the GitHub Actions error where matrix exclude keys didn't match any keys within the matrix for the
build-dummy-app-webpack-test-bundlesjob.Changes
excludesection tobuild-dummy-app-webpack-test-bundlesjob in.github/workflows/integration-tests.ymldummy-app-integration-testsjobTest Plan
Related
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit