Add Rails 7.1 testing #35

argvniyx-enroute · 2024-04-17T20:02:39Z

Summary

I stumbled upon an error when trying to add Rails 7.1 support to a journaled dependent: in audit_log.rb, we call skip_audit_log on certain ActiveRecord classes which no longer inherit from ActiveRecord::Base in 7.1, so that would raise with a NoMethodError. This PR:

Bumps .ruby-version to 3.2.2 (most if not all of us are using >=3.0) as our daily driver, so it's a bit of an ergonomic change)
Includes rubocop autocorrect changes (the vast majority of these are the frozen_string_literal: true lints; mostly felt the need to do this to unblock running be rake locally)
Skips calling skip_audit_log on ActiveRecord classes when on Rails 7.1
Only sets legacy_connection_handling when on Rails 7.0

Other Information

There might be some other things worth considering (not ignoring lockfiles(?), actually using the new connection handling (even if it is only a dummy app)) but I didn't want to drag out a PR that's already pretty big.

Thanks for contributing to Journaled!

/domain @smudge @Betterment/journaled-owners
/platform @smudge

These were the offenses: 68 Style/FrozenStringLiteralComment [Unsafe Correctable] 4 Performance/StringIdentifierArgument [Safe Correctable] 1 Style/HashEachMethods [Unsafe Correctable] Still sticking to `TargetRubyVersion` being 2.6 to avoid possibly autocorrecting in a backwards incompatible way, though

