Fix release task to commit version bump before gem release (#???) #195
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConsolidates the release process into a single Rake task with optional dry-run, updates documentation accordingly, and revises the release Rake task to handle version bumping, lockfile update, commit, tag, push, and gem publishing with OTP. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant R as Rake release task
participant B as Bundler
participant G as Git
participant GH as GitHub
participant RG as RubyGems
Dev->>R: rake release[VERSION,DRY_RUN]
R->>R: Bump version (--version)
R->>R: Load actual VERSION from version.rb
R->>B: bundle install (update Gemfile.lock)
alt DRY_RUN = true
R-->>Dev: Print planned actions with resolved version
else DRY_RUN = false
R->>G: commit version bump + Gemfile.lock
R->>G: create tag v<VERSION>
R->>G: push commits and tags
G->>GH: update repo and tag
R->>RG: gem build & push (OTP prompt)
RG-->>R: publish result
R-->>Dev: Post-publish changelog reminder
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.81.1)rakelib/release.rakeCould not find gem 'rspec' in locally installed gems. ... [truncated 270 characters] ... fig_loader_resolver.rb:76:in 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 |
Code ReviewThanks for working on fixing the release task! The approach is on the right track, but there are several issues that need to be addressed before merging. Critical Issues1. Commit message will be incorrect for automatic patch bumpsLocation: rakelib/release.rake:47 When gem_version is empty (automatic patch bump), the commit message will be incomplete. Issue: The actual bumped version is not captured after gem bump runs. You need to read the version file after bumping to get the actual version. Suggested fix: After the bump command, require the version file and read CypressOnRails::VERSION to get the actual version, then use that in the commit message. 2. Dry-run mode not respectedLocation: rakelib/release.rake:46-47 The git commit commands run unconditionally, even when is_dry_run is true. This defeats the purpose of dry-run mode. Suggested fix: Wrap the git commands in an unless is_dry_run block. Design Concerns3. Inconsistency with documented release processThe RELEASING.md documentation describes a two-step process where commits are made manually. This PR makes the :release task auto-commit. Questions:
What's Good
Testing Recommendations
SummaryThe fix addresses the right problem but needs refinement to handle edge cases properly. Please address the critical issues above before merging. |
The gem bump command modifies the version file but the --no-commit flag means it doesn't commit it. This causes gem release to fail because it expects a clean working directory. This commit adds git commands to stage and commit the version file after bumping but before releasing the gem.
justin808
force-pushed
the
fix-release-commit
branch
from
dbd855d to
cd17a0d
Compare
There was a problem hiding this comment.
Pull Request Overview
Fixes the release rake task so that version bumps are committed, tagged, pushed, and then released automatically, resolving prior failure due to uncommitted changes after bumping. Key improvements add explicit version capture, lockfile update, and dry-run handling.
- Switches gem bump flag from -v to --version and reads the bumped version constant.
- Adds automated commit, tag, and push steps before running gem release.
- Introduces dry-run branching (though current implementation still mutates the working tree).
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
rakelib/release.rake
Outdated
|
|
||
| unless is_dry_run | ||
| # Commit the version bump and Gemfile.lock update | ||
| sh_in_dir(gem_root, "git add -A") |
There was a problem hiding this comment.
[nitpick] git add -A may unintentionally include unrelated local changes in the release commit. Restrict the add to the specific intended files, e.g.: git add lib/cypress_on_rails/version.rb Gemfile.lock to avoid accidental inclusion of unrelated modifications.
| sh_in_dir(gem_root, "git add -A") | |
| sh_in_dir(gem_root, "git add lib/cypress_on_rails/version.rb Gemfile.lock") |
Copilot uses AI. Check for mistakes.
| puts "Carefully add your OTP for Rubygems. If you get an error, run 'gem release' again." | ||
| sh_in_dir(gem_root, "gem release") | ||
| else | ||
| puts "DRY RUN: Would have committed, tagged v#{actual_version}, pushed, and released gem" |
There was a problem hiding this comment.
In dry-run mode the task still performs gem bump and bundle install earlier (modifying version.rb and Gemfile.lock), leaving a dirty workspace despite messaging that implies only hypothetical actions. Consider wrapping the bump and bundle install in the same unless is_dry_run block (or revert changes after) so a dry run does not alter the working tree.
Copilot uses AI. Check for mistakes.
|
@claude review this pr |
|
Claude finished @justin808's task —— View job Code Review for PR #195Tasks
Review SummaryThis PR correctly fixes the immediate issue where Critical Issues1. 🔴 Dry-run mode modifies the working treeLocation: Problem: Even in dry-run mode, the task executes:
This leaves the workspace dirty despite the message saying "Would have committed..." which implies no changes were made. Impact: Users running dry-run to preview the release will end up with modified files they need to revert manually. Suggested fix: Wrap the bump and bundle commands in the same unless is_dry_run
sh_in_dir(gem_root, "gem bump --no-commit --file lib/cypress_on_rails/version.rb #{gem_version.strip.empty? ? '' : %(--version #{gem_version})}")
# Read the actual version from the file after bump
require_relative "../lib/cypress_on_rails/version"
actual_version = CypressOnRails::VERSION
# Update Gemfile.lock files
sh_in_dir(gem_root, "bundle install")
# Commit, tag, push, release...
else
puts "DRY RUN: Would bump to #{gem_version.strip.empty? ? 'next patch' : gem_version}, commit, tag, push, and release gem"
end2.
|
- Changed git add -A to explicitly add only version.rb and Gemfile.lock This prevents accidentally including unrelated files in release commits - Updated RELEASING.md to match the actual rake task implementation The docs referenced non-existent release:prepare and release:publish tasks Now correctly documents the single rake release task
![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
🧹 Nitpick comments (2)
RELEASING.md (2)
58-58: Optional: Consider using a Markdown link for the URL.The bare URL works fine but using a Markdown link would be more consistent with Markdown best practices and silence the linter warning.
- - Go to the releases page: https://github.com/shakacode/cypress-playwright-on-rails/releases + - Go to the [releases page](https://github.com/shakacode/cypress-playwright-on-rails/releases)As per static analysis hints.
132-145: Add explicit HEAD verification before reset
Include a step to confirm the release commit is at HEAD (e.g.,git log --oneline -1) before runninggit reset --hard HEAD~1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
RELEASING.md(4 hunks)rakelib/release.rake(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
RELEASING.md
58-58: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: rails_8
- GitHub Check: rails_7_2
- GitHub Check: rails_6_1
- GitHub Check: claude-review
🔇 Additional comments (8)
rakelib/release.rake (5)
43-43: LGTM! Correct use of --version flag.The command correctly uses
--version(not-v) to specify the version forgem bump. The--no-commitflag appropriately defers committing to allow bundling the version change with Gemfile.lock updates.
49-50: LGTM! Gemfile.lock update is necessary.Running
bundle installafter the version bump correctly updates Gemfile.lock to reflect the new gem version, ensuring consistency.
54-54: LGTM! Specific file staging addresses past concern.The command explicitly stages only
lib/cypress_on_rails/version.rbandGemfile.lock, avoiding the risk of accidentally committing unrelated changes that might exist in the working directory.Based on past review comments.
67-68: Dry-run implementation correctly avoids filesystem changes.The dry-run logic correctly skips all git operations and gem release, addressing the past concern about leaving a dirty workspace. Since
gem bump --no-commitandbundle installrun before the dry-run check, the workspace will be modified, but this is acceptable because:
- The initial clean check (lines 30-33) ensures a clean starting state
- Users can easily revert with
git checkout lib/cypress_on_rails/version.rb Gemfile.lock && bundle install- The dry-run message clearly indicates the version that would be released
Based on past review comments.
52-66: Fail-fast release workflow matches documentation
RELEASING.md specifies that the release task fails fast on errors and can be retried (with dry-run support) after fixing issues, so no additional atomic rollback is needed.RELEASING.md (3)
13-32: LGTM! Clear documentation of the unified release command.The documentation clearly describes the single-task release workflow with helpful examples for explicit versions, auto-patch bumps, and dry runs. This aligns well with the implementation in
rakelib/release.rake.
34-45: Documentation accurately reflects implementation.The 7-step workflow description matches the implementation in
rakelib/release.rake:
- ✓ git pull (line 42)
- ✓ version bump (line 43)
- ✓ bundle install (line 50)
- ✓ commit version + Gemfile.lock (lines 54-55)
- ✓ create tag (line 58)
- ✓ push commit and tag (line 61)
- ✓ publish gem with OTP prompt (lines 64-65)
46-67: Clear post-release workflow documentation.The post-release steps are well-documented with clear commands for updating CHANGELOG.md, creating GitHub releases, and optional announcements. This provides helpful guidance for completing the release process.
|
PR Review: Fix release task to commit version bump before gem release - This PR fixes critical issues in the release workflow. Full review with multiple points follows in next comments. |
Code Quality - STRENGTHS: Well-structured workflow (bump → bundle → commit → tag → push → release). Clean implementation. Proper dry-run support for all git operations (release.rake:52-67). Good error handling with pre-flight check for uncommitted changes (release.rake:31-33). Documentation improvements are excellent - much clearer release process with practical examples. |
CRITICAL ISSUE: Version Constant Caching (release.rake:46-47) - After gem bump modifies the version file, require_relative loads the module, but Ruby constants are cached. If this task runs multiple times in the same process, CypressOnRails::VERSION won't reflect the new value. RECOMMENDATION: Read version directly from file using regex parsing instead of require_relative to avoid caching issues. |
Potential Bug: gem bump flag (release.rake:43) - Changed from -v to --version. Please verify with gem-release documentation which is correct. The -v flag is more commonly documented. |
Error Recovery Issue (release.rake:61) - Using git push && git push --tags in single command. If first push fails, tags won't be pushed. If first succeeds but tag push fails, you're in inconsistent state. RECOMMENDATION: Split into two separate sh_in_dir calls for better error recovery and clarity. |
Security: Command Injection Risk (release.rake:55,58) - Version string is interpolated into shell commands without validation. While gem-release validates format, safer to add explicit version format check or use Shellwords.escape() for defense in depth. Consider: unless actual_version.match?(/\A\d+.\d+.\d+\z/) then raise end |
Test Coverage - No automated tests exist for the release task. Optional but consider adding tests for: version parsing logic, dry-run mode behavior, error handling for dirty working directory. Release tasks are difficult to test, so this is low priority. |
Additional Suggestions - 1) Consider adding confirmation prompt before pushing to prevent accidental releases. 2) Consider running tests before release (though CI should handle this). 3) If gem release fails after git push (line 61), commit and tags are pushed but gem isn't published - add rescue block with guidance. 4) RELEASING.md could make CI status check more prominent in checklist. |
Conclusion - This is a solid improvement! Main concerns: 1) CRITICAL: Fix version constant caching issue. 2) IMPORTANT: Verify --version flag is correct for gem-release. 3) Nice to have: Better error recovery and security hardening. The documentation improvements are excellent and make the release process much clearer. Recommendation: Approve with minor changes requested for the version reading logic. Great work aligning this with the react_on_rails pattern! |
Changed from require_relative to load with File.expand_path to ensure the bumped VERSION constant is properly picked up at runtime after gem bump modifies the file. Using load forces Ruby to re-evaluate the file even if it was previously required, which is necessary since gem bump modifies the version.rb file during the rake task execution.
|
Review of PR #195: Fix release task Strengths
Code Quality Issues1. Module Loading (rakelib/release.rake:46-47) 2. Git Operations (rakelib/release.rake:61) 3. Dry Run Mode (rakelib/release.rake:42-50) Security Concerns
Test CoverageNo automated tests for the release task. Should add tests for error conditions, dry-run mode, and version bumping. RecommendationsMust Fix:
Should Fix:
SummaryApprove with recommendations. This significantly improves the release workflow. Core logic is sound but consider the edge cases above. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rakelib/release.rake (1)
43-43: Dry-run still modifies version.rb.This line executes even when
is_dry_runis true, modifyinglib/cypress_on_rails/version.rband leaving the workspace dirty. Wrap this and the subsequentbundle install(line 50) in the sameunless is_dry_runblock so a dry run performs no file modifications.Apply this diff to respect dry-run:
- sh_in_dir(gem_root, "git pull --rebase") - sh_in_dir(gem_root, "gem bump --no-commit --file lib/cypress_on_rails/version.rb #{gem_version.strip.empty? ? '' : %(--version #{gem_version})}") - - # Read the actual version from the file after bump - load File.expand_path("../lib/cypress_on_rails/version.rb", __dir__) - actual_version = CypressOnRails::VERSION - - # Update Gemfile.lock files - sh_in_dir(gem_root, "bundle install") - - unless is_dry_run + sh_in_dir(gem_root, "git pull --rebase") + + unless is_dry_run + sh_in_dir(gem_root, "gem bump --no-commit --file lib/cypress_on_rails/version.rb #{gem_version.strip.empty? ? '' : %(--version #{gem_version})}") + + # Read the actual version from the file after bump + load File.expand_path("../lib/cypress_on_rails/version.rb", __dir__) + actual_version = CypressOnRails::VERSION + + # Update Gemfile.lock files + sh_in_dir(gem_root, "bundle install") + # Commit the version bump and Gemfile.lock updateFor the dry-run path, you'll need to determine what version would be bumped. Consider using
gem bump --pretendor parsing the current version to display the intended change.
🧹 Nitpick comments (2)
rakelib/release.rake (2)
61-61: Consider atomic push with--follow-tags.Using
git push && git push --tagsrequires two network round-trips. If the first succeeds but the second fails, you'll have pushed the commit without its tag, requiring manual recovery.Consider this more atomic alternative:
- sh_in_dir(gem_root, "git push && git push --tags") + sh_in_dir(gem_root, "git push --follow-tags")This ensures the commit and its associated tag are pushed together in a single operation.
64-65: Gem release error recovery could be clearer.If the user's OTP is incorrect or expires,
gem releasewill fail. However, re-runninggem releasealone won't work because the version has already been bumped, committed, and tagged locally. The user would need to either:
- Re-run the entire
rake releasetask (which would fail the uncommitted changes check), or- Manually run
gem releasefrom the correct directory after fixing OTP issues.Consider updating the error guidance to be more specific:
- puts "Carefully add your OTP for Rubygems. If you get an error, run 'gem release' again." + puts "Carefully add your OTP for Rubygems. If you get an error, cd to the gem root and run 'gem release' manually."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rakelib/release.rake(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: claude-review
- GitHub Check: rails_7_2
- GitHub Check: rails_6_1
- GitHub Check: rails_8
🔇 Additional comments (2)
rakelib/release.rake (2)
45-47: Version reload correctly usesload.Good - using
loadinstead ofrequire_relativeensures the bumped VERSION constant is picked up at runtime, addressing the previous review concern.
54-54: Specific file paths in git add - correctly addresses past review.Good - restricting
git addto onlylib/cypress_on_rails/version.rb Gemfile.lockprevents accidentally committing unrelated local changes.
| puts "Carefully add your OTP for Rubygems. If you get an error, run 'gem release' again." | ||
| sh_in_dir(gem_root, "gem release") | ||
| else | ||
| puts "DRY RUN: Would have committed, tagged v#{actual_version}, pushed, and released gem" |
There was a problem hiding this comment.
❓ Verification inconclusive
Dry-run message will fail after fixing the workspace modification issue.
After addressing the critical dry-run issue (moving bump/install inside the conditional), actual_version won't be defined in the else branch, causing this line to fail with an undefined variable error.
After applying the fix for line 43, you'll need to determine the version for the dry-run message. Consider one of these approaches:
- Parse current version and calculate next version:
else
# For dry-run, calculate what the version would be
require_relative "../lib/cypress_on_rails/version"
current_version = CypressOnRails::VERSION
if gem_version.strip.empty?
# Calculate patch bump
parts = current_version.split('.')
parts[-1] = (parts[-1].to_i + 1).to_s
next_version = parts.join('.')
else
next_version = gem_version
end
puts "DRY RUN: Would have committed, tagged v#{next_version}, pushed, and released gem"
end- Use
gem bump --pretendif supported:
else
output = `cd #{gem_root} && gem bump --pretend --file lib/cypress_on_rails/version.rb #{gem_version.strip.empty? ? '' : %(--version #{gem_version})}`
# Parse output to get version
puts "DRY RUN: Would have committed, tagged v<version>, pushed, and released gem"
endRun this script to check if gem bump supports --pretend:
🏁 Script executed:
#!/bin/bash
gem bump --help | grep -E '(pretend|dry.?run|simulate)' -iLength of output: 83
Handle undefined actual_version in dry-run branch
After moving the bump/install into the production path, actual_version no longer exists in the else branch and will trigger an undefined variable error. Replace the dry-run message with a manual version calculation (since gem bump --pretend isn’t supported):
else
require_relative "../lib/cypress_on_rails/version"
current_version = CypressOnRails::VERSION
gem_version_strip = gem_version.strip
next_version = if gem_version_strip.empty?
parts = current_version.split('.')
parts[-1] = (parts[-1].to_i + 1).to_s
parts.join('.')
else
gem_version_strip
end
puts "DRY RUN: Would have committed, tagged v#{next_version}, pushed, and released gem"
end🤖 Prompt for AI Agents
In rakelib/release.rake around line 67, the dry-run branch references undefined
actual_version after moving bump/install into the production path; fix by
loading the current gem version (require_relative
"../lib/cypress_on_rails/version" and read CypressOnRails::VERSION), strip
gem_version, and compute next_version: if gem_version is blank, split current
version on '.', increment the last numeric segment and rejoin, otherwise use the
stripped gem_version; then use next_version in the dry-run puts message so no
undefined variable is referenced.
Summary
Problems Fixed
1. Immediate Issue
The release script was failing with "Uncommitted changes found" because:
gem bump --no-commitmodifieslib/cypress_on_rails/version.rbgem releaseexpects a clean working directory and aborts if there are uncommitted changes2. Design Issues
Solution
Updated the release task to match the react_on_rails pattern:
gem bump --no-commitbundle installto update Gemfile.lockgem releaseto publishAll git operations respect the dry-run flag.
Testing
The release task should now complete successfully without manual intervention.
Note
The RELEASING.md documentation references
release:prepareandrelease:publishtasks that don't exist. This should be addressed in a follow-up PR.Summary by CodeRabbit
Documentation
Chores