Make MiqProductFeature seeding pluggable #18806
Conversation
Fryguy
force-pushed
the
product_feature_seed_plugins
branch
from
432651a
to
ed02420
Compare
|
Checked commits Fryguy/manageiq@cb61dd4~...ed02420 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 spec/models/miq_product_feature_spec.rb
|
|
Note that I created a Seeding module in the PR, but didn't move much into it yet...I plan to do a larger refactoring to move that and improve the performance, but wanted to unstick any other PRs waiting on this feature first. |
| end | ||
|
|
||
| def seed_core_files | ||
| Dir.glob("#{FIXTURE_PATH}/*.y{,a}ml").sort |
There was a problem hiding this comment.
I appreciate the sort on Dir.glob ❤️
There was a problem hiding this comment.
I basically always sort that method now, because of its unpredictability... I'm almost tempted to create a more_core_extensions method that enforces it :)
| Dir.glob(path.join("*.yml")).each do |fixture| | ||
| seed_from_hash(YAML.load_file(fixture), seen, root_feature) | ||
| other_files.each do |file| | ||
| seed_from_array(YAML.load_file(file), seen, root_feature) |
There was a problem hiding this comment.
Let me say this to make sure I understand.
-
seed_from_hash is used for the core product features where the "everything" identifier is the root feature and all children fall under it
-
seed_from_array is for seeding an array of features all at the same level: direct children of the root "everything" feature. Each can have their own trees of features but they all get the "everything" feature as the root.
There was a problem hiding this comment.
Kind of? Yes. I didn't want to add the seed_from_array, but plugins can bring an array of features, so it made sense to abstract walking an array of hashes.
| @@ -18,8 +20,6 @@ class MiqProductFeature < ApplicationRecord | |||
| validates_presence_of :identifier | |||
| validates_uniqueness_of :identifier | |||
|
|
|||
| FIXTURE_PATH = Rails.root.join(*["db", "fixtures", table_name]) | |||
There was a problem hiding this comment.
haha, I didn't see this until now... I guess we were really worried about hardcoding "miq_product_features"... I can't believe I didn't see table_name before...
|
|
||
| module ClassMethods | ||
| def seed_files | ||
| return seed_root_filename, seed_core_files + seed_plugin_files |
There was a problem hiding this comment.
@Fryguy If I'm reading correctly, this doesn't break the UI plugin work by @martinpovolny because they should still be picked up in seed_core_file, right? Note, I'm not sure if we still need it but I'm making sure that I understand it. It looks to be still intact.
There was a problem hiding this comment.
Correct, this keeps @martinpovolny 's original "directory of feature yamls" in core, however that's not being used anywhere, so while we have it we may want to consider dropping it. On the plugin side though it could be useful to have separate yamls per feature in a dir, so I basically made plugins work like core.
There was a problem hiding this comment.
have it we may want to consider dropping it
Yes, I'd like to see it dropped. Also it does not support any nesting if I remember correctly.
Someone might me using it: https://documentation.commvault.com/commvault/v11_adminconsole/article?p=87519.htm and they should be provided with a way to migrate.
What we need with this new way is documentation.
|
LGTM so far |
|
|
||
| module ClassMethods | ||
| def seed_files | ||
| return seed_root_filename, seed_core_files + seed_plugin_files |
There was a problem hiding this comment.
As discussed, this return value feels strange to me, but I understand why it's done this way. Maybe in a future PR we can pull the everything feature out of the root file and remove the special handling of that file.
There was a problem hiding this comment.
I like this idea, and definitely think we should do it in a followup.
|
@jrafanie I just realized this might not play nice with i18n of features, due to https://github.com/ManageIQ/manageiq/blob/master/config/locale_task_config.yaml#L3-L5 However, I'd like to work on that in a follow up. cc @mzazrivec |
|
@jrafanie This is good to go |
@jrafanie Please review.
I want to run through other repo tests before merge, just to be sure this doesn't break anything.