allow MiqDialog to be seeded from plugins... #14653

durandom · 2017-04-05T16:03:44Z

...and add specs to test it actually does

required to merge ManageIQ/manageiq-providers-azure#16

@miq-bot assign @bdunne
@miq-bot add_labels enhancement

and add spec to test it

bdunne

Cool! I'm excited to get this moving. I know @djberg96 has a PR open to move some dialogs, but it should depend on this PR.

bdunne · 2017-04-05T16:10:46Z

app/models/miq_dialog.rb

-  def self.sync_from_dir
-    Dir.glob(File.join(DIALOG_DIR, "*.yaml")).each { |f| sync_from_file(f) }
+  def self.sync_from_dir(root)
+    root = root.join(DIALOG_DIR)


I would avoid redefining root

Also, in provider plugins, I would expect dialogs to live in content/dialogs to match the pattern we started in the manageiq-content repo.

durandom · 2017-04-05T17:10:13Z

app/models/miq_dialog.rb

-    Dir.glob(File.join(DIALOG_DIR, "*.yaml")).each { |f| sync_from_file(f) }
+  def self.sync_from_plugins
+    Vmdb::Plugins.instance.registered_provider_plugins.each do |plugin|
+      sync_from_dir(plugin.root.join('content', 'dialogs'))


do we really want to seed from content/dialogs ?
Or is it content/dialogs/miq_dialogs ?

TBH I have no idea what both are :/

Sorry, forgot there were two directories in there. I think content/miq_dialogs and content/service_dialogs would be a good layout.

durandom · 2017-04-05T17:10:42Z

@bdunne adressed feedback in last commit. See also my question about dialogs vs miq_dialogs

durandom · 2017-04-05T18:14:26Z

@bdunne amended the last commit to use content/miq_dialogs

bdunne

Code looks good, but I have a few comments about the specs

bdunne · 2017-04-05T19:34:23Z

spec/models/miq_dialog_spec.rb

+  context '.seed' do
+    it 'seeds from the core with correct metadata' do
+      root = Rails.root.join('product', 'dialogs', 'miq_dialogs')
+      allow(described_class).to receive(:find_by)


I think expect(described_class).to receive(:find_by).once.with(...) would be much simpler than allow and expect().to have_received().

right, when I was trying that, rspec complained about the other calls - now it works 🤷‍♂️
thanks for letting me re-visit that

here it is still, I need to stub find_by with allow(described_class).to receive(:find_by) otherwise I get this:
same is true for :sync_from_dir below.

Failures: 1) MiqDialog.seed seeds from the core with correct metadata Failure/Error: rec = find_by(:name => item[:name], :filename => item[:filename]) #<MiqDialog(id: integer, name: string, description: string, dialog_type: string, content: text, default: boolean, filename: string, file_mtime: datetime, created_at: datetime, updated_at: datetime, href_slug: string, region_number: integer, region_description: string) (class)> received :find_by with unexpected arguments expected: ({:name=>"miq_host_provision_dialogs", :filename=>"miq_host_provision_dialogs.yaml"}) got: ({:name=>"miq_provision_amazon_dialogs_template", :filename=>"miq_provision_amazon_dialogs_template.yaml"}) Diff: @@ -1,3 +1,3 @@ -[{:name=>"miq_host_provision_dialogs", - :filename=>"miq_host_provision_dialogs.yaml"}] +[{:name=>"miq_provision_amazon_dialogs_template", + :filename=>"miq_provision_amazon_dialogs_template.yaml"}] # ./app/models/miq_dialog.rb:36:in `sync_from_file' # ./app/models/miq_dialog.rb:20:in `block in sync_from_dir' # ./app/models/miq_dialog.rb:20:in `each' # ./app/models/miq_dialog.rb:20:in `sync_from_dir' # ./app/models/miq_dialog.rb:15:in `seed' # ./spec/models/miq_dialog_spec.rb:11:in `block (3 levels) in <top (required)>' # -e:1:in `<main>'

bdunne · 2017-04-05T19:36:34Z

spec/models/miq_dialog_spec.rb

+
+    it 'seed from plugins' do
+      mock_engine = double(:root => Pathname.new('/some/root'))
+      allow(Vmdb::Plugins.instance).to receive(:registered_provider_plugins).and_return([mock_engine])


Shouldn't this be expect? I feel like this is an important part of the code.

ok - why not :)

