diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..b6dcbac --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,35 @@ +name: Release + +on: + push: + tags: ["v*"] + +permissions: + contents: write # rake release (via rubygems/release-gem) pushes the tag; no-op here since it's already pushed + id-token: write # RubyGems.org OIDC trusted publishing + packages: write # GitHub Packages push + +jobs: + release: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Verify tag is reachable from develop + run: | + if ! git merge-base --is-ancestor "$GITHUB_SHA" origin/develop; then + echo "::error::$GITHUB_REF_NAME is not reachable from develop. Refusing to publish." + exit 1 + fi + + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + bundler-cache: true + ruby-version: "3.4" + + - name: Publish to RubyGems.org + uses: rubygems/release-gem@v1 diff --git a/.github/workflows/specs.yml b/.github/workflows/specs.yml index 1dfedc9..2f7763a 100644 --- a/.github/workflows/specs.yml +++ b/.github/workflows/specs.yml @@ -1,5 +1,6 @@ name: Run specs on: + workflow_dispatch: pull_request: branches: - develop @@ -8,18 +9,93 @@ on: - develop jobs: - build: + # This patch release changes no version constraint, so the matrix records the + # range the gem is verified against rather than narrowing what it allows - + # older Ruby and Rails remain permitted, just untested. Rails 6.1/7.0 and + # Ruby 2.7 are left out deliberately: they are long past upstream support, + # and they were the cells that needed harness pins (Rails <= 7.0 pins its + # sqlite3 adapter to `~> 1.4`, Rails < 7.1 predates concurrent-ruby 1.3.5 + # dropping a Logger constant) or a pinned runner image. The Gemfile still + # accepts those overrides for reproducing an old combination locally. + specs: + name: Ruby ${{ matrix.ruby }} / Rails ${{ matrix.rails }} runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - { ruby: '3.0', rails: '~> 7.1.0' } + - { ruby: '3.1', rails: '~> 7.1.0' } + - { ruby: '3.1', rails: '~> 7.2.0' } + - { ruby: '3.2', rails: '~> 7.1.0' } + - { ruby: '3.2', rails: '~> 7.2.0' } + - { ruby: '3.2', rails: '~> 8.0.0' } + - { ruby: '3.2', rails: '~> 8.1.0' } + - { ruby: '3.3', rails: '~> 7.1.0' } + - { ruby: '3.3', rails: '~> 7.2.0' } + - { ruby: '3.3', rails: '~> 8.0.0' } + - { ruby: '3.3', rails: '~> 8.1.0' } + - { ruby: '3.4', rails: '~> 7.2.0' } + - { ruby: '3.4', rails: '~> 8.0.0' } + - { ruby: '3.4', rails: '~> 8.1.0' } + - { ruby: '4.0', rails: '~> 7.2.0' } + - { ruby: '4.0', rails: '~> 8.0.0' } + - { ruby: '4.0', rails: '~> 8.1.0' } + env: + RAILS_VERSION: ${{ matrix.rails }} steps: - - uses: actions/checkout@v3 - - name: Set up Ruby 2.7 - uses: ruby/setup-ruby@v1 + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 with: - ruby-version: 2.7 + ruby-version: ${{ matrix.ruby }} + bundler-cache: true + - name: Run specs + run: bundle exec rspec + + # Sidekiq is optional, so it gets its own axis rather than multiplying the + # matrix above. All three states a consumer can be in are covered: absent, + # present with strict argument checking (the Sidekiq 7+ default), and present + # with it disabled. The 6.5 leg covers the Sidekiq::Worker-era fallback path. + sidekiq: + name: Sidekiq ${{ matrix.sidekiq }} (strict args ${{ matrix.strict_args || 'absent' }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - { sidekiq: '~> 6.5' } + - { sidekiq: '~> 7.0', strict_args: 'raise' } + - { sidekiq: '~> 7.0', strict_args: 'false' } + - { sidekiq: '~> 8.0', strict_args: 'raise' } + - { sidekiq: '~> 8.0', strict_args: 'false' } + - { sidekiq: 'none' } + env: + SIDEKIQ_VERSION: ${{ matrix.sidekiq }} + SIDEKIQ_STRICT_ARGS: ${{ matrix.strict_args }} + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.4' + bundler-cache: true + - name: Run specs + run: | + if [ "$SIDEKIQ_VERSION" = "none" ]; then + bundle exec rspec --exclude-pattern "sidekiq/**/*_spec.rb" + else + bundle exec rspec + fi + + sonarqube: + name: SonarQube + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.2' bundler-cache: true - name: Run specs - env: - CC_TEST_REPORTER_ID: ${{ secrets.CC_TEST_REPORTER_ID }} run: bundle exec rspec - name: SonarQube Scan diff --git a/CHANGELOG.md b/CHANGELOG.md index 3057983..1b833e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,77 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. Converted to GitHub Actions for CI +## [3.6.3] - 2026-09-09 + +Bug fixes only. No version constraint changes, no API changes and nothing +removed from the public interface, so anything that installed 3.6.2 installs +this. First step of a staged modernisation: declaring dependencies and adding +deprecations comes in 3.7.0, raising minimums in 4.0.0. + +### Fixed +- Sidekiq 7 and 8 compatibility. Strict argument checking, enabled by default + from Sidekiq 7, rejected the job arguments the workers enqueued: + `patches:run` passed the runner class object itself and `application_version` + used a symbol key, so enqueueing raised `ArgumentError: Job arguments to + Patches::Worker must be native JSON types`. The task now enqueues the runner + class name and the key is a string. The serialised payload is byte-identical + to 3.6.2's - the class already reached Redis as `"Patches::Runner"` and the + symbol key as `"application_version"` - so in-flight jobs and rolling deploys + that mix old and new workers are unaffected +- The install migration inherited from a bare `ActiveRecord::Migration`, which + Rails has refused since 5.0 with "Directly inheriting from + ActiveRecord::Migration is not supported", so + `rake patches:install:migrations && rake db:migrate` failed on every Rails + since. It now declares `ActiveRecord::Migration[5.0]`, which Rails still + supports through 8.1, so no minimum moves +- `Patches.default_path` guarded on `defined?(:Rails)`, which tests a symbol + literal and is always true; outside Rails it raised `NameError` instead of + returning nil + +### Added +- Specs for the install migration, which shipped with no coverage at all, and + for `Patches.sidekiq_job_module`, including its `Sidekiq::Worker` fallback +- A release workflow and `RELEASING.md`, both matching rdytech/superset-client: + pushing a `v`-prefixed tag publishes to RubyGems through trusted publishing, + and a tag not reachable from `develop` is refused. It also waits for the + version to appear on RubyGems, so a release that does not land fails loudly - + `3.6.1` was tagged and released on GitHub but never published, and nothing + reported it +- CI now runs 17 Ruby/Rails combinations (Ruby 3.0-4.0 against Rails 7.1-8.1) + in place of a single Ruby 2.7 cell, plus a leg for each Sidekiq state: + absent, 6.5, and 7.x/8.x with strict arguments both on and off. Ruby 2.7 and + Rails 6.1/7.0 are not exercised, being well past upstream support; they are + untested rather than blocked, since no constraint changed. README.md records + the verified range and how to check an older combination locally + +### Changed +- The workers resolve their mixin through `Patches.sidekiq_job_module`, which + prefers `Sidekiq::Job` (Sidekiq 6.3+) and falls back to `Sidekiq::Worker`, so + no Sidekiq version loses support +- Development dependencies pruned to those actually used: `capybara`, + `factory_girl`, `timecop`, `generator_spec`, `byebug` and `database_cleaner` + were referenced by no spec, and `rspec-rails` was never required, so plain + `rspec` replaces it. `rails`, `sqlite3`, `sidekiq` and `concurrent-ruby` moved + to the Gemfile so CI can vary them; the stale `sidekiq ~> 3.4.1` pin is what + hid the strict argument incompatibility, and it also held `json` at `~> 1.0` +- Sidekiq-dependent specs moved under `spec/sidekiq/` so the suite runs with the + gem absent + +### Known issues +- The install migration now declares `ActiveRecord::Migration[5.0]`, and + `ActiveRecord::Migration.[]` does not exist before Rails 5.0. A *fresh* + install on Rails 4.2 or earlier therefore needs `patches_patches` created by + hand - the table it wants is a `path` string, timestamps, and a unique index + on `path`. Existing installs are unaffected, having already copied the + migration into the host application. Those versions are far outside the range + CI verifies, and the previous file could not be loaded by any Rails from 5.0 + on, so this trades an impossible install for an unlikely one + +### Removed +- `lib/generators/patches.rb`, a dead duplicate of the patch generator that + wrote to `app/db/` from a template (`patch.erb`) that does not exist. Not + reachable through Rails' generator lookup, so not part of the public interface + ## [3.6.2] - 2022-08-10 Fixes incorrect release - tag and published gem back in sync diff --git a/Gemfile b/Gemfile index 84505c1..207680e 100644 --- a/Gemfile +++ b/Gemfile @@ -2,4 +2,38 @@ source 'https://rubygems.org' gemspec -gem "rails", "~> 7.0" +# rails, sqlite3, concurrent-ruby and sidekiq are declared here rather than in +# the gemspec so that CI can vary them across the support matrix in README.md. +# Reproduce any CI cell locally by setting the same variables: +# +# RAILS_VERSION="~> 7.0.0" SQLITE3_VERSION="~> 1.4" bundle install +# bundle exec rspec +# +# An unset *or empty* value falls back to the default, because GitHub Actions +# passes an empty string for matrix keys a given cell doesn't define. +def version_requirement(name, default = nil) + value = ENV.fetch(name, '').strip + value.empty? ? default : value +end + +# Unset, this resolves to the newest Rails the running Ruby allows. The gemspec +# floor stays at railties >= 3.2; see Compatibility in README.md for the range +# CI verifies. +gem 'rails', version_requirement('RAILS_VERSION', '>= 7.1') + +# Rails pins its sqlite3 adapter: <= 7.0 needs `~> 1.4`, 7.1 accepts either, +# 8.0+ needs `>= 2.1`. Unset, bundler picks the newest 2.x. +gem 'sqlite3', version_requirement('SQLITE3_VERSION', '>= 1.4') + +# Rails < 7.1 predates concurrent-ruby 1.3.5 removing the Logger constant that +# ActiveSupport::LoggerThreadSafeLevel expects. +concurrent_ruby = version_requirement('CONCURRENT_RUBY_VERSION') +gem 'concurrent-ruby', concurrent_ruby if concurrent_ruby + +# Sidekiq is an optional integration, not a runtime dependency. SIDEKIQ_VERSION +# =none leaves it out entirely so the `defined?(Sidekiq)` guards get exercised. +# The library itself still supports older Sidekiq through +# Patches.sidekiq_job_module; this default only picks what the suite runs +# against by default. +sidekiq = version_requirement('SIDEKIQ_VERSION') +gem 'sidekiq', *[sidekiq].compact unless sidekiq == 'none' diff --git a/README.md b/README.md index 9431325..6282ef4 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,98 @@ Please note the breaking change release around deployment. See [docs/usage.md](d TL;DR You need to manually declare the patches task to run in your deploy.rb +## Compatibility + +This release changes no version constraint: the gemspec still requires only +`railties >= 3.2` and sets no Ruby floor, so anything that installed 3.6.2 +installs 3.6.3. The table is what CI verifies, which is narrower than what the +gem permits - older Ruby and Rails are untested here, not blocked. + +| | Rails 7.1 | Rails 7.2 | Rails 8.0 | Rails 8.1 | +|---|---|---|---|---| +| **Ruby 3.0** | ✅ | | | | +| **Ruby 3.1** | ✅ | ✅ | | | +| **Ruby 3.2** | ✅ | ✅ | ✅ | ✅ | +| **Ruby 3.3** | ✅ | ✅ | ✅ | ✅ | +| **Ruby 3.4** | | ✅ | ✅ | ✅ | +| **Ruby 4.0** | | ✅ | ✅ | ✅ | + +A blank cell is a pairing Rails itself does not support. Ruby 2.7 and Rails +6.1/7.0 are deliberately not exercised - they are well past upstream support - +though the gem still installs and, as of this release, passes on them. + +Ruby 4.0 is newer than all of these Rails releases, so those cells record that +Patches is verified on it, not that Rails claims support for it. + +### Sidekiq + +Sidekiq is an **optional** integration, not a runtime dependency: Patches runs +each patch inline when Sidekiq is absent. Set `config.use_sidekiq = true` to run +them in the background - see [docs/usage.md](docs/usage.md). + +The workers resolve their mixin at load time via `Patches.sidekiq_job_module`, +preferring `Sidekiq::Job` (the name Sidekiq has used since 6.3) and falling back +to `Sidekiq::Worker`, so older Sidekiq keeps working. CI covers each state a +consumer can be in: + +| State | Covered by | +|---|---| +| Sidekiq not installed | A leg omitting the gem entirely, exercising the `defined?(Sidekiq)` guards | +| Installed, strict arguments on | Sidekiq 7.x and 8.x at their default (`:raise`) | +| Installed, strict arguments off | Sidekiq 7.x and 8.x with `Sidekiq.strict_args!(false)` | +| Older Sidekiq | A 6.5 leg, plus a spec covering the `Sidekiq::Worker` fallback | + +### Running one combination locally + +The Gemfile reads the same variables CI sets, so any cell above is reproducible. +Bundler re-evaluates the Gemfile on every invocation, so **export** them rather +than prefixing a single command — otherwise `bundle exec` silently resolves the +defaults and you test something other than what you intended: + +``` +export RAILS_VERSION="~> 8.0.0" + +bundle install +bundle exec rspec +``` + +Without Sidekiq at all, which is what a consumer that does not use it gets: + +``` +export SIDEKIQ_VERSION=none + +bundle install +bundle exec rspec --exclude-pattern "sidekiq/**/*_spec.rb" +``` + +With Sidekiq 7 and strict argument checking turned off: + +``` +export SIDEKIQ_VERSION="~> 7.0" SIDEKIQ_STRICT_ARGS=false + +bundle install +bundle exec rspec +``` + +Rails 6.1 and 7.0 need two extra pins if you do want to check them, neither +caused by Patches — Rails <= 7.0 pins its sqlite3 adapter to `~> 1.4`, and Rails +< 7.1 predates `concurrent-ruby` 1.3.5 removing a `Logger` constant it relies +on: + +``` +export RAILS_VERSION="~> 6.1.0" \ + SQLITE3_VERSION="~> 1.4" \ + CONCURRENT_RUBY_VERSION="< 1.3.5" + +bundle install +bundle exec rspec +``` + +`unset` the variables (or use a fresh shell) before running the default +combination again, and delete `test.db` when switching between them: the suite +creates `patches_patches` only when it is missing, so a database left by an +earlier run can mask ordering problems. + ## Installation Add this line to your application's Gemfile: @@ -41,7 +133,12 @@ docker-compose build docker-compose run app bundle exec rspec ``` -To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release` to create a git tag for the version, push git commits and tags, and push the `.gem` file to [rubygems.org](https://rubygems.org/gems/patches). +To install this gem onto your local machine, run `bundle exec rake install`. + +## Releasing + +Releases are published by GitHub Actions from a `vX.Y.Z` tag — see +[RELEASING.md](RELEASING.md). ## Contributing diff --git a/RELEASING.md b/RELEASING.md new file mode 100644 index 0000000..639efdf --- /dev/null +++ b/RELEASING.md @@ -0,0 +1,50 @@ +# Releasing patches + +Publishing is only ever done by GitHub Actions (`.github/workflows/release.yml`), triggered by +a `vX.Y.Z` tag whose commit is reachable from `develop`. Tags on any other branch are rejected by +the workflow before anything is built. No one needs local publishing credentials — the workflow +authenticates to RubyGems.org via OIDC trusted publishing. + +The `v` prefix is required: the workflow triggers on `v*`, and it is the tag `rake release` +creates. RubyGems.org strips it, so `v3.6.3` publishes version `3.6.3` — which is why the +versions listed on rubygems.org never carry the prefix. + +## Cutting a release + +1. Bump `MAJOR` / `MINOR` / `PATCH` in `lib/patches/version.rb`. +2. Add a new `## [X.Y.Z] - YYYY-MM-DD` entry to the top of `CHANGELOG.md` describing the changes. +3. Commit both changes and get them onto `develop` (PR + merge, as normal). +4. From an up-to-date `develop`, tag the commit and push the tag: + + ```bash + git tag vX.Y.Z + git push origin vX.Y.Z + ``` + +5. Watch the "Release" workflow run in the Actions tab. +6. Confirm the new version shows up on [rubygems.org](https://rubygems.org/gems/patches). + +Do **not** run `bundle exec rake release` from a workstation. It would push the gem under your own +credentials *and* push the tag, and the workflow's own `rake release` would then fail because the +version already exists. + +## One-time setup + +- **RubyGems.org**: an existing owner of the `patches` gem must add a Trusted Publisher for + `rdytech/patches`, workflow `release.yml` (no environment). See + [Trusted Publishing: adding a publisher](https://guides.rubygems.org/trusted-publishing/adding-a-publisher/). + Until this is done, the release will fail while configuring credentials. The gem currently lists + a single owner, handle `jr`. +- **GitHub Packages**: the workflow carries a `packages: write` permission, inherited from the + shared workflow, but no step publishes there and nothing is pushed to it. + +## Why this exists + +Six tagged versions never reached RubyGems.org — `2.0.1`, `2.1.0`, `2.2.0`, `2.3.0`, `3.0.0` and +`3.6.1` — and nothing reported the gap. The `3.6.2` entry in `CHANGELOG.md` ("Fixes incorrect +release - tag and published gem back in sync") is the visible trace of the most recent one: +`3.6.1` was tagged and released on GitHub but never published, because +`.github/workflows/publish.yml` was added the same day and has never run. + +`rubygems/release-gem` waits for the new version to appear on RubyGems.org before the job +succeeds, so a release that does not land now fails loudly instead of passing unnoticed. diff --git a/db/migrate/201506011700_create_patch.rb b/db/migrate/201506011700_create_patch.rb index b9b11fd..3a268be 100644 --- a/db/migrate/201506011700_create_patch.rb +++ b/db/migrate/201506011700_create_patch.rb @@ -1,4 +1,4 @@ -class CreatePatch < ActiveRecord::Migration +class CreatePatch < ActiveRecord::Migration[5.0] def change create_table :patches_patches do |t| t.string :path, null: false diff --git a/lib/generators/patches.rb b/lib/generators/patches.rb deleted file mode 100644 index b62ff10..0000000 --- a/lib/generators/patches.rb +++ /dev/null @@ -1,22 +0,0 @@ -module Patches - module Generators - class PatchGenerator < Rails::Generators::NamedBase - desc 'Adds an empty patch' - source_root File.expand_path('../templates', __FILE__) - - def generate_patch - template "patch.erb", "app/db/#{file_name}.rb" - end - - private - - def class_name - name.camelize - end - - def file_name - name.underscore - end - end - end -end diff --git a/lib/patches.rb b/lib/patches.rb index cd06ba4..1698365 100644 --- a/lib/patches.rb +++ b/lib/patches.rb @@ -2,7 +2,7 @@ module Patches def self.default_path - Rails.root.join('db/patches/') if defined?(:Rails) + Rails.root.join('db/patches/') if defined?(Rails) end def self.class_name(path) @@ -23,6 +23,7 @@ def self.logger=(log) require "patches/config" require "patches/tenant_run_concern" require "patches/application_version_validation" +require "patches/sidekiq_job" if defined?(Sidekiq) require "patches/tenant_worker" if defined?(Sidekiq) require "patches/engine" if defined?(Rails) require "patches/patch" diff --git a/lib/patches/sidekiq_job.rb b/lib/patches/sidekiq_job.rb new file mode 100644 index 0000000..3d6ee00 --- /dev/null +++ b/lib/patches/sidekiq_job.rb @@ -0,0 +1,11 @@ +require 'sidekiq' + +module Patches + # Sidekiq renamed Sidekiq::Worker to Sidekiq::Job in 6.3 and still ships the + # old name as a deprecated alias. Resolving it at include time keeps the + # workers working on every Sidekiq this gem has ever supported, so the change + # needs no version bump from consumers. + def self.sidekiq_job_module + defined?(::Sidekiq::Job) ? ::Sidekiq::Job : ::Sidekiq::Worker + end +end diff --git a/lib/patches/tenant_runner.rb b/lib/patches/tenant_runner.rb index 77043e8..aeaeb87 100644 --- a/lib/patches/tenant_runner.rb +++ b/lib/patches/tenant_runner.rb @@ -13,8 +13,10 @@ def perform if parallel? Patches::TenantWorker.perform_async( tenant, - path, - application_version: Patches::Config.configuration.application_version + # Sidekiq 7+ rejects anything that is not a native JSON type, and a + # path is commonly a Pathname (Patches.default_path returns one). + path && path.to_s, + 'application_version' => Patches::Config.configuration.application_version ) else run(tenant, path) diff --git a/lib/patches/tenant_worker.rb b/lib/patches/tenant_worker.rb index 87c8d4d..f213bed 100644 --- a/lib/patches/tenant_worker.rb +++ b/lib/patches/tenant_worker.rb @@ -1,13 +1,15 @@ -require 'sidekiq' +require 'active_support/core_ext/hash/keys' +require 'patches/sidekiq_job' class Patches::TenantWorker - include Sidekiq::Worker + include Patches.sidekiq_job_module include Patches::TenantRunConcern include Patches::ApplicationVersionValidation sidekiq_options Patches::Config.configuration.sidekiq_options def perform(tenant_name, path, params = {}) + params = (params || {}).stringify_keys if valid_application_version?(params['application_version']) run(tenant_name, path) else diff --git a/lib/patches/version.rb b/lib/patches/version.rb index 55af802..114311d 100644 --- a/lib/patches/version.rb +++ b/lib/patches/version.rb @@ -1,6 +1,6 @@ module Patches MAJOR = 3 MINOR = 6 - PATCH = 2 + PATCH = 3 VERSION = [MAJOR, MINOR, PATCH].compact.join(".").freeze end diff --git a/lib/patches/worker.rb b/lib/patches/worker.rb index 5c5ef5d..18a6ac1 100644 --- a/lib/patches/worker.rb +++ b/lib/patches/worker.rb @@ -1,12 +1,14 @@ -require 'sidekiq' +require 'active_support/core_ext/hash/keys' +require 'patches/sidekiq_job' class Patches::Worker - include Sidekiq::Worker + include Patches.sidekiq_job_module include Patches::ApplicationVersionValidation sidekiq_options Patches::Config.configuration.sidekiq_options def perform(runner, params = {}) + params = (params || {}).stringify_keys if valid_application_version?(params['application_version']) runner.constantize.new.perform else diff --git a/lib/tasks/patches.rake b/lib/tasks/patches.rake index fa67c80..06ca1c5 100644 --- a/lib/tasks/patches.rake +++ b/lib/tasks/patches.rake @@ -9,8 +9,8 @@ namespace :patches do if defined?(Sidekiq) && Patches::Config.configuration.use_sidekiq Patches::Worker.perform_async( - runner, - application_version: Patches::Config.configuration.application_version + runner.name, + 'application_version' => Patches::Config.configuration.application_version ) else runner.new.perform diff --git a/patches.gemspec b/patches.gemspec index ae90691..ca19044 100644 --- a/patches.gemspec +++ b/patches.gemspec @@ -19,21 +19,25 @@ Gem::Specification.new do |spec| spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ["lib"] + # Runtime constraints are deliberately unchanged here: raising a minimum can + # block an install that works today, which is a major-version change. + # README.md records the range CI actually verifies. Declaring activerecord + # (Patches::Patch subclasses ActiveRecord::Base) and bounding slack-notifier + # are queued for the next minor. spec.add_dependency "railties", ">= 3.2" spec.add_dependency "slack-notifier" spec.add_development_dependency "bundler", "> 1.8" spec.add_development_dependency "rake", "> 10.0" - spec.add_development_dependency "sqlite3", "> 1.3.5" - spec.add_development_dependency "rspec-rails", "~> 4.0.0" - spec.add_development_dependency "capybara", "~> 2.3.0" - spec.add_development_dependency "generator_spec", "~> 0.9.0" + spec.add_development_dependency "rspec", "~> 3.13" spec.add_development_dependency "simplecov", "~> 0.17", '< 0.18' # sonarscanner requires < 0.18 - spec.add_development_dependency "factory_girl", "~> 4.5.0" - spec.add_development_dependency "timecop", "~> 0.7.0" - spec.add_development_dependency "database_cleaner", "~> 1.3.0" spec.add_development_dependency "pry" - spec.add_development_dependency "sidekiq", "~> 3.4.1" spec.add_development_dependency "webmock" - spec.add_development_dependency "byebug" + + # rails, sqlite3, sidekiq and concurrent-ruby live in the Gemfile so CI can + # vary them across the range documented in README.md. capybara, factory_girl, + # timecop, generator_spec, byebug and database_cleaner were referenced by no + # spec, and rspec-rails was never required, so plain rspec replaces it. + # Sidekiq is an optional integration, not a runtime dependency, and the suite + # is expected to pass without it installed. end diff --git a/spec/migration_spec.rb b/spec/migration_spec.rb new file mode 100644 index 0000000..2c0f1a7 --- /dev/null +++ b/spec/migration_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +# The install migration ships with the gem and consumers run it via +# `rake patches:install:migrations`, but nothing exercised it: it inherited from +# a bare ActiveRecord::Migration, which Rails has refused since 5.0. +describe 'db/migrate/201506011700_create_patch.rb' do + let(:migration_path) do + File.expand_path('../db/migrate/201506011700_create_patch.rb', __dir__) + end + + it 'declares the Active Record release it was written for' do + expect(File.read(migration_path)).to match(/ActiveRecord::Migration\[\d+\.\d+\]/) + end + + it 'loads on the installed Rails version' do + load migration_path + expect(CreatePatch.superclass).to be < ActiveRecord::Migration + end + + describe 'running it' do + # Against a throwaway in-memory database, never the shared one: migrating + # that would leave Patches::Patch holding column information for a table it + # had just dropped, failing whichever spec ran next. + around do |example| + original = ActiveRecord::Base.connection_db_config + ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:') + example.run + ensure + ActiveRecord::Base.establish_connection(original) + Patches::Patch.reset_column_information + end + + it 'creates the patches_patches table with a unique path index' do + load migration_path + CreatePatch.new.tap { |migration| migration.verbose = false }.migrate(:up) + + connection = ActiveRecord::Base.connection + expect(connection.table_exists?(:patches_patches)).to be true + expect(connection.columns(:patches_patches).map(&:name)) + .to include('path', 'created_at', 'updated_at') + + path_index = connection.indexes(:patches_patches).find { |index| index.columns == ['path'] } + expect(path_index).not_to be_nil + expect(path_index.unique).to be true + end + end +end diff --git a/spec/patches_spec.rb b/spec/patches_spec.rb index 488492f..8ad6942 100644 --- a/spec/patches_spec.rb +++ b/spec/patches_spec.rb @@ -10,6 +10,13 @@ expect(Patches.default_path).to eql(Pathname.new('/tmp/db/patches/')) end + it 'has no default path outside Rails' do + # The guard used to read `defined?(:Rails)`, which tests a symbol literal + # and is therefore always true, so this raised NameError instead. + hide_const('Rails') + expect(Patches.default_path).to be_nil + end + context 'class name' do specify { expect(Patches.class_name('201203031234_class_name.rb')).to eql('ClassName') } specify { expect(Patches.class_name('201203031234_.rb')).to be_nil } diff --git a/spec/sidekiq/sidekiq_job_spec.rb b/spec/sidekiq/sidekiq_job_spec.rb new file mode 100644 index 0000000..e205b2c --- /dev/null +++ b/spec/sidekiq/sidekiq_job_spec.rb @@ -0,0 +1,13 @@ +require 'spec_helper' +require 'patches/sidekiq_job' + +describe 'Patches.sidekiq_job_module' do + it 'prefers Sidekiq::Job, the name Sidekiq has used since 6.3' do + expect(Patches.sidekiq_job_module).to eql(Sidekiq::Job) + end + + it 'falls back to Sidekiq::Worker when Sidekiq::Job is unavailable' do + hide_const('Sidekiq::Job') + expect(Patches.sidekiq_job_module).to eql(Sidekiq::Worker) + end +end diff --git a/spec/sidekiq/tasks_spec.rb b/spec/sidekiq/tasks_spec.rb new file mode 100644 index 0000000..e6a0614 --- /dev/null +++ b/spec/sidekiq/tasks_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' +require 'rake' +require 'sidekiq/testing' +require 'patches/worker' + +# The rake task is where the payload Sidekiq 7+ rejected was built. The worker +# specs pass already-correct arguments, so they stay green even if the task +# regresses; these exercise the task itself. The expectation is on the call +# rather than the recorded job, because Sidekiq JSON-normalises the payload and +# would mask a Class argument whenever strict checking is disabled. +describe 'patches:run' do + before do + Rake.application = Rake::Application.new + Rake::Task.define_task(:environment) + load File.expand_path('../../lib/tasks/patches.rake', __dir__) + + Sidekiq::Testing.fake! + Patches::Worker.jobs.clear + Patches::Config.configuration = nil + Patches::Config.configuration.use_sidekiq = true + allow(Patches::Config.configuration).to receive(:application_version).and_return('abc') + end + + after { Rake.application = Rake::Application.new } + + context 'without Apartment' do + before { hide_const('Apartment') } + + it 'enqueues the runner by name, with a string-keyed version' do + expect(Patches::Worker).to receive(:perform_async) + .with('Patches::Runner', { 'application_version' => 'abc' }) + .and_call_original + + expect { Rake::Task['patches:run'].invoke } + .to change(Patches::Worker.jobs, :size).by(1) + end + end + + context 'with Apartment and tenants' do + # Defined here rather than relying on another spec file having defined it. + before do + apartment = Module.new + stub_const('Apartment', apartment) + allow(apartment).to receive(:tenant_names).and_return(['tenant1']) + end + + it 'enqueues the tenant runner by name' do + expect(Patches::Worker).to receive(:perform_async) + .with('Patches::TenantRunner', { 'application_version' => 'abc' }) + .and_call_original + + expect { Rake::Task['patches:run'].invoke } + .to change(Patches::Worker.jobs, :size).by(1) + end + end +end diff --git a/spec/sidekiq/tenant_runner_spec.rb b/spec/sidekiq/tenant_runner_spec.rb new file mode 100644 index 0000000..27446ff --- /dev/null +++ b/spec/sidekiq/tenant_runner_spec.rb @@ -0,0 +1,66 @@ +require 'spec_helper' +require 'sidekiq/testing' + +module Apartment + module Tenant + end +end + +require 'patches/tenant_runner' +require 'patches/tenant_worker' + +describe Patches::TenantRunner do + let(:application_version) { 'd8f190c' } + + before do + Patches::Config.configuration = nil + Sidekiq::Testing.fake! + allow(Patches).to receive(:default_path).and_return('') + allow(Patches::Config.configuration).to receive(:application_version) { application_version } + end + + context 'perform' do + let(:tenant_names) { ['test'] } + + before { expect(Apartment).to receive(:tenant_names).and_return(tenant_names) } + + specify 'does not enqueue when running inline' do + expect(subject).to receive(:run).with('test', nil) + expect { subject.perform }.not_to change(Patches::TenantWorker.jobs, :size) + end + + context 'parallel' do + before { Patches::Config.configuration.sidekiq_parallel = true } + + specify do + expect(subject.tenants).to eql(['test']) + expect(Patches::TenantWorker).to receive(:perform_async).with('test', nil, { 'application_version' => application_version }).and_call_original + expect { subject.perform }.to change(Patches::TenantWorker.jobs, :size).by(1) + end + + context 'with a Pathname path' do + # Sidekiq 7+ raises on arguments that are not native JSON types, and + # Patches.default_path returns a Pathname. + subject { described_class.new(path: Pathname.new('/app/db/patches')) } + + specify do + expect(Patches::TenantWorker).to receive(:perform_async) + .with('test', '/app/db/patches', { 'application_version' => application_version }) + .and_call_original + expect { subject.perform }.to change(Patches::TenantWorker.jobs, :size).by(1) + end + end + + context 'for multiple tenants' do + let(:tenant_names) { ['test', 'test2'] } + + specify do + expect(subject.tenants).to eql(['test', 'test2']) + expect(Patches::TenantWorker).to receive(:perform_async).with('test', nil, { 'application_version' => application_version }).and_call_original + expect(Patches::TenantWorker).to receive(:perform_async).with('test2', nil, { 'application_version' => application_version }).and_call_original + expect { subject.perform }.to change(Patches::TenantWorker.jobs, :size).by(2) + end + end + end + end +end diff --git a/spec/tenant_worker_spec.rb b/spec/sidekiq/tenant_worker_spec.rb similarity index 95% rename from spec/tenant_worker_spec.rb rename to spec/sidekiq/tenant_worker_spec.rb index 8219cf8..b03565d 100644 --- a/spec/tenant_worker_spec.rb +++ b/spec/sidekiq/tenant_worker_spec.rb @@ -26,7 +26,7 @@ end it 'reschedules the job' do - expect(Patches::TenantWorker).to receive(:perform_in).with(1.minute, 'test', 'path', 'application_version' => 'd8f190c') + expect(Patches::TenantWorker).to receive(:perform_in).with(1.minute, 'test', 'path', { 'application_version' => 'd8f190c' }) subject.perform('test', 'path', 'application_version' => 'd8f190c') end end diff --git a/spec/worker_spec.rb b/spec/sidekiq/worker_spec.rb similarity index 71% rename from spec/worker_spec.rb rename to spec/sidekiq/worker_spec.rb index 349b9c8..64f8a6f 100644 --- a/spec/worker_spec.rb +++ b/spec/sidekiq/worker_spec.rb @@ -1,7 +1,22 @@ require 'spec_helper' require "patches/worker" +require 'sidekiq/testing' + describe Patches::Worker do + describe '.perform_async' do + before { Sidekiq::Testing.fake! } + after { Patches::Worker.jobs.clear } + + # Sidekiq >= 7 enables strict argument checking by default, so every + # argument must be a native JSON type. + it 'enqueues with strict-args compatible arguments' do + expect { + Patches::Worker.perform_async('Patches::Runner', 'application_version' => 'd8f190c') + }.to change(Patches::Worker.jobs, :size).by(1) + end + end + describe '#perform' do let(:runner) { instance_double(Patches::Runner, perform: true) } @@ -27,7 +42,7 @@ end it 'reschedules the job' do - expect(Patches::Worker).to receive(:perform_in).with(1.minute, 'Patches::Runner', 'application_version' => 'd8f190c') + expect(Patches::Worker).to receive(:perform_in).with(1.minute, 'Patches::Runner', { 'application_version' => 'd8f190c' }) subject.perform('Patches::Runner', 'application_version' => 'd8f190c') end end diff --git a/spec/sidekiq_optional_spec.rb b/spec/sidekiq_optional_spec.rb new file mode 100644 index 0000000..41eda89 --- /dev/null +++ b/spec/sidekiq_optional_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +# Sidekiq is an optional integration rather than a runtime dependency: the +# workers in lib/patches.rb are only loaded `if defined?(Sidekiq)`. This spec +# runs in both CI legs and asserts the contract from whichever side applies. +sidekiq_installed = begin + require 'sidekiq' + true +rescue LoadError + false +end + +describe 'optional Sidekiq integration' do + it 'loads the inline runners regardless of Sidekiq' do + expect(defined?(Patches::Runner)).to eql('constant') + expect(defined?(Patches::TenantRunner)).to eql('constant') + end + + if sidekiq_installed + it 'provides the workers when Sidekiq is installed' do + require 'patches/worker' + require 'patches/tenant_worker' + expect(Patches::Worker.ancestors).to include(Sidekiq::Job) + expect(Patches::TenantWorker.ancestors).to include(Sidekiq::Job) + end + else + it 'does not define the workers when Sidekiq is absent' do + expect(defined?(Patches::Worker)).to be_nil + expect(defined?(Patches::TenantWorker)).to be_nil + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 86b195e..f5c5ad9 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -11,13 +11,19 @@ require 'bundler/setup' require 'rails/all' -require 'database_cleaner' require 'active_model' require 'active_record' require 'patches' require 'pry' require 'webmock/rspec' +# Sidekiq 7+ enables strict argument checking by default, and some consumers +# turn it off. Job arguments must work either way, so CI runs both. +if ENV['SIDEKIQ_STRICT_ARGS'] && !ENV['SIDEKIQ_STRICT_ARGS'].strip.empty? + require 'sidekiq' + Sidekiq.strict_args!(ENV['SIDEKIQ_STRICT_ARGS'] == 'false' ? false : :raise) +end + ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'test.db') diff --git a/spec/tenant_runner_spec.rb b/spec/tenant_runner_spec.rb index c456732..d884279 100644 --- a/spec/tenant_runner_spec.rb +++ b/spec/tenant_runner_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require 'sidekiq/testing' module Apartment module Tenant @@ -7,15 +6,13 @@ module Tenant end require 'patches/tenant_runner' -require "patches/tenant_worker" +# Parallel (Sidekiq) behaviour is covered in spec/sidekiq/tenant_runner_spec.rb. describe Patches::TenantRunner do - let(:runner) { double('Runner') } let(:application_version) { 'd8f190c' } before do Patches::Config.configuration = nil - Sidekiq::Testing.fake! allow(Patches).to receive(:default_path).and_return('') allow(Patches::Config.configuration).to receive(:application_version) { application_version } end @@ -31,31 +28,10 @@ module Tenant before { expect(Apartment).to receive(:tenant_names).and_return(tenant_names) } - specify do + specify 'runs each tenant inline' do expect(subject.tenants).to eql(['test']) expect(subject).to receive(:run).with('test', nil) - expect { subject.perform }.not_to change(Patches::TenantWorker.jobs, :size) - end - - context 'parallel' do - before { Patches::Config.configuration.sidekiq_parallel = true } - - specify do - expect(subject.tenants).to eql(['test']) - expect(Patches::TenantWorker).to receive(:perform_async).with('test', nil, application_version: application_version).and_call_original - expect { subject.perform }.to change(Patches::TenantWorker.jobs, :size).by(1) - end - - context 'for multiple tenants' do - let(:tenant_names) { ['test', 'test2'] } - - specify do - expect(subject.tenants).to eql(['test', 'test2']) - expect(Patches::TenantWorker).to receive(:perform_async).with('test', nil, application_version: application_version).and_call_original - expect(Patches::TenantWorker).to receive(:perform_async).with('test2', nil, application_version: application_version).and_call_original - expect { subject.perform }.to change(Patches::TenantWorker.jobs, :size).by(2) - end - end + subject.perform end end end