Skip to content
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

upgrade rails to 7.2 #523

Merged
merged 41 commits into from
Nov 1, 2024
Merged

upgrade rails to 7.2 #523

merged 41 commits into from
Nov 1, 2024

Conversation

jlvallelonga
Copy link
Contributor

No description provided.

@jlvallelonga
Copy link
Contributor Author

working towards the defaults for 7.2 and then I'll remove this file: config/initializers/new_framework_defaults_7_2.rb

@jlvallelonga jlvallelonga changed the title upgrade rails to 7 2 upgrade rails to 7.2 Oct 22, 2024
db/schema.rb Outdated
@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.1].define(version: 2024_09_15_034529) do
ActiveRecord::Schema[7.1].define(version: 2024_10_21_060860) do
# These are extensions that must be enabled in order to support this database
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notably there are migrations but the schema file didn't update. Were those migrations no-ops or do you need to run db:migrate still to get the schema updated?

The app's use of ActiveStorage is pretty hacky, since it's all uploaded image files are being stored in ActiveRecord. With these new migrations and this upgrade to 7.2, be sure to test out that functionality in your dev environment to make sure the various AIs can can still answer questions about images. (If can't remember if I got it working on all three or not, if it doesn't work on Llama it may be that I only implemented it for GPT and Claude)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the migrations are already in place, I think because we started on 7.1.3 but I wasn't certain if they were doing anything different with the 7.1.4 update so I just left them. I guess since the schema.rb didn't change then they can be removed. I'll check on the ActiveStorage functionality to make sure it's working

Copy link
Contributor

@krschacht krschacht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimming this and it looks good. I'm really glad we have so much test coverage on the app to make upgrades like this less risky. :) I left one comment about the migrations, but once you think this PR is good to go feel free to merge in

@jlvallelonga
Copy link
Contributor Author

getting this a lot with the 7.2 update: <ActiveRecord::ActiveRecordError: Cannot expire connection, it is not currently leased.>

@jlvallelonga
Copy link
Contributor Author

getting this a lot with the 7.2 update: <ActiveRecord::ActiveRecordError: Cannot expire connection, it is not currently leased.>

this might be a fix though: rails/rails#53147 (comment)

@@ -7,7 +7,7 @@ class APIService < ApplicationRecord

has_many :language_models, -> { not_deleted }

enum driver: %w[ openai anthropic ].index_by(&:to_sym)
enum :driver, %w[ openai anthropic ].index_by(&:to_sym)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old syntax is deprecated so moving to positional arguments with the enums

@jlvallelonga
Copy link
Contributor Author

jlvallelonga commented Oct 27, 2024

ok seems to be working, but we should wait on a release of 7.2 with a fix so we don't have to point to the 7-2-stable branch - something later than 7.2.1.2 will hopefully have the fix: https://github.com/rails/rails/releases

@jlvallelonga jlvallelonga merged commit db435e7 into main Nov 1, 2024
6 checks passed
@jlvallelonga jlvallelonga deleted the chore/upgrade-rails-to-7-2 branch November 1, 2024 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants