-
Notifications
You must be signed in to change notification settings - Fork 269
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 #32276: Add Katello hammer plugin #936
Conversation
@@ -26,7 +26,7 @@ class { 'foreman::cli': | |||
|
|||
it_behaves_like 'hammer' | |||
|
|||
['discovery', 'remote_execution', 'tasks', 'templates'].each do |plugin| | |||
['discovery', 'remote_execution', 'tasks', 'templates', 'katello'].each do |plugin| |
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.
expected Package "tfm-rubygem-hammer_cli_foreman_katello" to be installed
you'll need to fixup the logic below to not add the foreman_
prefix and add it to the plugins here, thanks 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.
You're missing the actual include foreman::cli::katello
. I would expect that it's not packaged for Debian so add it like ansible/azure. Right now we also don't have any explicit checks for those and assume that if Puppet worked correctly, it's good enough.
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.
Small coding style suggestions, but otherwise 👍
spec/classes/cli_plugins_spec.rb
Outdated
if plugin == 'azure' | ||
'foreman_azure_rm' | ||
elsif plugin == 'katello' | ||
'katello' | ||
else | ||
"foreman_#{plugin}" | ||
end |
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.
Feels like a case statement is more natural
if plugin == 'azure' | |
'foreman_azure_rm' | |
elsif plugin == 'katello' | |
'katello' | |
else | |
"foreman_#{plugin}" | |
end | |
case plugin | |
when 'azure' | |
'foreman_azure_rm' | |
when 'katello' | |
'katello' | |
else | |
"foreman_#{plugin}" | |
end |
|
||
describe package(package_name) do | ||
it { is_expected.to be_installed } | ||
package_name = fact('os.release.major') == '7' ? "tfm-rubygem-hammer_cli_katello" : "rubygem-hammer_cli_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.
package_name = fact('os.release.major') == '7' ? "tfm-rubygem-hammer_cli_katello" : "rubygem-hammer_cli_katello" | |
package_name = (fact('os.release.major') == '7' ? "tfm-" : "") + "rubygem-hammer_cli_katello" |
Or
package_name = fact('os.release.major') == '7' ? "tfm-rubygem-hammer_cli_katello" : "rubygem-hammer_cli_katello" | |
package_name = "#{fact('os.release.major') == '7' ? "tfm-" : ""}rubygem-hammer_cli_katello" |
Or, since it's already used elsewhere, define a regular variable that can also be used in the regular plugin test:
scl_prefix = fact('os.release.major') == '7' ? "tfm-" : ""
Some of the failures here are related to #937 |
Adding this here will allow adding to the foreman-installer similar to all other plugin hammer packages and then allows being able to drop these requirements from the katello meta RPM (https://github.com/theforeman/foreman-packaging/blob/rpm/develop/packages/katello/katello/katello.spec#L34-L36).