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
Refs #34385 - use Foreman::Plugin.installed?
instead of defined?
#526
Conversation
lib/foreman_openscap/engine.rb
Outdated
@@ -1,6 +1,6 @@ | |||
module ForemanOpenscap | |||
def self.with_katello? | |||
defined?(::Katello) | |||
Foreman::Plugin.installed?("Katello") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plugin really named 'Katello'? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's katello
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just katello
lowered case though :O
test/test_plugin_helper.rb
Outdated
@@ -5,14 +5,14 @@ | |||
FactoryBot.definition_file_paths << File.join(File.dirname(__FILE__), 'factories') | |||
# Add factories from foreman_ansible | |||
FactoryBot.definition_file_paths << File.join(ForemanAnsible::Engine.root, '/test/factories') | |||
FactoryBot.definition_file_paths << File.join(ForemanPuppet::Engine.root, '/test/factories') if defined?(ForemanPuppet) | |||
FactoryBot.definition_file_paths << File.join(ForemanPuppet::Engine.root, '/test/factories') if Foreman::Plugin.installed?("foreman_puppet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is quite soon in the init proces and just for tests, I'd go with defined?(ForemanPuppet::Engine
)` here, but probably both will work, so I'm fine both ways ^_^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@lzap @adiabramovitch could you take a look? |
@@ -5,14 +5,14 @@ | |||
FactoryBot.definition_file_paths << File.join(File.dirname(__FILE__), 'factories') | |||
# Add factories from foreman_ansible | |||
FactoryBot.definition_file_paths << File.join(ForemanAnsible::Engine.root, '/test/factories') | |||
FactoryBot.definition_file_paths << File.join(ForemanPuppet::Engine.root, '/test/factories') if defined?(ForemanPuppet) | |||
FactoryBot.definition_file_paths << File.join(ForemanPuppet::Engine.root, '/test/factories') if defined?(ForemanPuppet::Engine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, Why are these changes needed if you stayed with the defined?
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @ezr-ondrej mentioned: #526 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then LGTM :)
Thanks @Ron-Lavi and all the reviewers, merging! |
No description provided.