Upgrade application to unblock infrastructure#68
Open
JuanVqz wants to merge 21 commits into
Open
Conversation
Ruby 2.7.2/Rails 6.1 on Debian buster needs its apt sources pointed at archive.debian.org (buster is EOL) and Node 12 installed from the official tarball since NodeSource dropped its setup script. Debian buster's glibc is also too old for nokogiri's precompiled native gem, so bundler is configured to build it from source instead. database.yml now reads host/username/password from env vars (defaulting to the existing local behavior) so the same file works inside and outside Docker. Mounting a volume over /usr/local/bundle would shadow the gems baked into the image at build time, so docker-compose relies on the image's built-in gem set instead.
Bumping the patch surfaced two transitive incompatibilities to fix first: concurrent-ruby >= 1.3.5 breaks Rails' logger require order on Ruby 2.7, and the nokogiri ~> 1.8.2 pin is too old for the loofah/ rails-html-sanitizer versions that ship with the newer Rails patch. Test suite, rubocop, and reek all pass against 6.1.7.10.
Gemfile.next existed as an ad-hoc symlink pointing at rails/rails main (unstable). Points it at the 7.0.0 release series instead, adds the next_rails gem for NextRails.next? in application code and deprecation tracking, and bakes both dependency sets into the Docker image so `BUNDLE_GEMFILE=Gemfile.next` works without a separate install step. Getting both sides to boot and pass required unpinning a few long-stale dev/asset gems that only worked by accident on old Rails: - font-awesome-rails: >= 4.7.0.9 (old version capped railties < 7) - listen: >= 3.5 (Rails 7's evented file watcher needs ~> 3.5) - spring / spring-watcher-listen: dropped from the `next?` side -- the Rails-7-compatible spring 4.x requires Ruby >= 3.1, which is a separate hop; dev-only, not needed for CI or tests. Both dependency sets boot, migrate, serve requests, and pass the full test suite plus rubocop.
Gemfile/Gemfile.lock now target 7.0.10 (was 6.1.7.10), matching Gemfile.next/Gemfile.next.lock. The if next?/else conditional, the next_rails gem, and the CircleCI build-current/build-next jobs stay in place since the next planned hop is 7.0 -> 7.1 and this scaffolding gets reused rather than torn down and rebuilt. Both branches temporarily resolve to the same "~> 7.0.0" constraint; bump the `if next?` branch to 7.1.0 to start that hop (rubocop's Style/IdenticalConditionalBranches is disabled inline for that block since the duplication is intentional here). db/schema.rb picks up Rails 7's schema format (versioned ActiveRecord::Schema[7.0], explicit precision: nil on datetime columns) now that 7.0 is genuinely the default and not just the dual-boot side. Full test suite, rubocop, and reek pass clean.
GemfilesController#show passed template: "gemfiles/show.html.erb" (literal extension included) and layout: "pdf.html" to wicked_pdf's render call. Rails 6.1's template resolver tolerated the redundant format/handler suffix; Rails 7.0's stricter resolver does not, so both raised ActionView::MissingTemplate once 7.0 became the default. Also, wicked_pdf forwards render's :formats straight through to render_to_string. Without an explicit :formats, Rails used the current request format (:pdf, since this runs inside format.pdf do...end) to resolve the template, but the view is only authored as .html.erb. 6.1's format negotiation fell back to :html; 7.0's does not. Passing formats: [:html] explicitly restores the original behavior.
Nothing exercised the format.pdf branch before, which is exactly the code path the Rails 6.1 -> 7.0 upgrade silently broke. This would have caught the regression fixed in the previous commit.
wkhtmltopdf-binary (0.12.3.1) ships a 32-bit x86 binary. On Apple Silicon, running it under full arm64 emulation failed with "qemu-i386: Could not open '/lib/ld-linux.so.2': No such file or directory" -- i386 is only a valid multiarch pairing alongside an amd64 base, not arm64. Forcing the web service to build/run as linux/amd64 (Rosetta on Apple Silicon) and installing libc6:i386/libstdc++6:i386 lets the kernel run the 32-bit binary directly, no further emulation needed. Verified: the PDF export request spec, which was silently failing before with no signal locally, now runs the real wkhtmltopdf binary end-to-end and passes.
Runs the Rails 7.1 dual-boot side (currently 7.0.10, same as the default Gemfile) on port 3001, reusing the image already built for `web` instead of a second build -- only BUNDLE_GEMFILE differs. Not using `extends`: it merges array fields like `ports` instead of overriding them, which leaked web's 3000:3000 mapping into web_next too when tried. A plain service definition avoids that. web and web_next share the same bind-mounted /app, so they'd otherwise race on tmp/pids/server.pid and refuse to boot at the same time; gives web_next its own --pid path and has the entrypoint clean up both.
web and web_next intentionally share one database (matches how dual-boot is run in other upgrades here). Both containers run db:prepare on boot, so starting or restarting them around the same time hits Rails' migration advisory lock: the second one raised ActiveRecord::ConcurrentMigrationError and the container exited. The migration itself is a no-op once the first process finishes (both sides are on the same schema during this hop), so retrying a few times resolves it instead of failing the boot. Reproduced the race with a simultaneous `docker restart` of both containers and confirmed the retry fires and both come up healthy.
Resolves to 7.1.3.4, not the latest 7.1.6: activerecord 7.1.4+ uses def method_missing(name, ...) (leading arg + ... forwarding), which is Ruby 3.0+ only syntax and fails to even parse on our Ruby 2.7.2, despite the rails gemspec still advertising ruby_version >= 2.7.0. Confirmed by diffing activerecord/lib/active_record/attribute_methods.rb across tags: absent through v7.1.3, present from v7.1.4 on. Cap stays until Ruby is separately upgraded past 3.0. bundle_report compatibility --rails-version=7.1.6 (and re-checked against 7.1.3.4): 0 incompatible gems. Boot smoke test, full suite (11 examples), rubocop, and reek all pass clean on Gemfile.next. Detection scan found one deprecation (config.cache_classes -> config.enable_reloading) -- deferred to the commit that makes 7.1 the default, since config/environments/*.rb are shared unconditionally with the still-current Rails 7.0 side, which doesn't have enable_reloading at all.
Gemfile/Gemfile.lock now target 7.1.3.4 (was 7.0.10), matching Gemfile.next/Gemfile.next.lock. Both branches stay at the same >= 7.1.0, < 7.1.4 range for now (capped below the Ruby-3.0-only method_missing syntax in 7.1.4+); bump the if next? branch to start the 7.1 -> 7.2 hop. config.cache_classes -> config.enable_reloading (inverted boolean) in all three environments, per Rails 7.1's deprecation. Deferred from the dual-boot commit since config/environments/*.rb are shared unconditionally and Rails 7.0 doesn't have enable_reloading at all. db/schema.rb picks up Rails 7.1's schema version tag now that 7.1 is genuinely the default. Full test suite (11 examples), rubocop, and reek all pass clean.
paperclip (last real release 2018) calls URI.escape in lib/paperclip/url_generator.rb on every asset URL it generates -- URI.escape was fully removed in Ruby 3.0. Confirmed even paperclip's latest release (6.1.0, also 2018) never fixed this; the gem has had no maintenance since Active Storage replaced it. This was a hard blocker for any future Ruby 3.x upgrade. kt-paperclip is an actively maintained fork (latest release 8.0.0) that fixes this via URI::RFC2396_Parser instead, and stays drop-in compatible -- same Paperclip:: module and require path, just a different gem name (require: "paperclip" in the Gemfile line). As a side effect, kt-paperclip 8.0.0 drops the mimemagic dependency entirely, which also resolves an unrelated landmine: mimemagic 0.3.10 (our prior locked version) was yanked from RubyGems in 2020 over a license violation, so any clean bundle install without a cache hit would have failed regardless of Ruby version. Verified end-to-end on the current stack (Ruby 2.7.2, Rails 7.1.3.4): full test suite (11 examples), rubocop, and reek all pass. Manually exercised the real upload -> HTML view -> PDF export flow via HTTP (not rails runner) to exercise kt-paperclip's URL generation under real request conditions -- no errors, and the "URI.escape is obsolete" warnings that appeared in every previous test run are now gone.
Ports both spec files to Minitest, preserving every case including the PDF-export coverage added earlier: - spec/models/gemfile_spec.rb -> test/models/gemfile_test.rb (uses Bundler::Audit::Scanner.stub via minitest/mock instead of rspec-mocks; needed explicit `require "bundler/audit/scanner"` and `require "minitest/mock"` since nothing else forces those to load before the stub call, unlike RSpec where `RSpec.describe Gemfile` itself triggered the Gemfile model's autoload at file-parse time) - spec/controllers/gemfiles_controller_spec.rb -> test/controllers/gemfiles_controller_test.rb, written as ActionDispatch::IntegrationTest (real routes, real view rendering, matching RSpec's `render_views` + `type: :controller` behavior) Moved fixtures from spec/support/fixtures/ to the Rails-conventional test/fixtures/files/. Deleted test/fixtures/gemfiles.yml -- it was a stale default-scaffold fixture referencing a `file` column that has never existed (Paperclip splits it into file_file_name etc.), dead weight since the project used RSpec until now and never exercised it. Removed rspec-rails and rubocop-rspec from the Gemfile (capybara / selenium-webdriver stay -- Rails' own system tests need them regardless of test framework). Stripped RSpec-only cops and stale spec/ file references from .rubocop.yml / .rubocop_with_todo.yml / .rubocop_todo.yml. Deleted config/spring.rb, dead config left over from when the spring gem was dropped during the Rails 7.0 hop. Updated CircleCI (both build-current and build-next) to run `bin/rails test` instead of `bundle exec rspec`, README's documented test command, and .gitignore's test-fixture storage path (spec/test_files -> test/test_files, also updated in config/environments/test.rb). Also fixes a Rails 7.1 deprecation surfaced while writing the integration test: config.action_dispatch.show_exceptions = false -> :none. Verified: full suite (11 runs, 35 assertions, 0 failures, 0 errors), rubocop (49 files, no offenses), reek clean. Manually exercised the real upload -> HTML view -> PDF export flow via HTTP end to end.
GemfileTest#stub_scanner and #valid_file don't touch instance state by design (thin fixture/stub helpers), which reek flags as UtilityFunction. Excluded them the same way the codebase already excludes GemfilesController#vulnerabilities_count for the same cop. This is what was actually failing CI (build-current and build-next), not the test suite itself. Confirmed locally with an explicit path (bundle exec reek .) -- bare `bundle exec reek` with no argument reads from stdin when stdin isn't a TTY (reek's own input_was_piped? check), which is exactly what happens under non-interactive `docker compose run`, so every previous local reek check in this session silently scanned nothing instead of the codebase. CircleCI's invocation doesn't hit that path, which is why CI caught what local testing didn't.
JuanVqz
changed the title
Single matrixed job (gemfile: [Gemfile, Gemfile.next]) instead of two duplicated jobs, replacing CircleCI's build-current/build-next split. Each leg does rubocop, reek, db setup, asset precompile, and bin/rails test. Runs on GitHub-hosted ubuntu-latest (genuine x86_64, unlike our local arm64 Docker setup), but still needs the i386 multiarch libs for wkhtmltopdf-binary's bundled 32-bit binary -- ubuntu-latest doesn't enable that by default even though it's already x86_64. Both lint steps use explicit paths (`rubocop ... .`, `reek .`) rather than bare invocations. reek specifically: bare `reek` with no path falls back to reading stdin when stdin isn't a TTY, which silently no-ops instead of scanning the codebase -- this bit local testing earlier in this branch's history. Verified locally with `act` (nektos/act) before pushing, which caught two real bugs a plain YAML review wouldn't have: - `libjpeg62-turbo` is a Debian package name (carried over from our Dockerfile, which targets Debian buster); ubuntu-latest is Ubuntu 24.04 and doesn't have that package at all. Turned out to be unneeded anyway -- nothing in the app's asset pipeline or PDF templates actually processes JPEGs, and imagemagick pulls its own JPEG delegate transitively. - Installing an old Node (to match .nvmrc, for ExecJS) via actions/setup-node breaks any later JS-based `uses:` step in the same job (ruby/setup-ruby included) -- GitHub Actions runs those on the runner's own Node, and swapping it out mid-job for an ancient version breaks them. ExecJS just needs *a* working JS runtime, not specifically Node 12, so installed it via apt instead of setup-node -- a plain binary on PATH, no runner-wide Node swap. Not touched: whatever CircleCI project/webhook exists on circleci.com's side for this repo. That's dashboard configuration, not a repo file, and someone with access there should unlink it separately.
Removes default Rails-scaffold comments that no longer add anything (commented-out gem suggestions, restating what the line below already says) and normalizes remaining string literals to double quotes. The two comments explaining non-obvious constraints (concurrent-ruby pinned below 1.3.5, Rails capped below 7.1.4) are dropped here too -- both are already documented in this branch's commit history (25aea53, 613b896) if that context is needed again.
JuanVqz
changed the title
app/helpers/home_helper.rb was a completely empty module (`module HomeHelper; end`) -- never populated, never referenced from any view. No mailer is ever actually sent either (ApplicationMailer is just the default rails new scaffold, no subclass, no .deliver/mail() call anywhere), but the mailer scaffold itself is left for now since it wasn't explicitly flagged for removal. test/integration/.keep and test/mailers/.keep were placeholders for directories with no actual test files in them.
Replaces the standard/rubocop-rails combo with rubocop-rails-omakase,
the Rails-team-maintained lint config (bundled by default in fresh
Rails 7.1+ apps). Simpler than deciding how to keep standard's base
style and rubocop-rails' Rails cops in sync separately.
Getting there required a small dependency cascade:
- rubocop-rails-omakase pulls in a rubocop.yml that uses
`EnforcedShorthandSyntax: either` / newer cop options not valid on
our old locked rubocop (1.24.1) -- resolves on paper (gem version
constraints are satisfied) but breaks at runtime (the config file
itself isn't compatible with that cop implementation).
- rubocop was stuck at 1.24.1 because reek 6.0.4 pins
`parser ~> 3.0.0`; newer rubocop needs a newer parser. reek's own
latest release (6.5.0) fixes this (parser ~> 3.3.0) but itself
requires Ruby >= 3.1.0 -- same "dev-only gem needs newer Ruby than
this hop" situation as spring earlier in this branch. reek 6.1.4 is
the newest release still on Ruby >= 2.6.0, and its parser ~> 3.2.0
is enough to unblock rubocop up to 1.59.0, which the omakase config
runs cleanly on.
- Fixed rubocop-rails-omakase's default EnforcedStyle: space for
Layout/SpaceInside{Array,Hash}Literal{Brackets,Braces} back to
no_space in .rubocop_todo.yml to match this codebase's existing
convention, instead of reformatting every array/hash literal in the
repo to add spaces. Also caught and reverted two pre-existing
spaced literals (app/models/gemfile.rb,
config/initializers/paperclip.rb, config/environments/production.rb)
that were previously carved out via Exclude: entries for the
opposite reason.
- A `bundle lock` run from the host (arm64 macOS) had polluted
Gemfile.lock/Gemfile.next.lock's PLATFORMS with arm64-darwin,
replacing the generic `ruby` platform entirely and pulling in a
precompiled nokogiri-arm64-darwin gem the Linux container can't
load. Fixed by forcing force_ruby_platform on the host too (matches
the Docker image's own bundle config) and re-locking to `ruby` only.
- test/models/gemfile_test.rb needed an explicit `require "ostruct"`
-- something in the relocked dependency chain no longer pulls it in
transitively, and the file uses OpenStruct directly.
Also drops byebug (dev debugging gem): zero call sites anywhere in
app/ or lib/.
Verified: full test suite (11 runs, 35 assertions, 0 failures, 0
errors), rubocop (40 files, no offenses), reek (0 warnings) all pass
clean on both Gemfile and Gemfile.next.
Targets 3.2.x specifically (not just "3.x"): it satisfies both Rails 7.2's Ruby floor (>= 3.1.0) and Rails 8.0/8.1's (>= 3.2.0) in one hop, so this doesn't need revisiting for the next two Rails hops. One real hard blocker found and fixed: aws-sdk v2's aws-sdk-core 2.3.23 calls `Proc.new` relying on implicit block capture from the caller, a Ruby feature fully removed in 3.0 -- raises `ArgumentError: tried to create Proc object without a block` on any boot. kt-paperclip's S3 storage adapter already targets aws-sdk-s3 (the modern per-service SDK, same Aws::S3:: namespace) directly, so this was a straight gem swap with zero application code changes. Everything else installed and booted cleanly on the first try -- confirmed by installing the full gem set locally under Ruby 3.2.11 before touching Docker/CI. Re-added spring / spring-watcher-listen, dropped during the Rails 6.1 -> 7.0 hop specifically because spring 4.x needs Ruby >= 3.1 (now satisfied). Needed config/spring.rb back too -- Spring.dangerously_allow_disabling_reloading = true, since this app's test environment intentionally runs with config.enable_reloading = false and Spring's reloader otherwise refuses to preload it. Also bumped reek to its latest release (6.5.0, needs Ruby >= 3.1 -- was capped at 6.1.4 during the rubocop-rails-omakase migration for the same reason). Dockerfile rebased from ruby:2.7.2 (Debian buster, EOL) to ruby:3.2.11 (Debian 13 trixie, current). This removes three buster-specific workarounds that no longer apply: the archive.debian.org apt-source patch, the NodeSource-dropped-Node-12 tarball install (nodejs is a plain apt package on trixie), and bundle's force_ruby_platform (buster's glibc was too old for nokogiri's precompiled native gem; trixie's isn't). The wkhtmltopdf 32-bit-binary i386 multiarch libs stay -- that's an architecture issue, not a Ruby or Debian-version one. GitHub Actions' ruby-version bumped to match. Verified: full gem install, full test suite (11 runs, 35 assertions, 0 failures, 0 errors), rubocop (41 files, no offenses), and reek (0 warnings) all pass on Ruby 3.2.11 across both Gemfile and Gemfile.next. Manually exercised the real upload -> HTML view -> PDF export flow via HTTP end to end -- confirms kt-paperclip, aws-sdk-s3, and wkhtmltopdf all work together correctly under the new Ruby.
…Cable/ActionText JS Rails unconditionally adds activestorage, actioncable, and actiontext JS to config.assets.precompile even when those frameworks are unused (this app uploads via Paperclip, has no rich text, no websockets). Their ES6 syntax (const, class, spread) fails Uglifier's ES5-only parser, crashing `rake assets:precompile` in production with Uglifier::Error. Also drop app/assets/javascripts/cable.js: a dead default-generated file that required action_cable directly, pulling actioncable.js into the compile graph via application.js's require_tree regardless of the precompile_assets flag. ActiveStorage/ActionCable have official precompile_assets opt-out flags. ActionText doesn't, so its entries are subtracted in config.after_initialize (its own engine initializer runs after config/initializers and would otherwise re-add them). Also add a placeholder config/storage.yml: ActiveStorage's lazy on_load(:active_storage_blob) hook raises if it's missing, even though this app doesn't use ActiveStorage for uploads.
load_defaults 5.1 implies cache_format_version 6.1. Rails 7.1 deprecates support for that format (removed in 7.2). Bump just this setting ahead of the full load_defaults alignment to silence the warning now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR:
Description:
Adds a Docker/docker-compose dev setup and prepares the Rails 6.1 → 7.0 upgrade using dual-boot (
next_rails). This PR does not flip the app to Rails 7.0 yet — it lands the groundwork so the version bump itself can be a small, low-risk follow-up.Dockerfile,docker-compose.yml,docker/entrypoint.sh). Debian buster (base Ruby image) is EOL, so apt is pointed atarchive.debian.organd Node 12 is installed from the official tarball since NodeSource dropped its setup script. Nokogiri is built from source since buster's glibc is too old for its precompiled native gem.database.yml.samplenow reads host/username/password from env vars, defaulting to the existing local behavior, so the same file works inside and outside Docker.concurrent-rubypinned< 1.3.5(breaks Rails' logger require order on Ruby 2.7; Rails 7.0.10 already patched this, 6.1.7.10 hasn't) andnokogiriunpinned from a years-stale~> 1.8.2to>= 1.13.0(too old for theloofah/rails-html-sanitizerthat ship with the newer Rails patch).Gemfile.nextpreviously pointed atrails/railsmain(unstable, ad hoc). It now targets the7.0.0release series (resolves to 7.0.10, latest in that EOL series) via thenext_railsgem. Both dependency sets are baked into the Docker image soBUNDLE_GEMFILE=Gemfile.nextworks without a separate install step.font-awesome-rails(old version cappedrailties < 7),listen(Rails 7's evented file watcher needs~> 3.5).spring/spring-watcher-listenfrom thenext?side — the Rails-7-compatiblespring4.x requires Ruby >= 3.1, a separate future hop; dev-only, doesn't affect CI or tests.build-current/build-nextjobs from a prior attempt at this — confirmed they still work against the correctedGemfile.next, no changes needed.Breaking-change scan for the 6.1 → 7.0 hop found nothing blocking. Two non-blocking "fix when ready" items for a future PR: Turbolinks → Turbo migration, and a CSRF config line worth a glance in
config/environments/test.rb.config.load_defaultsstays at5.1for now — untouched by design, to be aligned incrementally in its own pass once the version bump lands.Related links (if applicable):
N/A
How has this been tested?
What manual tests have been run?
bundle exec rspec),rubocop -c .rubocop_with_todo.yml, andreekpass clean on bothGemfile(6.1.7.10) andGemfile.next(7.0.10).bundle_report compatibility --rails-version=7.0.10→ 0 incompatible gems.docker compose up --buildfrom a clean state: app boots, migrates, and servesGET /(200) on both dependency sets.