Fixes #37143 - Register plugin's template dirs for db:seed

[stejskalleos]

No description provided.

[stejskalleos]

For plugin usage, see https://github.com/theforeman/foreman_ansible/pull/692/files

[ofedoren]

For plugin usage, see https://github.com/theforeman/foreman/pull/10038/files

Wrong link? :)

[ekohl]

An alternative thought: would it make sense to adopt a convention? Convention over configuration is a common design principle in Rails.

In our case it would mean that for every plugin we would agree on a directory path and if that path exists, Foreman loads it.

[ofedoren]

Also, after thinking why we didn't have this earlier, we did, but just in a different way: https://github.com/theforeman/foreman_openscap/blob/master/db/seeds.d/75-job_templates.rb.

Does it mean this PR should unify the way the templates are being seeded? If so, wouldn't it conflict with the current way? I mean, it would mean some plugins will try to seed templates twice...

[ekohl]

Also, after thinking why we didn't have this earlier, we did, but just in a different way: https://github.com/theforeman/foreman_openscap/blob/master/db/seeds.d/75-job_templates.rb.

That has a condition, but Foreman has a mechanism to depend on plugins. We would need to migrate the plugins to adopt that if needed.

And more realistically, REX should provide a seed that checks all job templates. That may be more complex, but not impossible.

Does it mean this PR should unify the way the templates are being seeded? If so, wouldn't it conflict with the current way? I mean, it would mean some plugins will try to seed templates twice...

I like that idea. Perhaps we can deal with it by calling uniq, though by now I think an often better approach is to actively update all plugins when you can than to write a compatibility layer.

Though one possible obvious way is to create a new directory. I've always thought app/views is an odd place for them. We can introduce app/templates/$type. That way a plugin can move over all their templates to opt-in to the new behavior.

[stejskalleos]

An alternative thought: would it make sense to adopt a convention? Convention over configuration is a common design principle in Rails.

I like it and agree. This PR would require changes in the plugins anyway, so let's go with the convention way.

Does it mean this PR should unify the way the templates are being seeded?

Yes, that's the goal.

I will update the code to follow a convention that all plugins must have templates in the :foreman_plugin/app/views/:foreman_plugin/:kind/*.erb.
And will push PRs in the plugins and post about it on the community web.

[ofedoren]

We can introduce app/templates/$type.

I'd give my vote for that. Even though I'm quite used to look for the templates in app/views, it still pokes me in the eye that it's not... a view.

[stejskalleos]

@ofedoren @ofedoren updated

Foreman will look for templates in :plugin/app/templates/:kind directories.
Other directories can be registered with :register_template_dirs method.

[ekohl]

Test failures look related.

[ekohl]

@ofedoren?

[ofedoren]

Haven't tested, but LGTM.

[ofedoren]

I've just tested it by adding a new template to the default path (app/templates) of a plugin and the template didn't seed automatically. I think the new paths don't get involved into seeding at all. It seems seeding is done by https://github.com/theforeman/foreman/blob/develop/app/services/foreman_seeder.rb#L37 which uses seed directories (https://github.com/theforeman/foreman/blob/develop/app/services/foreman_seeder.rb#L14) in which there is a file running a seed when loaded (https://github.com/theforeman/foreman/blob/develop/db/seeds.d/090-report_templates.rb#L3). Each template kind has its own file being loaded separately, these "mini-seeders" use SeedHelper.*_templates helpers explicitly. That's why it works now, but doesn't work with the new approach.

The fix is probably lying in adjustment of https://github.com/theforeman/foreman/blob/develop/app/services/foreman_seeder.rb#L14-L15.

Please, correct me if it works. Maybe I missed something...

[jeremylenz]

@stejskalleos Is there another PR replacing this?