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

Fixes #33909 - Drop data migrations before Foreman 2.0 #8920

Merged
merged 6 commits into from Nov 15, 2021

Conversation

tbrisker
Copy link
Member

All data migrations only modify data on upgrades. On new installs they
are not required. Since we do not generally support upgrading multiple
releases, we can clean up old data migrations. This commit cleans up all
the data migrations that were present before Foreman 2.0.

Second commit, since I know this topic may be controversial:

Refs #33909 - Delete empty migrations

New installs will not run these empty migrations, and old installs will
only list that they have been executed when checking the rake db:migrate:status with no file, which is harmless.

@theforeman-bot
Copy link
Member

Issues: #33909

@ezr-ondrej
Copy link
Member

Could we add some purge task to remove the migrations from schema_migrations? It would be my only concern with deleting migrations - having a way to remove them from DB (and also having some list of such)

@tbrisker
Copy link
Member Author

@ezr-ondrej we could, but I don't see much benefit in doing so - if we keep them, we can tell that they ran in the past even if they no longer exist (i.e. identify if the db is new or was upgraded).
Having timestamps in schema_migrations without files is valid. Many rails apps delete all old migrations, when the schema.rb or structure.sql file is committed in source control and is used as the base db structure - which unfortunately we cannot do due to $reasons.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I still would have preferred to really squash the migrations so avoid changing tables, but I can live with this as a start.

Operatingsystem.all.each do |os|
case os.name
when /RedHat|Centos|Fedora/i
os.family_id = Operatingsystem::FAMILIES.index :RedHat
Copy link
Member

Choose a reason for hiding this comment

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

Fun fact: I can't see how this worked. AFAIK Families never had an entry which accepted symbols and it always was Redhat, not RedHat.

class FakeSubnet < ApplicationRecord
self.table_name = 'subnets'
end

def up
add_column :subnets, :ipam_tmp, :string, :default => IPAM::MODES[:dhcp], :null => false, :limit => 255
Copy link
Member

Choose a reason for hiding this comment

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

If you're dropping the migration, there's no reason for a tmp column. Can't this be squashed down to a change_column? Same comment for down.

Comment on lines -3 to -4
if User.unscoped.find_by_login(User::ANONYMOUS_ADMIN).present?
User.as_anonymous_admin do
Copy link
Member

Choose a reason for hiding this comment

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

Was this here to disable auditing or something? This is fairly recent (d60d1cb) so maybe @ares remembers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, possibly an attempt to bypass taxonomy or authorization scope. update_all triggers sql UPDATE query directly without initializing AR objects so it wouldn't have been audited in any case.

@ekohl
Copy link
Member

ekohl commented Nov 15, 2021

Tests fail with:

NoMethodError: undefined method `find_by_lower_login' for #<Class:0x0000000009c55fb8>
Did you mean?  find_by_login

@tbrisker
Copy link
Member Author

I've re-added the column info reset to the migration that adds lower_login to users. While the specific migration that fails should be fixed by GH-8918, there could potentially be other migrations in plugins that try looking up users (which they shouldn't, but that's outside the scope of this PR to fix).

All data migrations only modify data on upgrades. On new installs they
are not required. Since we do not generally support upgrading multiple
releases, we can clean up old data migrations. This commit cleans up all
the data migrations that were present before Foreman 2.0.
New installs will not run these empty migrations, and old installs will
only list that they have been executed when checking the `rake
db:migrate:status` with no file, which is harmless.
There is no need to lookup the admin user and use it to update the
http proxy fields.
While migrations shouldn't rely on any users existing in the DB, there
are some that currently do. If they try to use User.find_by_lower_login
they will currently error out since the method won't be defined yet.
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I agree that ultimate goal should be migration squash, but this is definitelly a step in the right direction. So big 👍

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I wonder if this has a (positive) performance impact or if it's negligible. I'll see this as a step in the right direction to actually squashing.

@ezr-ondrej
Copy link
Member

Thanks @tbrisker ! :)

@ezr-ondrej ezr-ondrej merged commit 4b440ef into theforeman:develop Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants