Fixes #27585 - default dns timeout value is nil

[lzap]

This should fix the issue and sets up a new way we should be "migrating"
settings. These settings operations are usually:

• rename a setting
• change type of a setting

In my case this is both, but the idea is to simply do this during seed
instead of Rails migration which is very clunky because Rails
migrations:

• happen before setting are populated for new deployments
• happen with old values on existing deployments or upgrades
• are the wrong tool for the job

A seed script will always happen after settings are initialized and are
the right tool for the job. Migrations should manipulate schemas, seed
scripts should manipulate data.

[theforeman-bot]

Issues: #27585

[lzap]

Rebased.

[lzap]

This is what I think should fix it. I will be honest - I don't have time for full 1.23 installation with upgrade to 1.24 and reproducing this. So I made the seed task as generic as possible - when anything is not expected it tries to fix it.

[ekohl]

I think seeds are not intended for migrations. Seeds are for providing initial data. This will also be executed on every single upgrade and test run where migrations are only executed once. 👎 from me.

[ehelms]

I'm trying to follow where migrations fall down given the intent is to migrate data from one state to another. If settings are not created until Rails starts for the first time, maybe that design is more the problem. Just thinking a bit outloud because we've been having discussions about how seeds are used for a variety of functions and are hard to track.

[tbrisker]

I agree with @ekohl, we do have a lot of data changes in migrations. Seeds are for creating the initial state, not modifying the existing state.
Settings are a bit of an odd bird since we do their data seeding in the initializers instead of the seed, and updating them is a bit more complex, but we still have multiple migrations where we modify setting values.

[ares]

If I'm not mistaken, settings are initialized before first migration runs. Rails environment is already loaded in migrations. Therefore, Setting should always be there. The migration just needs to check, if the old setting is present and if that's the case, migrate it. If not, it means this was new deployment and the setting is initialized correctly from the first step.

So I guess, we just need to move the code from seed to migration?

[tbrisker]

@lzap what's the status here? iiuc the consensus is that we shouldn't do that in the seed, but rather fix in a migration.
It looks like the reason this is failing is that nil setting, which should mean "fall back to default" was serialized by the previous migration, causing the lookup to return the decoded nil instead of the default. the culprit is in: https://github.com/theforeman/foreman/blob/develop/app/models/setting.rb#L123-L125

since if we serialized nil into the value we would get:

Setting.find_by_name('dns_timeout')[:value] #=> "--- \n"
Setting[:dns_timeout] #=> nil

Therefor, the fix for this should be as simple as a migration that does:

Setting.find_by_name('dns_timeout').update_column(:value, nil) if Setting[:dns_timeout].nil?

[tbrisker]

to reproduce the original issue, run

Setting.find_by_name('dns_timeout').update_column(:value, nil.to_yaml)

[lzap]

Is it just me who thinks that all settings (including those managed by UI/API/CLI) should be stored in plain files (Yaml, JSON or whatever does not matter)? Number one offender in ActiveRecord instance allocation is Settings model, no surprise. It's slow. We need to cache it. It's not practical. And migrations are challenge (serialization would not necessary improve that tho).

[lzap]

@tbrisker hmmm this does not appear to work:

[11] pry(main)> Setting.find_by_name('dns_timeout').update_column(:value, nil.to_yaml)
=> true
[12] pry(main)> Setting.find_by_name('dns_timeout').value
=> nil
[13] pry(main)> Setting[:dns_timeout]
=> nil

[lzap]

Ok so the correct statement is:

unless Setting[:dns_timeout].is_a?(Integer) || Setting[:dns_timeout].is_a?(Array)
  Setting.find_by_name('dns_timeout').update_column(:value, nil)
end

[lzap]

This should do it then if you prefer migrations.

[tbrisker]

@tbrisker hmmm this does not appear to work:

[11] pry(main)> Setting.find_by_name('dns_timeout').update_column(:value, nil.to_yaml)
=> true
[12] pry(main)> Setting.find_by_name('dns_timeout').value
=> nil
[13] pry(main)> Setting[:dns_timeout]
=> nil

that's exactly what it should be displaying, if you check Setting.find_by_name('dns_timeout')[:value] you will see it is nil serialized to yaml. once you update the column to nil (not nil.to_yaml), you'll get the default for both of those (though you might need to clear your cache afterward).
Anyways the current fix works for me, but looks like your rebase pulled in an extra commit for some reason.

[lzap]

ECOFFEE rebased.

[tbrisker]

Looks like this fails if the settings haven't been initialized yet, let's be defensive:

Suggested change

	Setting.find_by_name('dns_timeout').update_column(:value, nil)
	Setting.find_by_name('dns_timeout')&.update_column(:value, nil)

[lzap]

Facepalm rebased.

[tbrisker]

Thanks @lzap !

[tbrisker]

1.24 - 4ddb8c0