Require explicit truthy values for SKIP and FORCE env vars#926
Require explicit truthy values for SKIP and FORCE env vars#926
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Renames the variable to better reflect its purpose now that it handles both FORCE and SKIP modes. Adds a note that FORCE takes precedence when both are set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously, any set value for ENV["SKIP"] or ENV["FORCE"] (including "false" or "0") would activate skip/force mode. Now only "true", "1", and "yes" (case-insensitive) are treated as truthy. Extracts a truthy_env? helper in template.rb, reused by binstubs.rb. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a SKIP=true installer mode and a truthy_env? helper; changes conflict-resolution to prefer FORCE when truthy, otherwise honor SKIP, and applies the new conflict_option ({ force: true } / { skip: true } / {}) across installer templates and binstub copying. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 16: Update the CHANGELOG entry that currently reads "Added `SKIP=true`
installer mode..." to link to the correct PR number by replacing the PR link and
reference from `#893` to [PR
`#926`](https://github.com/shakacode/shakapacker/pull/926) while keeping the
author reference as [justin808](https://github.com/justin808); also append a
short clause to the same entry mentioning the truthy-values behavioral change
(e.g., "also fixes handling of truthy values") so the description matches the PR
objectives title and conforms to the required `[PR
`#123`](https://github.com/.../pull/123) by
[username](https://github.com/username)` format.
🧹 Nitpick comments (1)
lib/install/template.rb (1)
30-30: Pre-existing inconsistency:USE_BABEL_PACKAGEScheck doesn't usetruthy_env?.Line 30 manually checks
== "true" || == "1"but doesn't recognize"yes". Consider using the newtruthy_env?helper for consistency, which would also add"yes"support.♻️ Suggested change
-if ENV["USE_BABEL_PACKAGES"] == "true" || ENV["USE_BABEL_PACKAGES"] == "1" +if truthy_env?("USE_BABEL_PACKAGES")
Greptile SummaryThis PR fixes a bug where
Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[User runs rails shakapacker:install] --> B{Check ENV variables}
B --> C{truthy_env? FORCE}
C -->|true/1/yes| D[conflict_option = force: true]
C -->|Other values| E{truthy_env? SKIP}
E -->|true/1/yes| F[conflict_option = skip: true]
E -->|Other values| G[conflict_option = empty]
D --> H[copy_file/directory with options]
F --> H
G --> H
H --> I{File exists?}
I -->|Yes + force| J[Overwrite file]
I -->|Yes + skip| K[Skip file]
I -->|Yes + empty| L[Prompt user]
I -->|No| M[Create file]
Last reviewed commit: 3c664b6 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| { skip: true } | ||
| else | ||
| {} | ||
| end |
There was a problem hiding this comment.
Duplicated conflict_option block across two files
Low Severity
The conflict_option computation is duplicated identically in both template.rb and binstubs.rb. Since apply shares instance state but not local variables, using @conflict_option in template.rb would make it available in binstubs.rb without duplication. As-is, any future change to conflict resolution logic (e.g., adding a new env var) requires updating both files in sync, risking divergence.
Additional Locations (1)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ### Added | ||
|
|
||
| - **Added `SKIP=true` installer mode to preserve existing files**. [PR #893](https://github.com/shakacode/shakapacker/pull/893) by [justin808](https://github.com/justin808). Running `rails shakapacker:install SKIP=true` now skips conflicting files instead of overwriting them. This is useful for CI/CD pipelines and automated setups where you want to install only missing files without touching existing configuration. |
There was a problem hiding this comment.
Wrong PR number. The SKIP=true support is being introduced in this PR (#926), not in PR #893. The link should point to #926.
| - **Added `SKIP=true` installer mode to preserve existing files**. [PR #893](https://github.com/shakacode/shakapacker/pull/893) by [justin808](https://github.com/justin808). Running `rails shakapacker:install SKIP=true` now skips conflicting files instead of overwriting them. This is useful for CI/CD pipelines and automated setups where you want to install only missing files without touching existing configuration. | |
| - **Added `SKIP=true` installer mode to preserve existing files**. [PR #926](https://github.com/shakacode/shakapacker/pull/926) by [justin808](https://github.com/justin808). Running `rails shakapacker:install SKIP=true` now skips conflicting files instead of overwriting them. This is useful for CI/CD pipelines and automated setups where you want to install only missing files without touching existing configuration. |
| # Install Shakapacker | ||
|
|
||
| force_option = ENV["FORCE"] ? { force: true } : {} | ||
| def truthy_env?(name) |
There was a problem hiding this comment.
The truthy_env? method is defined at the top level of the generator template, which in Rails generator context means it becomes an instance method on the generator object — that's the correct pattern here and it's why binstubs.rb can call it after apply.
One minor note: binstubs.rb recomputes conflict_option using truthy_env? rather than relying on the conflict_option local variable already computed in template.rb. This is intentional and correct — apply evaluates the file in the same generator instance (so instance methods like truthy_env? are shared) but with a new local variable scope (so conflict_option is not visible). A short comment explaining why the variable is duplicated would help future readers avoid "simplifying" it incorrectly.
# truthy_env? is defined in template.rb (the parent template).
# It is accessible here because apply shares the same generator instance.
# conflict_option must be recomputed because apply creates a new local variable scope.
ReviewThe core change is correct and well-scoped — using IssuesCHANGELOG references wrong PR number (inline comment filed)
Observations (non-blocking)No unit tests for Other env vars in |
Add truthy_env? helper to binstubs.rb so it works when loaded independently via the shakapacker:binstubs rake task. Previously the method was only defined in template.rb, causing NoMethodError when binstubs.rb was applied outside of the full install flow. Fixes CI failures in EngineRakeTasks specs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review SummaryOverall this is a solid improvement — fixing the bug where 1. Wrong PR number in CHANGELOGThe "Added SKIP=true installer mode" entry links to PR #893, but that feature is being introduced in this PR (#926). The reference should be updated. 2. Duplicate
|
|
|
||
| ### Added | ||
|
|
||
| - **Added `SKIP=true` installer mode to preserve existing files**. [PR #893](https://github.com/shakacode/shakapacker/pull/893) by [justin808](https://github.com/justin808). Running `rails shakapacker:install SKIP=true` now skips conflicting files instead of overwriting them. This is useful for CI/CD pipelines and automated setups where you want to install only missing files without touching existing configuration. |
|
|
||
| - **Added `SKIP=true` installer mode to preserve existing files**. [PR #893](https://github.com/shakacode/shakapacker/pull/893) by [justin808](https://github.com/justin808). Running `rails shakapacker:install SKIP=true` now skips conflicting files instead of overwriting them. This is useful for CI/CD pipelines and automated setups where you want to install only missing files without touching existing configuration. | ||
|
|
||
| ### Fixed |
There was a problem hiding this comment.
This inserts a ### Fixed section here, but there is already a ### Fixed section later in the [Unreleased] block (below ### Changed). This results in two ### Fixed headings. The new entry should be merged into the existing ### Fixed section instead of creating a duplicate heading.
| @@ -1,6 +1,16 @@ | |||
| force_option = ENV["FORCE"] ? { force: true } : {} | |||
| def truthy_env?(name) | |||
There was a problem hiding this comment.
The PR description says this method is "reused via apply" from template.rb, but it's actually re-defined here independently. When binstubs.rb is evaluated via apply inside a running template.rb session, Ruby will emit a method-redefinition warning because truthy_env? was already defined.
For standalone execution of binstubs.rb the definition is necessary, but the two files are currently managing this independently. Consider either:
- Noting in a comment that the definition is intentionally duplicated for standalone-execution support, or
- Extracting to a shared helper file that both
require/apply
# Defined here for standalone execution; template.rb defines the same method
# when this file is run via `apply` in the full install flow.
def truthy_env?(name)
%w[true 1 yes].include?(ENV[name].to_s.downcase)
end
| # Install Shakapacker | ||
|
|
||
| force_option = ENV["FORCE"] ? { force: true } : {} | ||
| def truthy_env?(name) |
There was a problem hiding this comment.
No tests cover the falsy-value fix. The generator spec always invokes FORCE=true, which is truthy under both the old and new logic, so it doesn't exercise the actual bug being fixed. Consider adding a unit-level test that confirms FORCE=false, FORCE=0, and SKIP=false all produce an empty conflict_option.
1 participant
Summary
ENV["SKIP"]orENV["FORCE"](including"false"or"0") would activate skip/force mode in the installer"true","1", and"yes"(case-insensitive) are treated as truthytruthy_env?helper intemplate.rb, reused bybinstubs.rbviaapplyCloses #889
Test plan
SKIP=true rails shakapacker:install— should skip conflicting filesSKIP=false rails shakapacker:install— should NOT skip (prompts for conflicts)SKIP=0 rails shakapacker:install— should NOT skipFORCE=1 rails shakapacker:install— should force overwriteFORCE=no rails shakapacker:install— should NOT force🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Note
Low Risk
Low risk: installer conflict-handling now only enables
FORCE/SKIPfor explicit truthy values, which may change behavior for scripts relying on any non-empty value to trigger these modes.Overview
Installer conflict handling now supports
SKIP=true(skip conflicting files) and makesFORCE/SKIPopt-in via a newtruthy_env?check (onlytrue/1/yes, case-insensitive), withFORCEtaking precedence.Docs were updated (README + CHANGELOG) to describe the new
SKIPmode and the stricter env var parsing behavior.Written by Cursor Bugbot for commit 1d3c1ba. Configure here.