Run migrations if there are pending migrations

[ehelms]

This drops relying on a settings entry in favor of lettings Rails
tell us when there are migrations that need to be applied.

[ehelms]

This is an alternative solution to (#776) for at least handling the migrate part of the database.

[ekohl]

I like this because of the simplicity. Looks like we should also drop those settings from Foreman itself after it's been merged.

[ekohl]

Not a blocker to merge, but any thoughts about seeding? I think the refresh can be used to always seed when we migrate, but I wonder about cases where we need to seed without a migration. Since we currently don't that either, I'd be fine with leaving that out.

Once the build is green I'm happy to merge this.

[ehelms]

I went that route originally -- assume if migration that seed should happen. And had the same thought, seed doesn't require a migration. The seeds are tougher because by design they ought to be idempotent but I think your desire is that the exec is idempotent. So rather than let db:seed determine if changes are necessary, you prefer the exec determine that before running the db:seed. The current logic we live by is if Foreman or a plugin updates, try to db:seed. If the updates were all controlled inside the installer we could use that to notify the db:seed exec. However, we can't control that. I think that leaves us with using a file for seeds unfortunately if the desire is to avoid running db:seed on each puppet run.

[ekohl]

Currently the seeding happens because notifications have a transitive property. It's not written in the current tests, but you can:

it { should contain_foreman__rake('db:migrate').that_notifies('Foreman::Rake[db:seed]') }

That actually passes which means that every time a migration happens, a seeding also happens. It's fairly safe to assume that this is enough. Time will tell if there's ever a case something needs to be seeded to work but doesn't have a DB migration.

[ekohl]

This needs to be false when unless is set. Otherwise the whole trick still doesn't work.

[ehelms]

A plugin adding notifications is an example of seed with no migration, e.g. theforeman/foreman_ansible@d37ad23#diff-90c017e7983b50d96418476d81b442db

[ekohl]

You still need to change the refreshonly code.

A plugin adding notifications is an example of seed with no migration, e.g. theforeman/foreman_ansible@d37ad23#diff-90c017e7983b50d96418476d81b442db

Without this change that isn't covered either. Let's get this in and think about seeding in the next stage.

[ekohl]

I'd prefer an explicit test that passes in an unless to foreman::rake. In https://github.com/theforeman/puppet-foreman/compare/master...ekohl:fixes?expand=1 I've written one.

[ekohl]

Test is failing on this.

Also odd that prometheus is failing. I already disabled that on Debian but now it's failing on centos as well. Perhaps that really is broken.

[ekohl]

👍 on green tests

[ehelms]

Looks like further debugging needed as its failing for CentOS 7.

[ekohl]

Are you reading what I'm writing?

Also odd that prometheus is failing. I already disabled that on Debian but now it's failing on centos as well. Perhaps that really is broken.

Since then it also looks like master is showing the same issue. That means I'd be ok with just merging this. Alternatively first merging a PR that disables the test similar to c0b9611

[ehelms]

Sorry, I was. The output just made me think it was a regular curl status check that was failing. I do see now that the test description mentions prometheus.