-
Notifications
You must be signed in to change notification settings - Fork 180
Add composite cache actions #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@transphorm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 1 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 (1)
WalkthroughAdds four new composite GitHub Actions for caching (Yarn, Bundler, CocoaPods, Gradle), updates mobile CI workflows to use these local actions and a local mobile-setup action, and documents the shared caching guidance and formatting rules in AGENTS.md. Changes
Sequence Diagram(s)sequenceDiagram
participant WF as Workflow (mobile-*)
participant Local as Local composite action (.github/actions/cache-*)
participant Cache as actions/cache@v4
WF->>Local: uses ./.github/actions/cache-*
Local->>Cache: run cache step (with path, key, restore-keys)
Cache-->>Local: returns cache-hit / restored files
Local-->>WF: expose outputs (cache-hit) and restore files for job
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Nitpick comments (8)
AGENTS.md (1)
53-63: Good centralization; add guidance on cache-versioning and node_modules with Yarn v4
- The directive to use shared composite actions is solid. Consider explicitly instructing folks to bump a global GH_CACHE_VERSION env to invalidate all caches across workflows when needed.
- For Yarn v4, recommend caching .yarn/cache (and optionally ~/.cache/yarn), not node_modules. Caching node_modules can be large/fragile (native rebuilds, postinstall scripts) and is usually unnecessary with Yarn’s immutable installs. Clarifying this here will prevent divergent cache strategies in workflows.
.github/actions/cache-pods/action.yml (1)
6-15: Double-check tilde expansion in pathsactions/cache typically handles ~ in paths, but it’s safer to use $HOME for clarity and portability in composite actions.
Optional tweak:
- Replace ~/Library/Caches/CocoaPods with $HOME/Library/Caches/CocoaPods in the default path.
.github/actions/cache-yarn/action.yml (1)
5-11: Yarn v4 defaults: consider documenting multiple common cache pathsDefault only includes .yarn/cache. Many setups benefit from also caching ~/.cache/yarn. Optionally add a note to the action description, or expand the default to:
default: | .yarn/cache + ~/.cache/yarn.github/workflows/mobile-e2e.yml (4)
48-49: Pass an explicit cache-version for YarnTo enable controlled cache busting across repos/runs (e.g., breaking changes), pass a cache-version env or input. The composite defaults to v1.
Example:
- - name: Cache Yarn dependencies - uses: ./.github/actions/cache-yarn + - name: Cache Yarn dependencies + uses: ./.github/actions/cache-yarn + with: + cache-version: ${{ env.GH_CACHE_VERSION || 'v1' }}
67-68: Pass an explicit cache-version for GradleSame reasoning as Yarn: make cache busting intentional and repeatable.
- - name: Cache Gradle packages - uses: ./.github/actions/cache-gradle + - name: Cache Gradle packages + uses: ./.github/actions/cache-gradle + with: + cache-version: ${{ env.GH_CACHE_VERSION || 'v1' }}
175-179: Bundler cache: good inputs; consider cache-versionLock file and path are correct. Consider adding a pluggable cache-version for manual busts across changes.
- name: Cache Ruby gems uses: ./.github/actions/cache-bundler with: path: app/vendor/bundle lock-file: app/Gemfile.lock + cache-version: ${{ env.GH_CACHE_VERSION || 'v1' }}
180-186: Pods cache: good inputs; consider cache-versionPath and lock-file look right. Add cache-version for controlled busts.
- name: Cache Pods uses: ./.github/actions/cache-pods with: path: | app/ios/Pods ~/Library/Caches/CocoaPods lock-file: app/ios/Podfile.lock + cache-version: ${{ env.GH_CACHE_VERSION || 'v1' }}.github/workflows/mobile-deploy.yml (1)
557-560: Gradle cache: consider passing cache-version here as wellYou’re already versioning other caches; mirror that here for consistent busting.
- name: Cache Gradle dependencies id: gradle-cache uses: ./.github/actions/cache-gradle with: - cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_GRADLE_CACHE_VERSION }} + cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_GRADLE_CACHE_VERSION }}Note: Also ensure the composite exposes cache-hit if you want to log it.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/actions/cache-bundler/action.yml(1 hunks).github/actions/cache-gradle/action.yml(1 hunks).github/actions/cache-pods/action.yml(1 hunks).github/actions/cache-yarn/action.yml(1 hunks).github/workflows/mobile-deploy.yml(2 hunks).github/workflows/mobile-e2e.yml(4 hunks)AGENTS.md(1 hunks)
🔇 Additional comments (1)
.github/workflows/mobile-e2e.yml (1)
154-155: Yarn cache step LGTMSwitching to the shared action improves consistency and maintainability across workflows. No issues.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
.github/workflows/mobile-deploy.yml (4)
115-125: Invalid inputs for composite cache-yarn; switch to lock-file and cache-versionThe composite action ignores key and restore-keys. Use lock-file and cache-version (including Node version) to avoid cache skew and fix lints.
- name: Cache Yarn dependencies id: yarn-cache - uses: ./.github/actions/cache-yarn - with: - path: | - .yarn/cache - node_modules - ${{ env.APP_PATH }}/node_modules - key: ${{ runner.os }}-node${{ env.NODE_VERSION }}-yarn-${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}-${{ hashFiles('yarn.lock') }} - restore-keys: | - ${{ runner.os }}-node${{ env.NODE_VERSION }}-yarn-${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}- - ${{ runner.os }}-node${{ env.NODE_VERSION }}-yarn-${{ env.GH_CACHE_VERSION }}- + uses: ./.github/actions/cache-yarn + with: + path: | + .yarn/cache + node_modules + ${{ env.APP_PATH }}/node_modules + lock-file: yarn.lock + cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}-node${{ env.NODE_VERSION }}
128-135: Invalid inputs for composite cache-bundler; switch to lock-file and cache-versionkey/restore-keys aren’t valid inputs for the composite. Include Ruby version in cache-version.
- name: Cache Ruby gems id: gems-cache - uses: ./.github/actions/cache-bundler - with: - path: ${{ env.APP_PATH }}/vendor/bundle - key: ${{ runner.os }}-ruby${{ env.RUBY_VERSION }}-gems-${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}-${{ hashFiles('app/Gemfile.lock') }} - restore-keys: | - ${{ runner.os }}-ruby${{ env.RUBY_VERSION }}-gems-${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}- - ${{ runner.os }}-ruby${{ env.RUBY_VERSION }}-gems-${{ env.GH_CACHE_VERSION }}- + uses: ./.github/actions/cache-bundler + with: + path: ${{ env.APP_PATH }}/vendor/bundle + lock-file: app/Gemfile.lock + cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}-ruby${{ env.RUBY_VERSION }}
544-554: Android: fix cache-yarn inputs (same as iOS)Remove key/restore-keys and provide lock-file and cache-version to the composite.
- name: Cache Yarn dependencies id: yarn-cache - uses: ./.github/actions/cache-yarn - with: - path: | - .yarn/cache - node_modules - ${{ env.APP_PATH }}/node_modules - key: ${{ runner.os }}-node${{ env.NODE_VERSION }}-yarn-${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}-${{ hashFiles('yarn.lock') }} - restore-keys: | - ${{ runner.os }}-node${{ env.NODE_VERSION }}-yarn-${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}- - ${{ runner.os }}-node${{ env.NODE_VERSION }}-yarn-${{ env.GH_CACHE_VERSION }}- + uses: ./.github/actions/cache-yarn + with: + path: | + .yarn/cache + node_modules + ${{ env.APP_PATH }}/node_modules + lock-file: yarn.lock + cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}-node${{ env.NODE_VERSION }}
557-564: Android: fix cache-bundler inputsAs above, use lock-file and cache-version; remove key/restore-keys.
- name: Cache Ruby gems id: gems-cache - uses: ./.github/actions/cache-bundler - with: - path: ${{ env.APP_PATH }}/vendor/bundle - key: ${{ runner.os }}-ruby${{ env.RUBY_VERSION }}-gems-${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}-${{ hashFiles('app/Gemfile.lock') }} - restore-keys: | - ${{ runner.os }}-ruby${{ env.RUBY_VERSION }}-gems-${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}- - ${{ runner.os }}-ruby${{ env.RUBY_VERSION }}-gems-${{ env.GH_CACHE_VERSION }}- + uses: ./.github/actions/cache-bundler + with: + path: ${{ env.APP_PATH }}/vendor/bundle + lock-file: app/Gemfile.lock + cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}-ruby${{ env.RUBY_VERSION }}
🧹 Nitpick comments (5)
.github/actions/cache-bundler/action.yml (1)
14-17: Doc: Encourage including Ruby version in cache-versionTo avoid cross-version conflicts in cached gems, recommend composing cache-version with RUBY_VERSION (e.g., v1-ruby${{ env.RUBY_VERSION }}) in workflows. No code change needed here—just usage guidance.
.github/actions/cache-yarn/action.yml (2)
9-11: Consider caching Yarn install state by defaultFor Yarn v4, adding .yarn/install-state.gz improves restore speed and determinism.
default: | .yarn/cache + .yarn/install-state.gz
11-18: Lock-file guidanceDefaulting lock-file to yarn.lock is fine for a root-managed monorepo. If you ever introduce nested locks, consider allowing a glob in workflows (e.g., lock-file: '**/yarn.lock').
AGENTS.md (1)
62-63: Add example for composing cache-version with runtime versionsTo avoid cache poisoning across Node/Ruby upgrades, show how to include versions in cache-version.
Each action accepts an optional `cache-version` input (often combined with `GH_CACHE_VERSION` and a tool-specific version). Avoid calling `actions/cache` directly so future workflows follow the same strategy. +For example: +- Yarn: `cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_YARN_CACHE_VERSION }}-node${{ env.NODE_VERSION }}` +- Bundler: `cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_GEMS_CACHE_VERSION }}-ruby${{ env.RUBY_VERSION }}` +- Pods/Gradle: `cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_PODS_CACHE_VERSION }}` / `${{ env.GH_GRADLE_CACHE_VERSION }}`.github/workflows/mobile-e2e.yml (1)
72-73: Optional: provide cache-version for Gradle in e2eTo keep invalidation consistent with other jobs, pass a composed cache-version here too.
- - name: Cache Gradle packages - uses: ./.github/actions/cache-gradle + - name: Cache Gradle packages + uses: ./.github/actions/cache-gradle + with: + cache-version: ${{ env.GH_CACHE_VERSION }}-${{ env.GH_GRADLE_CACHE_VERSION }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/actions/cache-bundler/action.yml(1 hunks).github/actions/cache-gradle/action.yml(1 hunks).github/actions/cache-pods/action.yml(1 hunks).github/actions/cache-yarn/action.yml(1 hunks).github/workflows/mobile-deploy.yml(6 hunks).github/workflows/mobile-e2e.yml(2 hunks)AGENTS.md(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/mobile-e2e.yml
188-188: input "key" is not defined in action "Cache Bundler" defined at "./.github/actions/cache-bundler". available inputs are "cache-version", "lock-file", "path"
(action)
189-189: input "restore-keys" is not defined in action "Cache Bundler" defined at "./.github/actions/cache-bundler". available inputs are "cache-version", "lock-file", "path"
(action)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-ios
🔇 Additional comments (8)
.github/actions/cache-bundler/action.yml (1)
19-22: Good: cache-hit output is correctly exposedThe composite forwards actions/cache output via steps.cache.outputs.cache-hit. Downstream workflows can log or branch on hit/miss reliably.
.github/actions/cache-yarn/action.yml (1)
20-23: Good: cache-hit is exposed from inner cache stepThe outputs mapping is correct and will let workflows log cache hits.
.github/actions/cache-gradle/action.yml (2)
17-21: Good: exposes cache-hit for Gradle cacheOutput wiring is correct; the step id matches and callers can read hit/miss.
29-33: Key composition is reasonableOS + cache-version + hash of gradle files strikes a good balance. No change needed.
.github/actions/cache-pods/action.yml (2)
21-25: Good: cache-hit output correctly forwardedWorkflows can branch/log status from composite output.
33-37: Key strategy is correct for PodsUsing Podfile.lock for the key ensures cache invalidates when dependencies change. Restore keys provide graceful fallback.
.github/workflows/mobile-deploy.yml (2)
138-145: Pods caching step looks correctUses the composite with path, lock-file, and cache-version; aligns with the new policy.
567-570: Gradle caching looks goodUsing the composite with cache-version keeps invalidation centralized.
Summary
Testing
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: HH8 config error)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: Unsupported signature algorithm: undefined)yarn workspace @selfxyz/mobile-app testyarn workspace @selfxyz/mobile-sdk-alpha testhttps://chatgpt.com/codex/tasks/task_b_6899342965d4832d857774a75648463e
Summary by CodeRabbit
Chores
Documentation