Fixes #33089 - move (hammer_)plugin_prefix to globals #974
Conversation
|
@ekohl I guess moving |
| 'Linux': { | ||
| case $facts['os']['name'] { | ||
| 'Amazon': { |
There was a problem hiding this comment.
I just checked and it looks like Amazon Linux is still qualified in the Red Hat OS family. At some point we should clean this up.
There was a problem hiding this comment.
Not me, not today, not enough 💥 ;)
| @@ -9,7 +9,7 @@ | |||
| # $version:: The package version to ensure | |||
| # | |||
| define foreman::cli::plugin ( | |||
| String $package = "${foreman::cli::hammer_plugin_prefix}${title}", | |||
| String $package = "${foreman::cli::params::hammer_plugin_prefix}${title}", | |||
There was a problem hiding this comment.
Technically this doesn't have to change due to inheritance.
There was a problem hiding this comment.
Maybe. I was blindly copying from theforeman/puppet-foreman_proxy@935d45a ;)
There was a problem hiding this comment.
Honestly, if I'd read foreman::cli::hammer_plugin_prefix here, and wouldn't find it in cli.pp, I'd be confused at first, so I think explicitly using the one from params seems friendlier to the reader to me
There was a problem hiding this comment.
I've seen others confused by that as well so I guess it's ok.
| @@ -93,27 +93,27 @@ | |||
| # We use system packages except on EL7 | |||
| if versioncmp($facts['os']['release']['major'], '8') >= 0 { | |||
| $passenger_ruby_package = undef | |||
| $plugin_prefix = 'rubygem-foreman_' | |||
| $_plugin_prefix = 'rubygem-foreman_' | |||
There was a problem hiding this comment.
I think for both EL7 & EL8 we can nowadays use foreman-plugin- as a prefix. Puppet should support Provides and it would simplify matters. For example, the puppetdb plugin also follows the same convention (tasks appears to be foreman-plugin-foreman-tasks). Thoughts?
There was a problem hiding this comment.
Would surely make a great follow up PR? ;)
There was a problem hiding this comment.
But yes, puppet 5.5 already has the virtual_packages feature in yum and dnf providers, so it's probably safe to switch to that.
There was a problem hiding this comment.
An noting it down explicitly: we don't have a similar provides for Hammer plugins. We should, but that means we can't immediately depend on it.
There was a problem hiding this comment.
If we'd like to do that in the future, can an issue be filed to track it?
There was a problem hiding this comment.
Thanks! Should we file issues for the Foreman Proxy equivalents too while we're at it?
There was a problem hiding this comment.
Yeah, sounds good. I just didn't think of it as it's not part of this PR
There was a problem hiding this comment.
Shall I create them or will you? Just so we don't end up with duplicates.
There was a problem hiding this comment.
You. I'm away from the pc
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm ok with this. Let's move other variables in separate PRs.
@ehelms @wbclark please also have a look.
As for other variables:
version&plugin_version- I can still see a use case so I'm not entirely sure.user&group- these are hardly ever changed so I'd be in favor of moving themrails_env&app_rootsamevhost_prioritysame (perhaps even move it toapache.ppand only allow changing it via Hiera)
|
Thanks! I've also opened https://projects.theforeman.org/issues/33106 to track the extraction of the other variables |
No description provided.