[WIP] Support Rails 8 #824
Open
+67
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
EDIT: This is fixed in #826 for rails 7.2 I think what's happening is db:schema:load is run on the database after migrate so some constraints already exist. I will need to look into it because it looks like the load from schema.rb is trying to do something already done. This is what it looks on rails 7.2: On rails 8, it looks like it thinks the schema migration table didn't exist so it tried to create the db and load from schema.rb: def initialize_database(db_config)
with_temporary_pool(db_config) do
begin
database_already_initialized = migration_connection_pool.schema_migration.table_exists?
rescue ActiveRecord::NoDatabaseError
create(db_config)
retry
end
unless database_already_initialized
schema_dump_path = schema_dump_path(db_config)
if schema_dump_path && File.exist?(schema_dump_path)
load_schema(db_config)
end
end
!database_already_initialized
end |
11 tasks
Fryguy
reviewed
Dec 16, 2025
| strategy: | ||
| matrix: | ||
| ruby-version: | ||
| - '3.1' |
Member
There was a problem hiding this comment.
Oh nice - I missed this one - my rudimentary "is this an MIQ repo" checked exactly 3.1 and 3.3 (which is a weird combo, and why I thought it was a safe heuristic), but this repo apparently also had 3.4
jrafanie
commented
Dec 16, 2025
jrafanie
added a commit
to jrafanie/manageiq-schema
that referenced
this pull request
Dec 16, 2025
…hema.rb We need to do inheritance in both migrations and from schema dump/load. For constraints, we don't need to do them from schema load as t.check_constraints is now included for each metric_* subtable in the dumped schema.rb. Simplify the interface to add_miq_metric_table_constraint to pass the array of conditions inline. Note, this issue was found in ManageIQ#824 and extracted and simplified here.
jrafanie
added a commit
to jrafanie/manageiq-schema
that referenced
this pull request
Dec 16, 2025
We need to do inheritance in both migrations and from schema dump/load. For constraints, we don't need to do them from schema load as t.check_constraints is now included for each metric_* subtable in the dumped schema.rb, so we no longer pass the conditions from schema.rb dump/load. Simplify the interface to add_miq_metric_table_constraint to pass the array of conditions inline. Note, this issue was found in ManageIQ#824 and extracted and simplified here.
jrafanie
added a commit
to jrafanie/manageiq-schema
that referenced
this pull request
Dec 18, 2025
We need to do inheritance in both migrations and from schema dump/load. For constraints, we don't need to do them from schema load as t.check_constraints is now included for each metric_* subtable in the dumped schema.rb, so we no longer pass the conditions from schema.rb dump/load. Simplify the interface to add_miq_metric_table_constraint to pass the array of conditions inline. Note, this issue was found in ManageIQ#824 and extracted and simplified here.
Member
|
This pull request is not mergeable. Please rebase and repush. |
1 similar comment
Member
|
This pull request is not mergeable. Please rebase and repush. |
The migrate interface changed from 7.2 to 8.0: - def migrate(version = nil) + def migrate(version = nil, skip_initialize: false)
Fryguy
reviewed
Dec 18, 2025
Fryguy
reviewed
Dec 18, 2025
| track_miq_metric_table_inheritance(table) | ||
| end | ||
|
|
||
| def inherited_table_names |
Member
There was a problem hiding this comment.
If we have this method, do we need the track_miq_metric_table_inheritance method (and then inherited_metrics_tables method) anymore? It feels redundant. I see track_miq_metric_table_inheritance also needs the base table, but perhaps we can update inherited_table_names to also include that data as well (or have a shared method like inherited_table_details, and then inherited_tables_names calls that)
Member
There was a problem hiding this comment.
FWIW, I like your way better - feels more generic.
Member
Author
There was a problem hiding this comment.
I was debating having a single method that handled 7.2 and 8.0 but thought it would be easier to not touch the 7.2 logic and just delete the conditionals and the code for 7.2 when we're on 8.0.
This commit allows rails 7.2 to operate as it previously did. For 8.0, we have to tell it to ignore our inherits tables so we can dump them after the base table is dumped. This is because metric_rollups_base is AFTER metric_rollups_01, etc. when sorted The same problem happens with metrics_base sorting AFTER metrics_00. Rails 8.0 added inherits support in schema dumper here: https://www.github.com/rails/rails/pull/50475 A possibly related issue regarding table order with dumping inherits: https://www.github.com/rails/rails/issues/54841 A possible solution may come in a future release as part of: https://www.github.com/rails/rails/pull/54872
Fryguy
reviewed
Dec 19, 2025
| end | ||
|
|
||
| def inherited_table_names | ||
| @inherited_table_names ||= @connection.tables.select { |t| ActiveRecord::Base.connection.inherited_table_names(t).present? } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.