Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rake script to export/import roles #16717

Merged
merged 8 commits into from
Jan 30, 2018

Conversation

branic
Copy link
Contributor

@branic branic commented Dec 22, 2017

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

  • roles (MiqUserRole)

This PR uses the framework that was implemented for PRs #14126 and #15256 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.

@gmcculloug
Copy link
Member

Moving over to @gtanzillo to review as this is closer to core. Maybe @carbonin can help out here as well, he reviewed a previous import/export rake task PR.

@branic
Copy link
Contributor Author

branic commented Jan 8, 2018

@gtanzillo @carbonin I'm not sure why the travis-ci build is failing. Any ideas or pointers? The failures aren't in any of the files I've made a change to.

let(:role_one_name) { 'Role Import Test' }
let(:role_two_name) { 'Role Import Test 2' }

EvmSpecHelper.seed_specific_product_features(%w(
Copy link
Member

@carbonin carbonin Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be willing to bet that this is causing your spec failures. This should be in a before block.

@branic
Copy link
Contributor Author

branic commented Jan 8, 2018

Thanks @carbonin that fixed the travis-ci errors.

@branic
Copy link
Contributor Author

branic commented Jan 25, 2018

@gtanzillo @carbonin I know you guys are busy, but can one of you review this PR.

Thanks,
Brant

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments.

Exports.exclude_attributes(role.attributes, %w(created_at updated_at id)).merge('feature_identifiers' => role.feature_identifiers.sort)
end

roles.compact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need #compact! here. Or you could add #compact after the end above.

As is, this line doesn't do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I've moved the #compact to after the end

begin
roles = YAML.load_file(fname)
import_roles(roles)
rescue => e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there more specific exception we can rescue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what all of the exceptions are that could be raised so I used the more generic to catch any error with the import.

end

after do
# FileUtils.remove_entry export_dir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this or can we just remove this block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed so that when the test runs the directory that is created is removed. I had commented it out when I was testing the spec files and forgot to uncomment it before committing. I've uncommented the line.


def import_roles(roles)
roles.each do |r|
r['miq_product_feature_ids'] = MiqProductFeature.all.collect do |f|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to pull the MiqProductFeature.all out of the loop and put it in a var so that it doesn't get called for every iteration.

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have the one suggestion that I commented on. The rest looks good to me 👍

def import(options = {})
return unless options[:source]

available_features = MiqProductFeature.all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@branic This is good but I think you should move it into the method that needs it instead of passing it as an argument. You can just put it right above line 23 below, just above the roles.each do |r| line.

@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2018

Checked commits https://github.com/branic/manageiq/compare/5c84eaf7836b38795e1cf01f6c4ec24b0eb89b70~...21b299af24b641d95a80d3abefd23a0e473a0d2a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. 🍰

@gtanzillo gtanzillo added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 30, 2018
@gtanzillo gtanzillo merged commit cf8fbfa into ManageIQ:master Jan 30, 2018
@branic branic deleted the roles_export_import branch January 30, 2018 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants