-
Notifications
You must be signed in to change notification settings - Fork 130
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
Improve support for Foreman_proxy AD integration #429
base: master
Are you sure you want to change the base?
Conversation
…= ad' Created a new 'Foreman_proxy::AdConfig' datatype Add test coverage.
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 like the general direction of this. I'll need to look into the kafo issue to prevent our installer from breaking.
manifests/init.pp
Outdated
Stdlib::Absolutepath $realm_keytab = $::foreman_proxy::params::realm_keytab, | ||
String $realm_principal = $::foreman_proxy::params::realm_principal, | ||
Optional[Foreman_proxy::AdConfig] $ad_config = $::foreman_proxy::params::ad_config, |
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.
We still have an issue in our installer with a Struct because of Kafo. theforeman/puppet-foreman#601 was limited by the same issue. I still need to look into this.
manifests/init.pp
Outdated
@@ -415,9 +417,10 @@ | |||
Enum['ipmitool', 'freeipmi', 'shell'] $bmc_default_provider = $::foreman_proxy::params::bmc_default_provider, | |||
Boolean $realm = $::foreman_proxy::params::realm, | |||
Foreman_proxy::ListenOn $realm_listen_on = $::foreman_proxy::params::realm_listen_on, | |||
String $realm_provider = $::foreman_proxy::params::realm_provider, | |||
Enum['freeipa', 'ad'] $realm_provider = $::foreman_proxy::params::realm_provider, |
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'd prefer to keep this as a string since that allows third party plugins to still use the same infra.
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.
Sure, I can tweak that back...
@ekohl Cheers for the quick review... |
Looks like I forgot this. Looking back I'm wondering if this is a good idea. We don't expose any plugin options on the top level and it should already be possible to pass in all the parameters. I certainly like the inclusion of the AD module if that's the provider. For the specific options we can also expose the plugin in the installer. |
Include 'foreman_proxy::plugin::realm::ad' class if 'realm_provider = ad'
Created a new 'Foreman_proxy::AdConfig' datatype
Add test coverage.
TODO: