Add rake script to export/import miq alerts and alert profiles #14126

branic · 2017-03-01T21:50:33Z

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

alerts (MiqAlert)
alert profiles (MiqAlertSet)

This PR also sets up the framework for future export/import PRs for 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.

PR 112 in manageiq-appliance is dependant on this PR and provides a bash script wrapper for these rake scripts.

Fryguy · 2017-03-02T15:34:10Z

@branic First off, thanks for all of this! Just wanted to state that so you know that my comments below don't reflect that we don't appreciate all of this work 😄

Is the goal to just get these in as is, and then clean them up afterwards? Or do we want to clean them up in the course of this PR? For one, the Red Hat references and namings have to be removed. (e.g. rhconsulting)
Rubocop and style issues will probably need to be cleaned up for much stuff. If the goal is to just get the original in and iterate on that, we can let that go. Also, the lib/tasks directory is probably not the right place for most of the classes and utility modules, so those would probably have to move.
Why are there commented lines of code? Are those optional or just unused?
I see authorship at the top of these files, but we generally don't have authorship in files as per our license the authors are the "ManageIQ Authors". Is it ok, if we move that? Was there a license on the source of these that we need to adhere to? If the author wants attribution, perhaps they should make the PR?

branic · 2017-03-02T16:37:45Z

Thanks for the comments @Fryguy. This first commit was just a straight copy of the files as they exist in the rhconsulting github. I'll work on cleaning up based on your comments and the Rubocop issues.

The commented code is from keeping old code when the code changed and I can remove them as part of the cleanup effort.

Are additional commits to this PR ok for the cleanups?

Fryguy · 2017-03-02T16:42:14Z

I think comments will come as the code progresses. I think the best approach to get this in faster and easier might be to slim this down to, say, only one of the import scripts + the accompanying helper modules. A 1700+ line PR is almost guaranteed to take a long time. However, if you trim it down to just one script, then we can iterate the review process on just that script, uncovering all of the various comments, and get it in quickly. Once it's in, now you have a foundation for more rake tasks (probably one per PR) and since all of the comments have been already iterated on and discussed, getting these new PRs in will be a LOT faster.

Fryguy · 2017-03-02T16:47:43Z

Are additional commits to this PR ok for the cleanups?

Yes, that is preferable

branic · 2017-03-11T02:58:03Z

@Fryguy I've removed all but the script to export/import alerts and the two helper modules from this PR. There has been some re-factoring to get rid of references to rhconsulting and general cleanup. I'm not sure what the best way to fix the last of the rubocop offenses.

