Cleanup test workflow #2801
Cleanup test workflow #2801
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the GitHub Actions workflow configuration files across several YAML files. Key changes include the removal of a Docker image baking step in Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/reusable-test.yml (2)
54-54: Oh, changing the tag to 'care_local', I see what you did there.The tag name change from 'care:dev' to 'care_local' is more descriptive of its purpose, which is... nice, I suppose. Though I can't help but wonder if we should consider a more standardized naming convention like 'care:local' to maintain consistency with Docker's conventional tag format.
- --tag care_local \ + --tag care:local \
Line range hint
77-78: Interesting choice having two separate database dump steps...While I'm sure there's a perfectly good reason for having two separate 'make dump-db' commands, one might wonder if we could perhaps consolidate these into a single step with combined conditions. But who am I to judge? 😊
Consider consolidating the dumps:
- name: Dump db and upload artifact if: | inputs.event_name == 'push' || github.event_name == 'push' run: make dump-db - name: Upload db artifact if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/develop' }} uses: actions/upload-artifact@v4 with: name: care-db-dump path: care_db.dump retention-days: 30 compression-level: 0 # file is already compressedAlso applies to: 108-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/reusable-test.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / Test
- GitHub Check: Analyze (python)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2801 +/- ##
========================================
Coverage 56.15% 56.15%
========================================
Files 215 215
Lines 10214 10214
Branches 1030 1030
========================================
Hits 5736 5736
Misses 4462 4462
Partials 16 16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/linter.yml (2)
31-40: Cache configuration looks good, I suppose.The pre-commit cache setup is quite thorough. Though, if you really wanted to be precise, you could've included the Python version in the cache key. But I guess this works too.
Consider updating the cache key to include the Python version:
- key: ${{ runner.os }}-${{ runner.arch }}-pre-commit-${{ hashFiles('.pre-commit-config.yaml') }} + key: ${{ runner.os }}-${{ runner.arch }}-python-3.13-pre-commit-${{ hashFiles('.pre-commit-config.yaml') }}
42-48: That's quite a condition you've got there.The cache save logic is correct, though it's giving me a slight headache trying to parse that condition. Maybe we could make it more... readable? Just a thought.
Consider breaking down the condition using environment variables:
+ - name: Set cache save conditions + id: cache-conditions + run: | + echo "CACHE_MISS=${{ steps.cache-restore.outputs.cache-hit != 'true' }}" >> $GITHUB_ENV + echo "IS_PUSH=${{ github.event_name == 'push' }}" >> $GITHUB_ENV + echo "IS_TARGET_BRANCH=${{ github.ref == 'refs/heads/develop' || github.ref == 'refs/heads/staging' }}" >> $GITHUB_ENV - name: Save pre-commit cache id: cache-save - if: ${{ steps.cache-restore.outputs.cache-hit != 'true' && github.event_name == 'push' && (github.ref == 'refs/heads/develop' || github.ref == 'refs/heads/staging') }} + if: ${{ env.CACHE_MISS == 'true' && env.IS_PUSH == 'true' && env.IS_TARGET_BRANCH == 'true' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/linter.yml(2 hunks).github/workflows/test-merge-queue.yml(1 hunks).github/workflows/test-pull-request.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test-pull-request.yml
- .github/workflows/test-merge-queue.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test / Test
🔇 Additional comments (1)
.github/workflows/linter.yml (1)
8-11: Oh, I see we're running this on every push now too.The addition of push triggers for develop and staging branches is... reasonable. Just keep in mind this might increase our CI minutes usage, but I'm sure you've thought that through.
Proposed Changes
Associated Issue
Architecture changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit