-
Notifications
You must be signed in to change notification settings - Fork 982
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
Fixes #24838 - Use integer representation for boolean on sqlite #7171
Conversation
Issues: #24838 |
Migration is not trivial, most fields can be converted using the following task but there are many that aren't covered in it (either in core or in plugins): if ActiveRecord::Base.connection.adapter_name.downcase.starts_with?('sqlite')
ActiveRecord::Base.connection.tables.map do |t|
t.classify.constantize rescue nil
end.compact.each do |t|
next unless t.respond_to? :columns
t.columns.each do |col|
next unless col.type == :boolean
next if col.name == 'default'
t.where("#{col.name} = 't'").update_all(col.name.to_sym => 1)
t.where("#{col.name} = 'f'").update_all(col.name.to_sym => 0)
end
end
end |
We might also declare SQLite as dev only and unsafe for upgrades or similar ;) |
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.
Haven't tested the migration, but the actual patch makes sense and works nicely for me.
upgrade test fails 💔 |
that's wierd, it failed to create an admin user on 1.14... [test upgrade] |
@tbrisker Perhaps it's saved in the "old" format on the old Rails version and with that setting the new Rails isn't detecting it correctly? |
@mmoll it is, the seed is failing to find it due to the way it checks for existing admin user. I'll work around that in a second. |
We don't really need to migrate boolean fields as sqlite isn't supported production database and is only used for running build tasks.
@@ -42,11 +42,13 @@ | |||
end | |||
|
|||
# First real admin user account | |||
unless User.unscoped.only_admin.except_hidden.present? |
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.
only_admin
doesn't find the exising admin user when upgrading due to the changed format. I've made this seed more sensitive so it first checks if a user with the admin login exists, which will also be good so it doesn't fail when trying to seed an existing user name.
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.
Fine for me 👍. @tbrisker Could you add an 1.24 upgrade warning with the migration script? I hope, not many people are using sqlite, especially in production...
Opened theforeman/theforeman.org#1501 but looks like we actually default to SQLite on non-installer installations, I wonder how many users we have that use sqlite in production due to this. |
opened an RFC on discourse to make sure we can really drop sqlite in production: https://community.theforeman.org/t/dropping-support-for-sqlite-as-production-database/16158 |
merged, thanks @tbrisker! |
We don't really need to migrate boolean fields as sqlite isn't supported
production database and is only used for running build tasks.