ldap_log should be string not boolean to support network/network_unsafe options about puppet-rabbitmq HOT 3 OPEN

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.

from puppet-rabbitmq.

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.

from puppet-rabbitmq.

@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

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.

from puppet-rabbitmq.