bdunne · 2017-04-05T19:38:19Z

spec/models/miq_dialog_spec.rb

+    it 'seed from plugins' do
+      mock_engine = double(:root => Pathname.new('/some/root'))
+      allow(Vmdb::Plugins.instance).to receive(:registered_provider_plugins).and_return([mock_engine])
+      allow(described_class).to receive(:sync_from_dir)


Same comment about allow with expect().to have_received()

durandom · 2017-04-06T06:40:03Z

@bdunne addressed your comments in the last commit

durandom · 2017-04-06T12:44:22Z

app/models/miq_dialog.rb

+  end
+
+  def self.sync_from_dir(root)
+    Dir.glob(root.join("*.{yaml,yml}")).each { |f| sync_from_file(f, root) }


I'm refactoring seeding of dialog.rb as well and noticed this loads .yaml and .yml files. So should we do here too

durandom · 2017-04-06T12:44:46Z

added another commit to also load .yml files

miq-bot · 2017-04-06T12:47:38Z

Checked commits durandom/manageiq@a2cf41b~...469689a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

bdunne · 2017-04-06T13:44:30Z

app/models/miq_dialog.rb

+  end
+
+  def self.sync_from_dir(root)
+    Dir.glob(root.join("*.{yaml,yml}")).each { |f| sync_from_file(f, root) }


bdunne · 2017-04-06T13:45:48Z

spec/models/miq_dialog_spec.rb

+  context '.seed' do
+    it 'seeds from the core with correct metadata' do
+      root = Rails.root.join('product', 'dialogs', 'miq_dialogs')
+      allow(described_class).to receive(:find_by)


allow MiqDialog to be seeded from plugins

a2cf41b

and add spec to test it

miq-bot assigned bdunne Apr 5, 2017

miq-bot added the enhancement label Apr 5, 2017

bdunne requested changes Apr 5, 2017

View reviewed changes

durandom commented Apr 5, 2017

View reviewed changes

durandom mentioned this pull request Apr 5, 2017

Remove Azure dialogue template #14655

Merged

seed plugins from content/dialogs

f7a5f3d

durandom force-pushed the miq_dialog_seed branch from 6cae6a8 to f7a5f3d Compare April 5, 2017 18:13

bdunne requested changes Apr 5, 2017

View reviewed changes

refactor specs

402bdd5

durandom force-pushed the miq_dialog_seed branch from 3f8f31a to 402bdd5 Compare April 6, 2017 06:39

also load .yml files

469689a

durandom commented Apr 6, 2017

View reviewed changes

durandom mentioned this pull request Apr 6, 2017

allow seeding of dialogs from plugins #14668

Merged

bdunne approved these changes Apr 6, 2017

View reviewed changes

bdunne merged commit 6410786 into ManageIQ:master Apr 6, 2017

durandom deleted the miq_dialog_seed branch April 6, 2017 15:17

bdunne added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 6, 2017

bdunne added fine/no pluggability/providers labels Apr 6, 2017

durandom mentioned this pull request Apr 11, 2017

Add Transform VM service dialog #13794

Closed

durandom mentioned this pull request Apr 21, 2017

Pluggable Providers TODO #14840

Closed

38 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow MiqDialog to be seeded from plugins... #14653

allow MiqDialog to be seeded from plugins... #14653

durandom commented Apr 5, 2017

bdunne left a comment

bdunne Apr 5, 2017

bdunne Apr 5, 2017

durandom Apr 5, 2017

bdunne Apr 5, 2017

durandom commented Apr 5, 2017

durandom commented Apr 5, 2017

bdunne left a comment

bdunne Apr 5, 2017

durandom Apr 6, 2017

durandom Apr 6, 2017

bdunne Apr 6, 2017

bdunne Apr 5, 2017

durandom Apr 6, 2017

bdunne Apr 5, 2017

durandom commented Apr 6, 2017

durandom Apr 6, 2017

bdunne Apr 6, 2017

durandom commented Apr 6, 2017

miq-bot commented Apr 6, 2017

bdunne Apr 6, 2017

bdunne Apr 6, 2017

allow MiqDialog to be seeded from plugins... #14653

allow MiqDialog to be seeded from plugins... #14653

Conversation

durandom commented Apr 5, 2017

bdunne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Apr 5, 2017

durandom commented Apr 5, 2017

bdunne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Apr 6, 2017

miq-bot commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment