-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rails 7.1 support #177
Rails 7.1 support #177
Conversation
@@ -3,7 +3,7 @@ name: build | |||
on: | |||
push: | |||
branches: | |||
- '**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't build for every push, just on master
so that every PR update doesn't trigger two builds
@@ -23,6 +23,7 @@ jobs: | |||
- gemfiles/Gemfile.rails-6.0 | |||
- gemfiles/Gemfile.rails-6.1 | |||
- gemfiles/Gemfile.rails-7.0 | |||
- gemfiles/Gemfile.rails-7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add 7.1 Gemfile
@@ -36,21 +37,23 @@ jobs: | |||
- gemfile: gemfiles/Gemfile.rails-5.2 | |||
ruby-version: 2.7.7 | |||
- gemfile: gemfiles/Gemfile.rails-6.0 | |||
ruby-version: 3.2.1 | |||
ruby-version: 2.7.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 2.7.7 is more practical for old versions.
- gemfile: gemfiles/Gemfile.rails-7.0 | ||
ruby-version: 3.2.1 | ||
ruby-version: 3.1.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails 7.0 has several bugs with 3.2.x so this seems appropriate.
ruby-version: 3.2.1 | ||
ruby-version: 3.1.6 | ||
- gemfile: gemfiles/Gemfile.rails-7.1 | ||
ruby-version: 3.2.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.2 was more recent at 7.1 launch
@@ -100,7 +105,7 @@ jobs: | |||
options: --health-cmd mongosh --health-interval 10s --health-timeout 5s --health-retries 5 | |||
|
|||
steps: | |||
- uses: actions/checkout@v2 | |||
- uses: actions/checkout@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid deprecation warnings.
# gem 'devise_token_auth' | ||
# https://github.com/lynndylanhurley/devise_token_auth/pull/1517 | ||
gem 'devise_token_auth', git: 'https://github.com/lynndylanhurley/devise_token_auth.git' | ||
gem 'devise_token_auth' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devise_token 1.2.3 supports 7.1
s.add_dependency 'i18n', '>= 0.5.0' | ||
s.add_dependency 'jquery-rails', '>= 3.1.1' | ||
s.add_dependency 'swagger-blocks', '>= 3.0.0' | ||
|
||
s.add_development_dependency 'puma', '>= 3.12.0' | ||
s.add_development_dependency 'sqlite3', '>= 1.3.13' | ||
s.add_development_dependency 'sqlite3', '>= 1.3.13', '< 2.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails doesn't support sqlite3 2.0+
s.add_development_dependency 'mysql2', '>= 0.5.2' | ||
s.add_development_dependency 'pg', '>= 1.0.0' | ||
s.add_development_dependency 'mongoid', '>= 4.0.0' | ||
s.add_development_dependency 'mongoid', '>= 4.0.0', '< 9.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9.0 is a very recent release and breaks things
@@ -19,6 +19,7 @@ group :test do | |||
gem 'committee-rails', '< 0.6' | |||
# gem 'coveralls', require: false | |||
gem 'coveralls_reborn', require: false | |||
gem 'mongoid', '>= 4.0.0', '< 8.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need older for 5.1 - not sure why 5.0 is ok.
@@ -77,7 +77,7 @@ | |||
test_instance.target = create(:admin) | |||
test_instance.key = "notification.#{simple_text_key}" | |||
expect(test_instance.text) | |||
.to eq("translation missing: en.notification.admin.#{simple_text_key}.text") | |||
.to eq("Translation missing: en.notification.admin.#{simple_text_key}.text") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -46,6 +46,7 @@ def clean_database | |||
end | |||
|
|||
RSpec.configure do |config| | |||
config.expect_with :minitest, :rspec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bit tricky - perform_enqueued_jobs
in the more recent rails calls minitest's assert
but its not autoloaded in the mongoid/railtie
path https://github.com/codalio/activity_notification/blob/f0fe033de7382c6808234c65c226db35c9814bc8/spec/rails_app/config/application.rb#L18 so this forces the minitest assertions to load.
I don't know much about mongoid.
LGTM. Thank you for your contribution! |
For #173.
I would respectfully suggest that 6.0 and lower be dropped soon. Rails 7.2 is in beta and 8 is in heavy development. This would also drop dynamoid support.