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

postfix-ldap required for ldap lookup type on RHEL8+ #398

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

edrude
Copy link
Contributor

@edrude edrude commented Aug 6, 2024

Pull Request (PR) description

Install postfix-ldap package when ldap=true on RHEL8+

This Pull Request (PR) fixes the following issues

Fixes #397

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Wouldn't it be better to use the module's hiera data for this kind of thing rather than using conditionals in code?

@smortex
Copy link
Member

smortex commented Aug 6, 2024

Wouldn't it be better to use the module's hiera data for this kind of thing rather than using conditionals in code?

+1: some Array[String[1]] $ldap_packages parameter with the right default values in hiera and a package { $postfix::ldap_packages: ensure => installed } in the manifest seems more flexible.

@edrude
Copy link
Contributor Author

edrude commented Aug 7, 2024

Absolutely, I was just trying to get something down. If I get some time tomorrow I will update it.

@edrude
Copy link
Contributor Author

edrude commented Aug 7, 2024

Made the suggested changes.

One thing I have been struggling with when using this module. I find a few options that would appear at first to be useful for me, but end up being too opinionated. $mta and $ldap are both examples.

In the case of $ldap I want my ldap lookups to be a part of 'virtual_alias_maps' instead of 'alias_maps', but if $ldap = true the ldap config file is added to the 'alias_maps' option.

I also can't use $mta because if $mta = true then 'virtual_alias_maps' is set for me.

I see the merit of building in some common opinionated scenarios, maybe this is more of a documentation issue. If the docs had some callout to "these options are equivalent to" or "opinionated setup of" I think I would have arrived where I am now much faster.

Specifically for $ldap it may make sense to separate out the installation of the $ldap_packages so that you can still "enable ldap" without doing everything that setting $ldap to true does. Anything that separates out "making postfix ready to use ldap" from "configuring how postfix uses ldap"

@smortex
Copy link
Member

smortex commented Aug 7, 2024

One thing I have been struggling with when using this module. I find a few options that would appear at first to be useful for me, but end up being too opinionated. $mta and $ldap are both examples.

I quite agree. On one hand some interfaces are really not great IMHO, but on the other hand we don't want to break the setup of users of the module. It is hard to change things when they are done in a certain way. That being said, I think that all efforts to make diving in with the module are good to have. Feel free to send PRs.

@smortex smortex merged commit 4ab126b into voxpupuli:master Aug 7, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing postfix-ldap rpm on RHEL8+ distro
3 participants