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

allow seeding of dialogs from plugins #14668

Merged
merged 4 commits into from
Apr 11, 2017
Merged

Conversation

durandom
Copy link
Member

@durandom durandom commented Apr 6, 2017

follows the same approach as in #14653

@miq-bot assign @bdunne
@miq-bot add_label enhancement, pluggable providers

are there many more file seeds of that kind that should be allowed to be seeded from plugins?
if so, we might group the seeding somewhere under Vmdb::Plugins - like you did with automate_domain.

I'm just not sure, if we can hook nicely into the seeding process. Also, to have classes seeded only in the class (and not from an external location like plugins) has its upsides too

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 Code looks good, would you mind adding a test?

@durandom
Copy link
Member Author

durandom commented Apr 6, 2017

oh, those tests.... I'll add some just for you 🌹 :)

@durandom
Copy link
Member Author

durandom commented Apr 7, 2017

@bdunne there you go. I had to refactor it a bit for the better and for better testability.

I could also remove the with_constant homegrown with http://www.relishapp.com/rspec/rspec-mocks/v/3-5/docs/mutating-constants/stub-defined-constant

expect(dialog_import_service).to receive(:import_all_service_dialogs_from_yaml_file).with(
Rails.root.join(test_file_path, "seed_test.yaml").to_path
)
described_class.seed
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 this should also expect(Dir).to receive(:glob).with(the path for the engine dialogs). The expect above it true, but it's not related to the plugin part of the code.

@durandom
Copy link
Member Author

@bdunne I set DIALOG_DIR_CORE to 'some-non-existent-dir' which rules out the one expect being triggered from the core seeding.
Also expect on mock_engine receiving root

@durandom
Copy link
Member Author

I could not resist and changed with_constants to stub_const

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 Changes look good, can you take care of the rubocop indentation comments?

@durandom
Copy link
Member Author

👍 Changes look good, can you take care of the rubocop indentation comments?

doh, what happened to my pre commit hook...

@@ -1,8 +1,9 @@
class Dialog < ApplicationRecord
DIALOG_DIR = Rails.root.join("product/dialogs/service_dialogs")
DIALOG_DIR_CORE = 'product/dialogs/service_dialogs'.freeze
DIALOG_DIR_PLUGIN = 'content/service_dialogs'.freeze
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should just create a content/service_dialogs directory in manageiq proper, so the directories are consistent.

Copy link
Member

@bdunne bdunne Apr 11, 2017

Choose a reason for hiding this comment

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

We can drop the DIALOG_DIR_CORE logic once all dialogs have been moved to provider pugins or manageiq-content

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean move all from product/dialogs/service_dialogs to content/service_dialogs?

@durandom
Copy link
Member Author

addressed all rubocop issues

@miq-bot
Copy link
Member

miq-bot commented Apr 11, 2017

Checked commits durandom/manageiq@8b28480~...833f76d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

@durandom
Copy link
Member Author

I assume this is good to go?

@bdunne bdunne merged commit 03239d7 into ManageIQ:master Apr 11, 2017
@bdunne bdunne added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 11, 2017
@bdunne bdunne added the fine/no label Apr 11, 2017
@durandom durandom mentioned this pull request Apr 21, 2017
38 tasks
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.

4 participants