this workflow didn't run, probably because it's a syntax error to have two gemfile entries for a single ruby key (I wondered if I could have a single gemfile array keyval, but I'm trying to do the obvious thing first)

InternalMetadata and friends no longer inherit from ActiveRecord::Base in 7.1, so calling `skip_audit_log` on them fails. This is a hacky workaround to avoid calling that when using Rails 7.1

this setting was removed in 7.1, so specs will fail (the dummy app's config sets it)

Rails.version returns the whole version string, we just need to see that we're on 7.0

argvniyx-enroute · 2024-04-17T22:59:36Z

spec/dummy/config/application.rb

@@ -12,6 +14,6 @@ module Dummy
  class Application < Rails::Application
    config.autoloader = Rails::VERSION::MAJOR >= 7 ? :zeitwerk : :classic
    config.active_record.sqlite3.represent_boolean_as_integer = true if Rails::VERSION::MAJOR < 6
-    config.active_record.legacy_connection_handling = false if Rails::VERSION::MAJOR >= 7
+    config.active_record.legacy_connection_handling = false if Rails::VERSION::MAJOR == 7 && Rails::VERSION::MINOR == 0


This setting was removed in 7.1.

argvniyx-enroute · 2024-04-17T23:03:51Z

journaled.gemspec

@@ -34,7 +36,7 @@ Gem::Specification.new do |s|
  s.add_development_dependency "rspec-rails"
  s.add_development_dependency "spring"
  s.add_development_dependency "spring-commands-rspec"
-  s.add_development_dependency "sqlite3"
+  s.add_development_dependency "sqlite3", '~> 1.4'


I was facing some CI troubles when this was unpinned, e.g. https://github.com/Betterment/journaled/actions/runs/8727969396/job/23946685922. I'm guessing this hadn't surfaced before because the newer versions didn't exist(?)

argvniyx-enroute · 2024-04-17T23:05:03Z

lib/journaled/version.rb

 module Journaled
-  VERSION = "5.3.1".freeze
+  VERSION = "5.3.2"


I'm only bumping patch because our actual dependencies were already sitting on 7.1 versions and I'm not dropping support for anything. We simply didn't actually test them in CI.

argvniyx-enroute · 2024-04-17T23:15:03Z

lib/journaled/audit_log.rb

+
+        # These classes do not inherit from ActiveRecord::Base in 7.1
+        # so calling `skip_audit_log` on them will raise.
+        %w(ActiveRecord::InternalMetadata ActiveRecord::SchemaMigration).include?(name)


I am assuming the original reason for skipping these was that we didn't want them to have the callbacks defined in this module, but that they would have them because we include the module in the on_load hook for AR (i.e. in 7.1, since these classes don't inherit from Base, then they don't get the methods after all)

Would it make sense to exclude these from DEFAULT_EXCLUDED_CLASSES at class load time if we're on 7.1 or greater?

(And, yeah, that was the goal. If they don't inherit, then they don't need to be included.)

Alternatively, we could check .respond_to?(:skip_audit_log), but I would start with the more explicit list.

Makes total sense; I updated the const's assignment and removed this method! I felt like inlining the array for both branches of the if-expr wasn't that bad/verbose (and decided to drop it by a line to avoid having a huge indent).

smudge

TAFN -- just the one question about changing the way the constant is defined, rather than dynamically skipping the classes later.

… time from PR feedback; instead of doing the version check at runtime, do it at class load time, i.e. when the constant is defined

argvniyx-enroute · 2024-04-24T05:17:16Z

spec/lib/journaled/audit_log_spec.rb

+      if Gem::Version.new(Rails.version) < Gem::Version.new('7.1')
+        %w(
+          Delayed::Job
+          PaperTrail::Version
+          ActiveStorage::Attachment
+          ActiveStorage::Blob
+          ActiveRecord::InternalMetadata
+          ActiveRecord::SchemaMigration
+        )
+      else
+        %w(
+          Delayed::Job
+          PaperTrail::Version
+          ActiveStorage::Attachment
+          ActiveStorage::Blob
+        )
+      end


I decided to just have the same if-expr here instead of fiddling with mocking the version and whatnot.

smudge

domain LGTM && platform LGTM!

lindan-betterment · 2024-04-29T15:14:34Z

@argvniyx-enroute @smudge: Unblocked! - disabled Semgrep for this repo.

argvniyx-enroute added 6 commits April 17, 2024 12:00

bump .ruby-version

b095881

add 7.1 appraisal entry and generate gemfile

eaec3d9

add rails 7.1 matrix entries in tests.yml

29cdd29

add ruby 2.6 key to yml

3919d8e

this workflow didn't run, probably because it's a syntax error to have two gemfile entries for a single ruby key (I wondered if I could have a single gemfile array keyval, but I'm trying to do the obvious thing first)

try pinning sqlite3 (?)

259b3fc

argvniyx-enroute force-pushed the argv/master/rails-7-1-support branch from 6802d21 to 259b3fc Compare April 17, 2024 22:23

argvniyx-enroute added 3 commits April 17, 2024 16:31

only skip_audit_log on AR classes when under Rails 7.1

3935b0a

InternalMetadata and friends no longer inherit from ActiveRecord::Base in 7.1, so calling `skip_audit_log` on them fails. This is a hacky workaround to avoid calling that when using Rails 7.1

only configure legacy_connection_handling when on Rails 7.0

6672557

this setting was removed in 7.1, so specs will fail (the dummy app's config sets it)

fix rails version logic in application.rb

ca9640a

Rails.version returns the whole version string, we just need to see that we're on 7.0

argvniyx-enroute commented Apr 17, 2024

View reviewed changes

bump version.rb

50c4e72

argvniyx-enroute commented Apr 17, 2024

View reviewed changes

argvniyx-enroute marked this pull request as ready for review April 17, 2024 23:16

smudge requested changes Apr 24, 2024

View reviewed changes

argvniyx-enroute added 2 commits April 23, 2024 22:20

Don't exclude classes that don't inherit from AR in 7.1 at class load…

fb6b937

… time from PR feedback; instead of doing the version check at runtime, do it at class load time, i.e. when the constant is defined

update audit_log_spec to reflect changes in default excluded classes

efde80b

argvniyx-enroute commented Apr 24, 2024

View reviewed changes

smudge approved these changes Apr 24, 2024

View reviewed changes

smudge merged commit 19e4983 into Betterment:master Apr 29, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rails 7.1 testing #35

Add Rails 7.1 testing #35

argvniyx-enroute commented Apr 17, 2024 •

edited

Loading

argvniyx-enroute Apr 17, 2024

argvniyx-enroute Apr 17, 2024

argvniyx-enroute Apr 17, 2024 •

edited

Loading

argvniyx-enroute Apr 17, 2024

smudge Apr 24, 2024

smudge Apr 24, 2024

smudge Apr 24, 2024

argvniyx-enroute Apr 24, 2024

smudge left a comment

argvniyx-enroute Apr 24, 2024

smudge left a comment

lindan-betterment commented Apr 29, 2024

Add Rails 7.1 testing #35

Add Rails 7.1 testing #35

Conversation

argvniyx-enroute commented Apr 17, 2024 • edited Loading

Summary

Other Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

argvniyx-enroute Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge left a comment

Choose a reason for hiding this comment

lindan-betterment commented Apr 29, 2024

argvniyx-enroute commented Apr 17, 2024 •

edited

Loading

argvniyx-enroute Apr 17, 2024 •

edited

Loading