-
-
Notifications
You must be signed in to change notification settings - Fork 635
Fix yarn
/bundle install
in CI
#1735
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe updates modify several GitHub Actions workflow YAML files to improve dependency management and caching strategies. Environment variables such as Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Matrix Job
participant Yarn
participant Bundler
GitHub Actions->>Matrix Job: Start job with matrix version
Matrix Job->>Matrix Job: Set env BUNDLE_FROZEN (dynamic)
Matrix Job->>Matrix Job: Set Yarn install command (conditional --frozen-lockfile)
Matrix Job->>Bundler: Install Ruby gems (cache keyed by Gemfile.lock)
Matrix Job->>Yarn: Install Node modules (cache keyed by yarn.lock)
Yarn-->>Matrix Job: Node modules installed
Bundler-->>Matrix Job: Ruby gems installed
Matrix Job-->>GitHub Actions: Job complete
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
4c8a153
to
8877840
Compare
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: 6
🧹 Nitpick comments (1)
.github/workflows/lint-js-and-ruby.yml (1)
45-45
: Review static-oldest
cache key suffix
Both Ruby gem cache keys are suffixed with-oldest
, but this job has nomatrix.versions
. Confirm if the suffix is intentional or should be removed/standardized.Also applies to: 71-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/examples.yml
(2 hunks).github/workflows/lint-js-and-ruby.yml
(2 hunks).github/workflows/main.yml
(3 hunks).github/workflows/package-js-tests.yml
(1 hunks).github/workflows/rspec-package-specs.yml
(2 hunks)
🔇 Additional comments (11)
.github/workflows/rspec-package-specs.yml (2)
15-16
: Dynamically configuringBUNDLE_FROZEN
environment variable
Correctly setsBUNDLE_FROZEN
to'false'
for the oldest variant and'true'
otherwise, enforcing frozen bundle installs only when intended.
43-43
: Switch cache key toGemfile.lock
UsinghashFiles('Gemfile.lock')
combined with${{ matrix.versions }}
ensures accurate cache invalidation based on the actual locked dependencies and the matrix variant..github/workflows/main.yml (4)
59-59
: Usespec/dummy/yarn.lock
for dummy-app node modules cache
Switching the cache key tohashFiles('spec/dummy/yarn.lock')
plus${{ matrix.versions }}
ensures precise invalidation based on the dummy app’s actual dependencies.
129-129
: UseGemfile.lock
for root Ruby gems cache
Changing the cache key tohashFiles('Gemfile.lock')
plus${{ matrix.versions }}
standardizes the strategy and improves cache accuracy.
134-134
: Usespec/dummy/Gemfile.lock
for dummy-app gems cache
Aligns the dummy app cache key with its lockfile to ensure proper cache invalidation.
139-139
: Cache dummy-app node modules by lockfile
UsinghashFiles('spec/dummy/yarn.lock')
with${{ matrix.versions }}
ensures the cache reflects the correct dependencies..github/workflows/examples.yml (2)
15-17
: StandardizeSKIP_YARN_COREPACK_CHECK
andBUNDLE_FROZEN
in job env
MovingSKIP_YARN_COREPACK_CHECK
insideenv
and addingBUNDLE_FROZEN
per matrix version aligns this workflow with others, enforcing frozen installs consistently.
70-70
: Switch main Ruby gems cache toGemfile.lock
UsinghashFiles('Gemfile.lock')
combined with${{ matrix.versions }}
improves cache precision based on actual dependencies..github/workflows/lint-js-and-ruby.yml (3)
11-12
: Enforce frozen bundle installs unconditionally
SettingBUNDLE_FROZEN: true
ensures all bundle installs respect the lockfile.
48-48
: Use frozen lockfile for Yarn installs
Including--frozen-lockfile
guarantees Yarn adheres strictly toyarn.lock
.
56-56
: Use frozen lockfile for dummy-app Yarn installs
Consistent use of--frozen-lockfile
for the dummy app aligns with the dependency strategy.
b2163b8
to
43f5fa6
Compare
cache: yarn | ||
cache-dependency-path: '**/yarn.lock' |
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.
See https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#caching-packages-data and https://github.com/actions/cache/blob/main/examples.md#node---npm for why this is preferred over caching node_modules
. In our case it also avoids duplication between node_modules
and spec/dummy/node_modules
.
- name: Save root ruby gems to cache | ||
uses: actions/cache@v4 | ||
with: | ||
path: vendor/bundle | ||
key: package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||
key: package-app-gem-cache-${{ hashFiles('Gemfile.lock') }}-${{ matrix.versions }} |
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.
setup-ruby
documentation says action/cache
isn't enough and action/cache
docs link to it.
But I am awaiting their reply for what to do for multiple Gemfile.lock
s in a single repo.
Summary
node_modules
andvendor/bundle
should use lockfile hash in their key, not the mainpackage.json
/Gemfile
yarn install
andbundle install
in frozen lockfile mode to be sure the lockfiles are updated.Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit
--frozen-lockfile
flag andBUNDLE_FROZEN
variable where appropriate.