search

voxpupuli / puppet-rabbitmq

RabbitMQ Puppet Module
http://forge.puppetlabs.com/puppet/rabbitmq
Apache License 2.0
171 stars 500 forks source link

ldap_log should be string not boolean to support network/network_unsafe options #960

Open ArcticSnowman opened 1 year ago

ArcticSnowman commented 1 year ago

Currently the rabbitmq::ldap_log only accepts true or false. According to https://www.rabbitmq.com/ldap.html it should also accept network and network_unsafe

This can be achieved by changing the type from boolean to string

wyardley commented 1 year ago

I think normally it would be somewhat of an anti-pattern to have something that's an optional boolean, and having it a string (with true / false as possible options) would be breaking at best, and kind of wacky UI at worst.

Probably the best bet would be something like Variant[Enum['network', 'network_unsafe', 'true', 'false'], Boolean], but might be good to get some other opinions from Voxpupuli collaborators (maybe on IRC) to confirm that this is a good way to go.

Are you willing / able to make a PR (including some updates / additions to tests) to handle this?

If this feature was added later, it might also be necessary to gate it depending on RMQ version, though I'm not strongly opinionated about this.

smortex commented 1 year ago

Boolean are tricky because undef and false both evaluate to false, so we must be sure to output a value when the setting is set to false.

Changing the accepted value from Boolean to String is a backwards-incompatible change, maybe we can accept a Boolean as before, optionally emitting a deprecation warning if a Boolean is passed, so that we can remove it later. I am not a user of this module, IMHO the most important is to have some consistency at the module level: if it use Boolean in other places, it make sense to accept a Boolean value. If it is an exception, it make sense to deprecate this usage.

wyardley commented 1 year ago

@smortex: thanks for weighing in

Changing the accepted value from Boolean to String is a backwards-incompatible change, maybe we can accept a Boolean as before, optionally emitting a deprecation warning if a Boolean is passed

From an interface standpoint, I agree it's not ideal having both bools and strings work, though it's the best chance at avoiding breaking changes here, and having the strings "true" / "false" be the only way to set true / false doesn't seem ideal anyway.

That said, I don't think there's any worry about undef if we used the variant above, since undef still wouldn't be allowed, and we are not evaluating ldap_log as a boolean directly anywhere I can see -- just using it in a template, where I think we're already ending up with the string "true" or "false" in the config file anyway, if I'm reading https://github.com/voxpupuli/puppet-rabbitmq/blob/8854872f9bc9b46bf3629a15025ae5e2856ee28c/templates/rabbitmq.config.erb#L194 correctly.

If we were to add an if statement around ldap_log somewhere in that template in the future (doesn't look like there's one now), agree that could cause some unpredictable results.