Add support for exporting and importing provision dialogs #17739

branic · 2018-07-22T02:56:36Z

These rake scripts and classes provide functionality for exporting/importing of the following ManageIQ object types:

Provision Dialogs

This PR uses the framework that was implemented for PRs #14126, #15256, #16717, and #16983 to export/import other ManageIQ object types.

These scripts are based on the CFME RH Consulting Scripts and are used by Red Hat consultants to enable storing customizations in Git and maintaining customizations between environments (e.g. dev/qa/prod) for an SDLC lifecycle.

Links [Optional]

Tracking Issue: Implement export/import functionality of Miq objects via command-line

Steps for Testing/QA [Optional]

Exporting

Create a directory for the exports

mkdir /tmp/provision_dialogs

Export provision dialogs

vmdb
bin/rake evm:export:provision_dialogs -- --directory /tmp/provision_dialogs

Importing

Import all provision dialog yaml files in a directory

# vmdb
# bin/rake evm:import:provision_dialogs -- --source /tmp/provision_dialogs

or Import specific provision_dialog yaml file

# vmdb
# bin/rake evm:import:provision_dialogs -- --source /tmp/provision_dialogs/Test_Dialog.yaml

branic · 2018-07-22T02:59:36Z

/cc @gtanzillo @gmcculloug

gmcculloug · 2018-07-23T13:02:43Z

lib/task_helpers/exports/provision_dialogs.rb

+        end
+
+        dialogs.each do |dialog|
+          $log.send(:info, "Exporting Provision Dialog: #{dialog[:name]} (#{dialog[:description]})")


No need to call send. Use $log.info

gmcculloug · 2018-07-23T13:02:56Z

lib/task_helpers/imports/provision_dialogs.rb

+
+        glob = File.file?(options[:source]) ? options[:source] : "#{options[:source]}/*.yaml"
+        Dir.glob(glob) do |fname|
+          $log.send(:info, "Importing Provision Dialog from: #{fname}")


d-m-u · 2018-07-23T14:45:46Z

lib/task_helpers/exports/provision_dialogs.rb

+      def export(options = {})
+        export_dir = options[:directory]
+
+        dialogs = if options[:all]


This could be a ternary.

d-m-u · 2018-07-23T14:47:17Z

lib/task_helpers/exports/provision_dialogs.rb

+                    MiqDialog.order(:id).where(:default => [false, nil])
+                  end
+
+        dialogs = dialogs.collect do |dialog|


Could use collect! and not need the variable.

collect! isn't defined for an ActiveRecord::Relation [] so the only option is using collect

