ci: matrix package exclude #3257
Conversation
WalkthroughThe changes in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
This reverts commit 9ad00be.
Deploying sanguine-fe with
|
| Latest commit: |
f75ee1e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://40642c24.sanguine-fe.pages.dev |
| Branch Preview URL: | https://ci-matrix-package-exclude-2.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3257 +/- ##
===================================================
- Coverage 93.10658% 90.43902% -2.66756%
===================================================
Files 89 54 -35
Lines 2205 1025 -1180
Branches 278 82 -196
===================================================
- Hits 2053 927 -1126
+ Misses 143 95 -48
+ Partials 9 3 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/solidity.yml (4)
124-124: LGTM! Consider a minor readability improvement.The change correctly excludes the
slitherjob when only thesolidity-devopspackage is modified, which aligns with the PR objective and the comment about Slither's irrelevance forsolidity-devops.For improved readability, consider using the
containsfunction:if: ${{ needs.changes.outputs.package_count > 0 && !contains(fromJson(needs.changes.outputs.packages), 'solidity-devops') }}This change would make the condition more explicit about checking for the presence of
solidity-devopsin the list of changed packages.
242-242: LGTM! Consider the same readability improvement.The change correctly excludes the
gas-diffjob when only thesolidity-devopspackage is modified, which is consistent with the PR objective and the comment about gas diff being irrelevant forsolidity-devops.For consistency with the previous suggestion and improved readability, consider using the
containsfunction:if: ${{ needs.changes.outputs.package_count > 0 && !contains(fromJson(needs.changes.outputs.packages), 'solidity-devops') }}This change would make the condition more explicit and maintain consistency across job definitions.
293-293: LGTM! Consider readability improvement and refactoring.The change correctly excludes the
size-checkjob when only thesolidity-devopspackage is modified, which is consistent with the PR objective and the comment about size check being irrelevant forsolidity-devops.For consistency and improved readability, consider using the
containsfunction:if: ${{ needs.changes.outputs.package_count > 0 && !contains(fromJson(needs.changes.outputs.packages), 'solidity-devops') }}Given that this condition is now identical across multiple jobs (
slither,gas-diff, andsize-check), consider refactoring to reduce duplication. You could define a reusable condition at the workflow level:env: SOLIDITY_JOBS_CONDITION: ${{ needs.changes.outputs.package_count > 0 && !contains(fromJson(needs.changes.outputs.packages), 'solidity-devops') }} jobs: # ... other jobs ... slither: if: ${{ env.SOLIDITY_JOBS_CONDITION }} # ... rest of the job definition ... gas-diff: if: ${{ env.SOLIDITY_JOBS_CONDITION }} # ... rest of the job definition ... size-check: if: ${{ env.SOLIDITY_JOBS_CONDITION }} # ... rest of the job definition ...This refactoring would improve maintainability by centralizing the condition logic.
Line range hint
1-324: Overall assessment: Changes are correct and align with PR objectives.The modifications consistently implement the exclusion of the
solidity-devopspackage from theslither,gas-diff, andsize-checkjobs. This aligns well with the PR objective of excluding the matrix package for these specific jobs.Key points:
- The changes are logically correct and consistent across all affected jobs.
- The exclusion is properly justified by comments indicating that these jobs are irrelevant for the
solidity-devopspackage.- The workflow structure remains intact, with only the job execution conditions being modified.
To further improve the workflow:
- Consider implementing the suggested readability improvements using the
containsfunction for clearer condition checks.- Evaluate the proposed refactoring to reduce duplication of the condition across multiple jobs, which would enhance maintainability.
These changes will make the workflow more robust and easier to maintain in the future.
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
solidity-devopspackage from specific analysis jobs.