-
Notifications
You must be signed in to change notification settings - Fork 987
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 #29498 - improve pending_migrations check #7574
Conversation
Issues: #29498 |
This comment has been minimized.
This comment has been minimized.
9e80029
to
9256785
Compare
f8de81e
to
68042bc
Compare
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.
No more comments from my side. Would be good to get someone who knows Rails to take a look as well.
rescue NameError => e | ||
Foreman::Logging.exception("Failed to register plugin #{name}", e) | ||
nil | ||
Dir.glob(plugin.engine.root + 'db/seeds.d/*.rb') if plugin.engine |
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.
Would it make sense to add a helper in the plugin itself to return this glob? Like plugin.seeds
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.
unsure about this one 🤔 we don't need it anywhere else, and there is the same path for foreman few lines above this one. I don't have preference tho, I can abstract it to the plugin if it makes more sense :)
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'm fine with this the way it is
needs rebase |
[test foreman] |
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.
Test failures seem related
end | ||
|
||
def migrations_paths | ||
engine.paths['db/migrate'].existent |
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.
The test failures seem to be related to the fact that engine
can be nil
, or at least in tests
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.
Yeah, then skipping the check is a valid option, as without engine, the plugin has no migrations. Done that, by early return.
🍏 |
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.
Looks good to me
Thank you @ezr-ondrej and everyone involved in the discussion! |
|
||
pending_migrations = ActiveRecord::Base.connection.migration_context.needs_migration? | ||
@pending_migrations = ActiveRecord::MigrationContext.new(migrations_paths, ActiveRecord::SchemaMigration).needs_migration? |
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.
It looks like this caused https://projects.theforeman.org/issues/30821. If a plugin has no migrations but uses permissions, this fails in packaging. I think it should still take the main DB into account, as well as plugins.
Check plugins migrations instead of core ones (or should we check both?).
More info in the ticket.