NoMethodError: undefined method `collect!' for #<ActiveRecord::Relation []> Did you mean? collect

d-m-u · 2018-07-23T14:48:01Z

lib/task_helpers/imports/provision_dialogs.rb

+
+          miq_dialog = MiqDialog.where(:name => dialog[:name]).first
+
+          if miq_dialog.nil? then


This could be a ternary too.

And the then shouldn't be necessary.

d-m-u · 2018-07-23T14:49:17Z

Other than a couple silly nits, looks great! Thanks!

d-m-u · 2018-07-23T15:15:23Z

lib/task_helpers/imports/provision_dialogs.rb

+
+          dialog = YAML.load_file(fname)
+
+          miq_dialog = MiqDialog.where(:name => dialog[:name]).first


Prefer find_by(:name => dialog[:name]) instead of where().first

These are uniq'd by dialog type, does this need to account for that as well?

Both the export and import need to account for them being uniq'd by dialog type. On the export side, with the way the filename is currently generated, when there are two dialogs of different types that have the same name one will overwrite the other.

eclarizio

I don't currently have anything else to add to what @d-m-u and @gmcculloug have said.

@branic Let me know when you've made the requested changes and I'll take another look.

👍

miq-bot · 2018-07-25T15:41:07Z

This pull request is not mergeable. Please rebase and repush.

branic · 2018-07-28T02:18:48Z

@eclarizio, @d-m-u, @gmcculloug I've addressed the comments with the latest commit. I had to rebase so in the process I squashed the commits into a single commit.

gmcculloug · 2018-07-30T17:14:24Z

lib/task_helpers/exports/provision_dialogs.rb

+
+        dialogs = options[:all] ? MiqDialog.order(:id).all : MiqDialog.order(:id).where(:default => [false, nil])
+
+        dialogs = dialogs.collect do |dialog|


Minor de-duping. You can move the order(:all) down to the caller.

dialogs = options[:all] ? MiqDialog.all : MiqDialog.where(:default => [false, nil]) dialogs = dialogs.order(:id).collect do |dialog|

gmcculloug · 2018-07-30T19:38:34Z

@eclarizio PTAL

eclarizio

I originally had some of the same nitpicks I saw you fixed, but now it looks good overall. 👍

The only reservation I have at first is from the exports/provision_dialogs_spec.rb, there's the tests for what happens when --all isn't specified and when it is, but it doesn't at first appear that anything is different. It's particularly confusing that you are expecting the file count to be 1 for one of the tests when --all isn't specified, and then 2 for the second test, and then when --all is specified, you're expecting the count to be 2 again.

I understand that the specs are different because they're checking different descriptions, but it's just not immediately apparent.

I'm not sure the best way to remedy this, perhaps breaking up the specs into separate it blocks? Something like:

# bunch of setup here for all commonalities

it "exports all provision dialogs to a given directory" do
  expect(<filecount>).to eq(2)
end

it "matches the first dialog content and description to the first dialog" do
  dialog1 = YAML.load_file(dialog_filename1)
  expect(dialog1[:content]).to eq(content)
  expect(dialog1[:description]).to eq(dialog_desc1)
end

it "matches the second dialog content and description to the second dialog" do
  dialog2 = YAML.load_file(dialog_filename2)
  expect(dialog2[:content]).to eq(content)
  expect(dialog2[:description]).to eq(dialog_desc2)
end

# etc.

It's a pretty minor nitpick, so I'm going to approve, but something to think about going forward.

branic · 2018-07-30T22:31:17Z

@eclarizio With the 2nd it (starting on line 85) what I'm trying to test is that two different types of provisioning dialogs with the same name are saved to different files.

I can rework the test to try and be clearer in what is being tested. I'm thinking that I can break the it block on line 85 into its own describe block as it really applies regardless of whether the --all option was specified.

Thoughts?

eclarizio · 2018-07-30T22:49:39Z

@branic Right, I got that after reading it more closely, I was mainly confused at first glance. What you suggested is probably a good way to go about it 👍

miq-bot · 2018-07-30T23:16:53Z

Checked commits https://github.com/branic/manageiq/compare/1a97e432cb3f7a1eae53ff0f2be427b27c39d407~...7357e42e2e838f293f17d3887d549e0d6b1987d9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🏆

branic · 2018-07-31T19:05:50Z

@eclarizio does this rework of the spec look ok?

eclarizio · 2018-08-01T16:59:08Z

@branic Yeah, that looks fine 👍

branic mentioned this pull request Jul 22, 2018

Implement export/import functionality of Miq objects via command-line #15350

Open

18 tasks

gmcculloug reviewed Jul 23, 2018

View reviewed changes

gmcculloug self-assigned this Jul 23, 2018

gmcculloug requested review from eclarizio and d-m-u July 23, 2018 14:37

gmcculloug added enhancement lifecycle/provisioning labels Jul 23, 2018

d-m-u reviewed Jul 23, 2018

View reviewed changes

eclarizio reviewed Jul 23, 2018

View reviewed changes

miq-bot added the unmergeable label Jul 25, 2018

Add support for exporting and importing provision dialogs

1a97e43

miq-bot removed the unmergeable label Jul 30, 2018

ManageIQ deleted a comment from branic Jul 30, 2018

gmcculloug reviewed Jul 30, 2018

View reviewed changes

De-dup order statement and use collect!

7aa8fce

eclarizio approved these changes Jul 30, 2018

View reviewed changes

Make spec test clearer

7357e42

gmcculloug merged commit b44e073 into ManageIQ:master Aug 1, 2018

gmcculloug added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 1, 2018

branic deleted the provision_dialogs_export_import branch August 1, 2018 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for exporting and importing provision dialogs #17739

Add support for exporting and importing provision dialogs #17739

branic commented Jul 22, 2018

branic commented Jul 22, 2018

gmcculloug Jul 23, 2018

gmcculloug Jul 23, 2018

d-m-u Jul 23, 2018

d-m-u Jul 23, 2018

branic Jul 28, 2018

d-m-u Jul 23, 2018

d-m-u Jul 24, 2018

d-m-u commented Jul 23, 2018

d-m-u Jul 23, 2018

d-m-u Jul 23, 2018

branic Jul 28, 2018 •

edited

Loading

eclarizio left a comment

miq-bot commented Jul 25, 2018

branic commented Jul 28, 2018

gmcculloug Jul 30, 2018

gmcculloug commented Jul 30, 2018

eclarizio left a comment

branic commented Jul 30, 2018

eclarizio commented Jul 30, 2018

miq-bot commented Jul 30, 2018

branic commented Jul 31, 2018

eclarizio commented Aug 1, 2018


		miq_dialog = MiqDialog.where(:name => dialog[:name]).first

		if miq_dialog.nil? then


		dialog = YAML.load_file(fname)

		miq_dialog = MiqDialog.where(:name => dialog[:name]).first


		dialogs = options[:all] ? MiqDialog.order(:id).all : MiqDialog.order(:id).where(:default => [false, nil])

		dialogs = dialogs.collect do \|dialog\|

Add support for exporting and importing provision dialogs #17739

Add support for exporting and importing provision dialogs #17739

Conversation

branic commented Jul 22, 2018

Links [Optional]

Steps for Testing/QA [Optional]

branic commented Jul 22, 2018

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

d-m-u commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branic Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

eclarizio left a comment

Choose a reason for hiding this comment

miq-bot commented Jul 25, 2018

branic commented Jul 28, 2018

Choose a reason for hiding this comment

gmcculloug commented Jul 30, 2018

eclarizio left a comment

Choose a reason for hiding this comment

branic commented Jul 30, 2018

eclarizio commented Jul 30, 2018

miq-bot commented Jul 30, 2018

branic commented Jul 31, 2018

eclarizio commented Aug 1, 2018

branic Jul 28, 2018 •

edited

Loading