Hopefully this makes it easier to review and can be used as a model for getting the other rake scripts added (in seperate PR's).

All feedback is appreciated.

carbonin

I'm thinking these helper classes should probably be namespaced a bit. Maybe Vmdb::Serializer::Alert (naming is hard). That would allow us to create a new Vmdb::Serializer for whatever we needed to add. Then common functionality like the illegal character and option parsing can go in the Vmdb::Serializer module itself. Then they would live in lib/vmdb/serializer.rb and lib/vmdb/serializer/alert.rb.

Judging from the amount of common code here, it feels like we could do even better with the design, but we can see how it evolves from there. What do you think @Fryguy ?

carbonin · 2017-03-15T17:27:58Z

lib/tasks/evm_import_export_illegal_chars.rb

+  @@regex_remove_spaces = %r{[/| ]}
+
+  # Replacement character
+  @@replacement         = '_'


These class variables should be constants.

carbonin · 2017-03-22T15:08:17Z

lib/tasks/evm_import_export_options.rb

@@ -0,0 +1,100 @@
+# @class EvmExportImportOptions
+# @brief Simple class to parse options passed in to a rake task.
+class EvmImportExportOptions


This class can probably be replaced by Trollop using the rake task options like

manageiq/lib/tasks/evm_dba.rake

Line 73 in 02c1f12

opts = Trollop.options(EvmRakeHelper.extract_command_options) do

carbonin · 2017-03-22T15:15:09Z

lib/tasks/evm_import_export_alerts.rake

+require_relative 'evm_import_export_illegal_chars'
+require_relative 'evm_import_export_options'
+
+class EvmImportExportAlerts


These classes should probably live in lib/task_helpers.

branic · 2017-03-23T06:52:03Z

@Fryguy, @carbonin,

I've refactored this PR to break out the classes and functions. I also implemented Trollop for processing the arguments and removed positional arguments (for example the destination directory for an export has to be specified as an option.

carbonin

A few of these comments apply to multiple places. I want to make suggestions about deduplicating some logic, but I'm not sure what will be common to all of the objects that you want to eventually add so I'll hold off for now.

carbonin · 2017-03-23T12:59:14Z

lib/task_helpers/imports/alerts.rb

+module TaskHelpers
+  class Imports
+    class Alerts
+      class ParsedNonDialogYamlError < StandardError; end


Where does this get used? I don't see it in any of these files. If it is used it should live in one place rather than in every class.

This class is defined in all of the rhconsulting rake scripts, but only used in one of them. I'll remove this class as it is not used.

carbonin · 2017-03-23T13:02:36Z

lib/task_helpers/exports.rb

+      Trollop.die :directory, 'Destination directory must already exist' unless File.directory?(options[:directory])
+      Trollop.die :directory, 'Destination directory must be writeable' unless File.writable?(options[:directory])
+
+      options.delete_if { |_k, v| v.nil? || v == false }


I'm not sure why this is needed.

I don't think it is necessary either. It came as part of the copying code from https://github.com/ManageIQ/manageiq/blob/22efb0914107963b95e064c9d7adb2aa6d905e2a/lib/tasks/evm_dba.rake for Trollop.

carbonin · 2017-03-23T13:07:22Z

lib/task_helpers/exports/alert_sets.rb

+
+      private
+
+      def export_alert_sets(options)


This doesn't need to be a separate method if the options are getting passed right through from #export.

Agreed. I'll change it.

carbonin · 2017-03-23T13:12:44Z

lib/task_helpers/imports/alert_sets.rb

+            alertsets = YAML.load_file(fname)
+            import_alert_sets(alertsets)
+          end
+        end


Dir.glob will work if given a file name, so this can be simplified:

glob = File.file?(options[:source]) ? options[:source] : "#{options[:source]}/*.yaml" Dir.glob(glob) do |fname| alertsets = YAML.load_file(fname) import_alert_sets(alertsets) end

Makes sense. I'll change to the simplified version.

carbonin · 2017-03-23T13:14:47Z

lib/task_helpers/exports.rb

+    # @param [Hash] options If options[:keep_spaces] == true, then keep spaces in
+    # \p str , otherwise replace spaces. (default = false, replace spaces)
+    def self.replace_chars(str, options = {})
+      if options.key?(:keep_spaces) && options[:keep_spaces] == true


A hash will return nil for a key that doesn't exist, so you don't need this check.

You can just check if options[:keep_spaces]

Changed this as well.

carbonin · 2017-03-23T14:27:43Z

lib/task_helpers/exports.rb

+    # @param [String] str string to replace
+    # @param [Hash] options If options[:keep_spaces] == true, then keep spaces in
+    # \p str , otherwise replace spaces. (default = false, replace spaces)
+    def self.replace_chars(str, options = {})


I feel like if this had a better name we wouldn't need the comment.

Maybe something like safe_filename?

Also it would document itself better if we passed the option specifically rather than passing the whole options hash for one value. Then the method would look like def self.safe_filename(filename, keep_spaces = false) which is a lot more clear.

Agreed. I'll change the method name and definition.

branic · 2017-03-23T18:57:25Z

@carbonin I've made the changes you requested. I've also adjusted the error handling and added some error handling when performing the imports.

carbonin · 2017-03-24T12:44:24Z

lib/task_helpers/exports.rb

+      options
+    end
+
+    def self.validate_directory(directory)


I think all of this should be done right after parsing the options.

Also this might work better if it just returned the string then you could do something like:

error = validate_directory(options[:directory]) Trollop.die :directory, error if error

Then you don't have to write to standard out and you also don't need to define the exceptions.

I've changed this as suggested.

carbonin · 2017-03-24T12:44:56Z

lib/task_helpers/exports.rb

+    end
+
+    def self.validate_directory(directory)
+      unless directory


I don't think this check is necessary because directory is a required argument.

branic · 2017-04-05T16:53:31Z

@carbonin @Fryguy

Are there other changes you would like to see with this PR?

The travis-cli failure was caused by a larger issue with travis when I pushed the last commit. I'm not sure how to clear the failure.

carbonin

I think the only other thing would be some specs. It would be great to see tests at least for the Exports.safe_filename and the validate_* methods.

Ideally we would have tests around the whole thing with simple data files to import from and export to. Then we would compare the output against the "right" answer ...

branic · 2017-04-06T23:55:42Z

@carbonin

I've added tests for the safe_filename method, but I could use some guidance on how to handle the tests for the validate_* methods if you have any ideas or can point me to some sample code.

Also, I had to rebase my branch to be able to get a working environment which looks like it messed up the commit logs and such. I'm not sure how to fix this or what I need to do.

As always any help and feedback is appreciated.

carbonin · 2017-04-07T20:35:48Z

spec/lib/task_helpers/exports_spec.rb

@@ -0,0 +1,44 @@
+describe '#safe_filename' do


The top level block here should be:

describe TaskHelpers::Exports do ... end

Ok. Changed.

carbonin · 2017-04-07T20:36:09Z

lib/task_helpers/exports.rb

+module TaskHelpers
+  class Exports
+    def self.new_filename(filename, keep_spaces = false)
+      new_filename = filename


It looks like you changed the method name and the variable name here @branic

Also, this particular line isn't needed because String#gsub returns a copy.

Oops I did change both trying to be too efficient with search and replace.

I had that line because the gsub returned nil if keep_spaces was true. I've changed the code to eliminate this assignment by using a ternary operator.

carbonin · 2017-04-07T20:40:28Z

lib/task_helpers/exports.rb

+      new_filename = filename
+      new_filename = new_filename.gsub(%r{[ ]}, '_') unless keep_spaces
+      new_filename = new_filename.gsub(%r{[/]}, 'slash')
+      new_filename.gsub(%r{[|]}, 'pipe')


I think you should be able to combine these by using a hash as the second parameter (see https://ruby-doc.org/core-2.3.3/String.html#method-i-gsub)

I missed that when I read the doc page the first time. I've changed to use a hash.

carbonin · 2017-04-07T20:40:44Z

lib/task_helpers/exports/alert_sets.rb

+        export_dir = options[:directory]
+
+        MiqAlertSet.order(:id).all.each do |a|
+          # Replace characters in the description that are not allowed in filenames


I don't think we need this comment

I've removed the comment.

carbonin · 2017-04-07T20:41:23Z

lib/tasks/evm_export_import.rake

+    task :alertprofiles => :environment do
+      options = TaskHelpers::Imports.parse_options
+      TaskHelpers::Imports::AlertSets.new.import(options)
+    end


Does this task need the exit also?

You are correct. I've added the missing line.

carbonin

Just one minor nit with the specs.

I'm okay with this. I would really like the main import/export methods to also have specs, but that may be asking a bit much for the first pass.

I'll leave the final determination to @Fryguy though.

carbonin · 2017-04-10T17:35:52Z

spec/lib/task_helpers/exports_spec.rb

@@ -0,0 +1,44 @@
+describe TaskHelpers::Exports do
+  context 'with filename' do


This should probably be a describe ".safe_filename" do

The convention in rspec is to describe the class/module under test then describe each method you are testing then use it ".." do blocks for each test case on a particular method.

Then context can be used for shared setup between it or describe blocks.

That makes sense. I'll add another describe block for the method.

branic · 2017-04-10T19:42:16Z

I'd like to be able to have tests for the import/export methods (and maybe even the rake tasks), but I'm not sure how to setup the tests so I need to do some more research in how to create a file to import and write and test the export within RSpec.

branic · 2017-04-10T20:43:06Z

I've thought about using FakeFS [1] to be able to temporarily create the files for the RSpec tests, but the gem isn't included in ManageIQ and I don't know if a gem should be brought in just for running tests.

[1] https://github.com/fakefs/fakefs

Fryguy · 2017-04-17T20:09:42Z

I'm ok with a gem that's just for tests. Just be sure to put it into the test group in the Gemfile.

carbonin

Looks great! Just one other comment.

carbonin · 2017-05-25T14:03:51Z

spec/lib/task_helpers/imports/alert_sets_spec.rb

@@ -0,0 +1,54 @@
+describe 'TaskHelpers::Imports::AlertSets' do


This should be a constant, not a string.

Ugh, I thought i got all of those. It's fixed now.

carbonin

Looks great @branic. Will merge when it goes green 😄

miq-bot · 2017-05-25T18:37:12Z

Some comments on commits https://github.com/branic/manageiq/compare/1c2d26a1930bfa362d928bcb0a9d09c60cf5a976~...042108573513b66e81121cc09e71febffbafa885

lib/task_helpers/imports/alert_sets.rb

⚠️ - 11 - Detected puts. Remove all debugging statements.

lib/task_helpers/imports/alerts.rb

⚠️ - 11 - Detected puts. Remove all debugging statements.

miq-bot · 2017-05-25T18:37:59Z

Checked commits https://github.com/branic/manageiq/compare/1c2d26a1930bfa362d928bcb0a9d09c60cf5a976~...042108573513b66e81121cc09e71febffbafa885 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
13 files checked, 0 offenses detected
Everything looks fine. 🍰

gmcculloug · 2017-06-09T20:26:12Z

@branic Can you modify the description of this PR to match the object type that is supported with the final version of the PR.

I would also suggest creating a git issue with a check list to track all the of the types so we can link to the PRs and check them off as we go.

branic · 2017-06-09T21:06:11Z

@gmcculloug I've updated the description as well as opened an issue to track all of the object types.

branic mentioned this pull request Mar 1, 2017

Add shell scripts to export/import alerts and alert profiles ManageIQ/manageiq-appliance#112

Closed

chessbyte assigned gmcculloug Mar 1, 2017

chessbyte requested a review from Fryguy March 1, 2017 22:41

Fryguy added enhancement core labels Mar 2, 2017

branic changed the title ~~Add rake scripts to export/import miq objects~~ Add alert rake script to export/import miq objects Mar 11, 2017

branic changed the title ~~Add alert rake script to export/import miq objects~~ Add rake script to export/import miq alerts and alert profiles Mar 11, 2017

carbonin reviewed Mar 15, 2017

View reviewed changes

carbonin reviewed Mar 22, 2017

View reviewed changes

carbonin suggested changes Mar 23, 2017

View reviewed changes

carbonin reviewed Mar 24, 2017

View reviewed changes

carbonin reviewed Apr 5, 2017

View reviewed changes

carbonin closed this Apr 5, 2017

carbonin reopened this Apr 5, 2017

carbonin reviewed Apr 7, 2017

View reviewed changes

carbonin approved these changes Apr 10, 2017

View reviewed changes

Fryguy assigned chessbyte and unassigned gmcculloug Apr 17, 2017

branic and others added 10 commits May 24, 2017 09:32

fix rubocop offenses

9b1db01

Remove unnecessary rake task specs

f36b246

Correct class name in imports_spec

2177dcb

Use FactoryGirl to create alerts to test exports

12404df

Assert on database contents for imported alerts

7a8a63d

Use FactoryGirl to create alertsets to test exports

d56d040

Assert on database contents for imported alertsets

122fa6c

Remove unneeded context and rework missing directory test

4042831

don't disable rubocop warning

9a8234f

Fix rspec test failure

ce0042f

carbonin reviewed May 25, 2017

View reviewed changes

branic and others added 2 commits May 25, 2017 11:27

change describe to constant

ac914a3

Fixup rubocop style warnings

0421085

carbonin approved these changes May 25, 2017

View reviewed changes

carbonin added this to the Sprint 62 Ending Jun 5, 2017 milestone May 25, 2017

carbonin merged commit cb5f57d into ManageIQ:master May 25, 2017

branic deleted the rhconsulting_scripts branch May 25, 2017 21:04

branic mentioned this pull request Jun 8, 2017

Add rake script to export/import policies and policy profiles #15256

Merged

branic mentioned this pull request Jun 9, 2017

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

Open

18 tasks

branic mentioned this pull request Dec 22, 2017

Add rake script to export/import roles #16717

Merged

branic mentioned this pull request Feb 9, 2018

Add support for exporting and importing tags #16983

Merged

branic mentioned this pull request Apr 2, 2018

Add support for exporting an importing service dialogs #17241

Merged

branic mentioned this pull request Jul 22, 2018

Add support for exporting and importing provision dialogs #17739

Merged

branic mentioned this pull request Aug 17, 2018

Add support for exporting and importing customization templates #17877

Merged

branic mentioned this pull request Dec 12, 2018

Add support for exporting and importing reports #18284

Merged

branic mentioned this pull request Jan 3, 2019

Add support for exporting and importing widgets #18327

Merged

		@@ -0,0 +1,44 @@
		describe TaskHelpers::Exports do
		context 'with filename' do

		@@ -0,0 +1,54 @@
		describe 'TaskHelpers::Imports::AlertSets' do

Add rake script to export/import miq alerts and alert profiles #14126

Add rake script to export/import miq alerts and alert profiles #14126

Conversation

branic commented Mar 1, 2017 • edited Loading

Fryguy commented Mar 2, 2017

branic commented Mar 2, 2017

Fryguy commented Mar 2, 2017

Fryguy commented Mar 2, 2017

branic commented Mar 11, 2017

carbonin 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

branic commented Mar 23, 2017

carbonin 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

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

branic commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branic commented Apr 5, 2017

carbonin left a comment

Choose a reason for hiding this comment

branic commented Apr 6, 2017

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

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

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branic commented Apr 10, 2017

branic commented Apr 10, 2017

Fryguy commented Apr 17, 2017

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonin left a comment

Choose a reason for hiding this comment

miq-bot commented May 25, 2017

miq-bot commented May 25, 2017

gmcculloug commented Jun 9, 2017

branic commented Jun 9, 2017

branic commented Mar 1, 2017 •

edited

Loading