Add support for exporting and importing tags #16983
Conversation
lib/task_helpers/exports/tags.rb
Outdated
| class Exports | ||
| class Tags | ||
| # Description attribute of Tag Categories that are not visible in the UI | ||
| SPECIAL_TAGS = ['Parent Folder Path (VMs & Templates)', 'Parent Folder Path (Hosts & Clusters)', 'User roles'].freeze |
There was a problem hiding this comment.
Where do these strings come from? Could we "accidentally" change them in message catalogs or fixtures can cause you problems here?
There was a problem hiding this comment.
@jrafanie These category descriptions were determined by exporting all of the tag classifications and finding ones that are not shown in the UI (Configuration -> Settings -> ManageIQ Region -> Tags -> My Company Categories.
You are right that the descriptions could change, but when I looked at the Classification I was not able to find anything that I could use to identify them. I'm also not sure how the UI decides to not show them when displaying the tag categories. I know that they are used and maintained internally by MIQ so the goal is to prevent a user from modifying them and causing lager problems.
From what I have been able to determine the two "Parent Folder Path ..." categories are not created until an infrastructure (or maybe cloud) provider has been added and the "User roles" category is created when setting up a region.
If the description did change the only effect would be that they would be exported and could be imported.
I'm open to handling this check another way that is more robust and not dependant on the name or description of the category. I just need a little help in what to look at to make the determination.
There was a problem hiding this comment.
You should be able to do this instead -
SPECIAL_TAGS = ["/managed/folder_path_yellow", "/managed/folder_path_blue", "/managed/user/role"]
Classification.includes(:tag).where(:parent_id => 0).where.not(:tags => {:name => SPECIAL_TAGS})
| cat.entries.each do |entry| | ||
| unless entry.default | ||
| export_tags << cat | ||
| break |
There was a problem hiding this comment.
I'm not following why we break out after the first non-default entry...
There was a problem hiding this comment.
This part is hard to follow and is probably why I usually forget how classifications work. Classifications containing both categories and entries and both could be default is confusing to me. As long as we have tests for this, I guess it's ok.
There was a problem hiding this comment.
The goal with this code is to not export default tag categories unless a tag category contains an entry that is not default.
I break out of the loop once one non-default entry is found because at that point the category and all entries will be exported so there is no need to continue looping through the entries.
The test "exports user tags to a given directory" does cover checking this case, but does not check the contents of the exported file. I'll add an expect to ensure that the exported files contents are as expected.
lib/task_helpers/imports/tags.rb
Outdated
| class ClassificationNameError < StandardError; end | ||
| class ClassificationEntryDescError < StandardError; end | ||
| class ClassificationEntryNameError < StandardError; end | ||
| class ClassificationYamlError < StandardError |
There was a problem hiding this comment.
I don't think these exceptions are needed... see below..
lib/task_helpers/imports/tags.rb
Outdated
| begin | ||
| tag_categories = YAML.load_file(fname) | ||
| import_tags(tag_categories) | ||
| rescue ClassificationDescError |
There was a problem hiding this comment.
I don't think we need to rescue all of these exceptions... see below
lib/task_helpers/imports/tags.rb
Outdated
| private | ||
|
|
||
| # Description attribute of Tag Categories that are not visible in the UI | ||
| SPECIAL_TAGS = ['Parent Folder Path (VMs & Templates)', 'Parent Folder Path (Hosts & Clusters)', 'User roles'].freeze |
There was a problem hiding this comment.
Again, hardcoded strings that we might accidentally change elsewhere that might cause problems in this script. I can't tell.
There was a problem hiding this comment.
See comment on the same issue for the export script above. The same applies.
lib/task_helpers/imports/tags.rb
Outdated
|
|
||
| def import_classification(tag_category) | ||
| raise ClassificationDescError unless tag_category['description'] | ||
| raise ClassificationNameError unless tag_category['name'] |
There was a problem hiding this comment.
I don't think we need to raise exceptions here. We already have validations on classification. I believe we should be able to do
c = Classification.new(...) # passing the values we want to import from each classification
unless c.valid?
# inspect c.errors to see what failed validation
end
I don't know if we're testing more or less things here vs. what's in the existing validations or if there's a reason we're skipping the activerecord validations and doing them manually here.
There was a problem hiding this comment.
Of course, you might have to treat "categories" different than "entries" but because they're all classifications in the end, they all must pass the existing validations so we should only have to add any additional validations you care about. In other words, we shouldn't need to check name and description above, it should invalid and activerecord will have why it's invalid.
There was a problem hiding this comment.
These exceptions are here to prevent a user from trying to import a file that they have modified and removed the name or description. The
classification = Classification.find_by_name(tag_category['name'])
will generate a TypeError if tag_category['name'] is nil. Having these checks here I think simplifies the error handling later in the script. The import will never succeed if either of these is nil so why even try. There is a check if errors were generated
raise ClassificationYamlError.new("Tag Category error", classification.errors.details) if classification.errors.count.positive?
to handle situations where the user changed a value to something illegal (like having a capital letter in the name).
I don't mind changing the error handling to using the c.valid? if that would be a better way to handle the error checking.
(In a quick test I found I can get around the TypeError in the Classification.find_by_name by using to_s on the tag_category['name'] so maybe that would be better and then just handle the error of the classification not being created because the name is nil)
There was a problem hiding this comment.
You can do something like this if the new classification object is not valid -
warn("Error importing #{c.errors.full_messages.inspect") unless c.valid?
That'll output all the invalid attributes at one time which would eliminate the need for a user to fix them one by one. This way you're allowing the validations in the model to handle it. See if that works for you and then you can get rid of all of the special exception classes you created.
There was a problem hiding this comment.
I'm going to leave the check for tag_category['name'] being nil because creating a Classification with name being nil causes an explosion. Having description nil or name being an invalid string (e.g. inValid) is handled by the create and the error is retuned as expected so I can get rid of those error classes.
irb(main):085:0> a
=> {"description"=>nil, "icon"=>nil, "read_only"=>false, "syntax"=>"string", "single_value"=>true, "example_text"=>"Brant Test", "parent_id"=>0, "show"=>true, "default"=>nil, "perf_by_tag"=>false, "name"=>nil}
irb(main):086:0> classification = Classification.create(a)
NoMethodError: undefined method `name' for nil:NilClass
from /home/bevans/my-repos/manageiq/app/models/classification.rb:326:in `name'
from /home/bevans/.gem/ruby/2.3.5/gems/activemodel-5.0.6/lib/active_model/validator.rb:149:in `block in validate'
from /home/bevans/.gem/ruby/2.3.5/gems/activemodel-5.0.6/lib/active_model/validator.rb:148:in `each'
from /home/bevans/.gem/ruby/2.3.5/gems/activemodel-5.0.6/lib/active_model/validator.rb:148:in `validate'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:405:in `public_send'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:405:in `block in make_lambda'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:169:in `block (2 levels) in halting'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:547:in `block (2 levels) in default_terminator'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:546:in `catch'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:546:in `block in default_terminator'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:170:in `block in halting'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:454:in `block in call'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:454:in `each'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:454:in `call'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:101:in `__run_callbacks__'
from /home/bevans/.gem/ruby/2.3.5/gems/activesupport-5.0.6/lib/active_support/callbacks.rb:750:in `_run_validate_callbacks'
... 17 levels...
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `transaction'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/transactions.rb:211:in `transaction'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/transactions.rb:392:in `with_transaction_returning_status'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/transactions.rb:319:in `block in save'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/transactions.rb:334:in `rollback_active_record_state!'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/transactions.rb:318:in `save'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/suppressor.rb:41:in `save'
from /home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/persistence.rb:34:in `create'
from (irb):86
from /home/bevans/.gem/ruby/2.3.5/gems/railties-5.0.6/lib/rails/commands/console.rb:65:in `start'
from /home/bevans/.gem/ruby/2.3.5/gems/railties-5.0.6/lib/rails/commands/console_helper.rb:9:in `start'
from /home/bevans/.gem/ruby/2.3.5/gems/railties-5.0.6/lib/rails/commands/commands_tasks.rb:78:in `console'
from /home/bevans/.gem/ruby/2.3.5/gems/railties-5.0.6/lib/rails/commands/commands_tasks.rb:49:in `run_command!'
from /home/bevans/.gem/ruby/2.3.5/gems/railties-5.0.6/lib/rails/commands.rb:18:in `<top (required)>'
from bin/rails:4:in `require'
from bin/rails:4:in `<main>'
There was a problem hiding this comment.
I've found a better way to handle a name being nil during import. I'm going to do a to_s before the import for the name.
lib/task_helpers/exports/tags.rb
Outdated
| class Exports | ||
| class Tags | ||
| # Description attribute of Tag Categories that are not visible in the UI | ||
| SPECIAL_TAGS = ['Parent Folder Path (VMs & Templates)', 'Parent Folder Path (Hosts & Clusters)', 'User roles'].freeze |
There was a problem hiding this comment.
You should be able to do this instead -
SPECIAL_TAGS = ["/managed/folder_path_yellow", "/managed/folder_path_blue", "/managed/user/role"]
Classification.includes(:tag).where(:parent_id => 0).where.not(:tags => {:name => SPECIAL_TAGS})| cat.entries.each do |entry| | ||
| unless entry.default | ||
| export_tags << cat | ||
| break |
There was a problem hiding this comment.
Not sure why there's a break here. Don't you need to check the rest of the entries to get any others that are not "default"?
There was a problem hiding this comment.
Once one non-defualt entry is found the category will be exported which will include all entries.
I could adjust the export to only ever export non-default entries, but it is possible to change the name and description so I felt it was best to export all entries if one non-default was found. Granted there is an edge case that isn't covered where a name or description of a default is changed and no entries are added. In that case the category will never be exported.
This logic is a little different than the script in rhtconsulting which exports all but the "special" categories.
lib/task_helpers/imports/tags.rb
Outdated
| private | ||
|
|
||
| # Description attribute of Tag Categories that are not visible in the UI | ||
| SPECIAL_TAGS = ['Parent Folder Path (VMs & Templates)', 'Parent Folder Path (Hosts & Clusters)', 'User roles'].freeze |
lib/task_helpers/imports/tags.rb
Outdated
|
|
||
| def import_classification(tag_category) | ||
| raise ClassificationDescError unless tag_category['description'] | ||
| raise ClassificationNameError unless tag_category['name'] |
There was a problem hiding this comment.
You can do something like this if the new classification object is not valid -
warn("Error importing #{c.errors.full_messages.inspect") unless c.valid?
That'll output all the invalid attributes at one time which would eliminate the need for a user to fix them one by one. This way you're allowing the validations in the model to handle it. See if that works for you and then you can get rid of all of the special exception classes you created.
…ring, simplify error handling and rspec tests
lib/task_helpers/imports/tags.rb
Outdated
| classification.update_attributes!(tag_category.select { |k| UPDATE_FIELDS.include?(k) }) | ||
| else | ||
| classification = Classification.create(tag_category) | ||
| raise ClassificationYamlError.new("Tag Category error", classification.errors.full_messages) unless classification.valid? |
There was a problem hiding this comment.
I've found a better way to handle a name being nil during import. I'm going to do a to_s before the import for the name.
I still think we can remove the checking for nil name altogether because we already have a validation for presence. We also validate :validate_format_of_name, which does additional checks. If you try to .update_attributes or .create (without the !), you should be able to fetch the validation errors like you're doing here and re-raise your own exception if you want, or you can just call the ! methods and see if the raised validation failures are good enough for you. Finally, you could also just call Classification.new and try to set the various values and call classification.valid? to check if it's valid.
There was a problem hiding this comment.
The only reason to have your own validation in these scripts is if you want the export/import to have stricter rules about what is imported/exported or add logging of specific types of data. If the model already has the validation you are trying to use, we should have a single place define that validation so if we change the model validation due to a bug, this script automatically gets fixed.
There was a problem hiding this comment.
I'm not sure I'm completely following. Are you saying to call Classification.new regardless of if the Classification already exists or not?
I've removed all of the additional checks I was doing and letting the model handle that. I do have to make sure that the name attribute for a Classification or Entry is a string. If it is nil from reading in the YAML file then the find_by_name and find_entry_by_name (lines 53 & 71) calls cause the rake task to be aborted with a TypeError: no implicit conversion of nil into String. So by making sure name is a string I can just depend on the models validations.
I see what you mean with the update_attributes so I'll make a change to handle errors when updating the attributes.
I left the one error class ClassificationYamlError so that I could output a bit nicer format of the validation errors for the user.
|
Checked commits https://github.com/branic/manageiq/compare/0c1b181244a4e8cc9481a980fa258334d7ca6920~...ad3a957af6f73345791508cb116286bb6037ce09 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 lib/task_helpers/imports/tags.rb
spec/lib/task_helpers/imports/tags_spec.rb
|
|
@gtanzillo @jrafanie Anything else you would like me to address? |
|
@gtanzillo @jrafanie Can this PR be merged or is there additional changes that you would like made? |
|
@branic I'm ok with the changes. As an aside, I have no idea what in this area would ever need to be backported to gaprindashvili, so keep that in mind when working on these tools. I'll assume no to backporting unless you say so. Also, do all of these export/import PRs support being run with inter-region (global exported/imported into region or vice versa). I'm curious what is the use case for either
|
|
@jrafanie There shouldn't be anything preventing these from being backported to Gaprindashvili, but I wasn't planning on requesting them to be backported. These scripts are meant to be able to move the pieces between regions. So for example to go from the development region to the production region. There is some use for some of the scripts going between global and region, but for the most part the OOTB replication will handle anything that needs to be made available in the global from the regional. |
These rake scripts and classes provide functionality for exporting/importing of the following ManageIQ object types:
This PR uses the framework that was implemented for PRs #14126, #15256, #16717 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.
Steps for Testing/QA
Exporting
Importing