Strong typing for Booleans

[jflorian]

This module has lots of params expecting (effectively) Enum["yes","no"]. What do you think about a new custom type for a BaculaBoolean accepting Variant[Boolean,Enum["yes","no"]]?

I don't recall if Bacula itself supports true/false, but we should be able to easily cast appropriately either using whatchamacallit from the stdlib or have our own corresponding output function.

[smortex]

Yes, I was also thinking about it without being sure about how to do it properly…

Sometimes, the templates attempts to write explicitly set values and to not write values which where not automatically set… This does not work well with boolean (here the default is false, but if you explicitely set it to false, the configuration change still does not have a TLS Verify Peer line ☹️ see note bellow):
https://github.com/xaque208/puppet-bacula/blob/cec644d6c6730e233477a405f659d0bb678aca70/templates/_tls_server.erb#L2-L4

Sometimes, the value is always output, and the default value of the manifest is used (it is supposed to be the default upstream value):
https://github.com/xaque208/puppet-bacula/blob/f4c37bbf7d242ca415b0db89e2b8e673d5a4fb3b/templates/job.conf.erb#L49

Distinguishing false and undefined is quite verbose, so I did not changed the code to use one of these two methods consistently. However, I guess that we are both expecting more consistency and it may make sense to change all these booleans to be added when explicitly set:

ERB:

<% unless @foo.nil? %>
  Foo = <%= @foo %>
<% end %>

EPP:

<% unless $foo == undef { %>
  Foo = <%= $foo %>
<% } %>

Looking at the bacula source code, it seems that bacula supports yes, true, no and false for booleans (src/lib/parse_conf.c:892).

I guess that the bool2str function from stdlib can be handy here, maybe by adding a custom bacula::yesno() function that accepts a single argument (a boolean or a string) and use bool2str() when it receives a boolean (so that we have the 'yes' and 'no' constants in a single place), and just passes the parameter when it is a string.

Writing this comment helped me a lot to make things more clear in my head 😄. Do you think this make sense? Do you see any pitfall?

If you can, please submit a Pull-Request 😉 It should not be a complicated change, but there are a lot of files to update…

[smortex]

D'oh, "Close and comment" is not "Preview"…

[jflorian]

That analysis is an excellent start. Thank you! I'm definitely wanting the consistency, whatever it may become. I do think this makes sense and I'll continue thinking about potential pitfalls, thought at the moment I cannot think of any.

Regarding the TLS Verify Peer example, why have the template output that conditionally? It may just be my Python bias 😇, but I believe explicit is always better than implicit, especially where security is concerned. Yeah, it's more to read, but that takes less time than figuring out what the default might be (and when it might have changed over time). Besides, if this module works correctly, nobody should really need read the output anyway. 😉 But you're absolutely right, that conditional behavior makes this endeavor less trivial.

Yes, there are LOTS of things that would need to be changed, which is why I wanted to start this discussion before delving in -- which I almost did and then my "whoa fella" alarm went off, largely because of the sighting of some conditional output in the templates.

[smortex]

Hehe… The conditional for the TLS Verify Peer was just an attempt to do like the other parameters which are not output unless an explicit value is set… Only after I realized it was not doing what I wanted but the result was a configuration working as expected. Trying to think about it I got quickly confused and gave up… From time to time I remind this and feel terrible about it 😱. So this issue seems to be a path to great improvement!

Regarding the explicit vs. implicit configuration, I trend to prefer to not output values if the user has not set one. It makes templates a bit more complicated, but when upstream defaults change, it's generally a change you want unless you have previously set an explicit value (and when the default value in a module is different from the default value in the software, it's a PITA… This happened for example for the apache module in 2013 and because of breaking compatibility with previous modules versions, it got fixed only in 2015… no … it was in Apr 2017… wait … some had to wait Nov 2017… Well, it is supposed to work as expected now 🙄 I still have the default value explicitly set in my manifests).

[jflorian]

Good point about allowing Bacula's own defaults flowing thru. Yet, your own final statement of being explicit kind of proves my point. 😉 (I realize though that happened because of a failed attempt at shadowing defaults.)

Whatever we decide here, I think it would be smart for us to declare BOLDLY the convention of this module in the README and that anything contrary to that statement is a BUG and should be reported/fixed as such with the hopes of eliminating diverging philosophies that occur on a line-by-line basis. Maybe we should start with that first? Something like:

Variant A: """This module aims to follow the upstream Bacula defaults to the greatest extent possible. Any parameter not declared in your use of it, will result in the omission in the corresponding config file. If you are explicit, you'll get what you asked for. If you are silent, you'll get Bacula's default, regardless of any changes upstream may make over time. Any contradiction of this is a bug in this module and should be reported as such."""

Variant B: """This module aims to follow the upstream Bacula defaults to the greatest extent possible. Any parameter not declared in your use of it, will result in a module-provided default in the corresponding config file. If you are explicit, you'll get what you asked for. If you are silent, you'll get this module's default, which may not accurately reflect any changes upstream may make over time. Any changes to this module's defaults will only occur with a major version bump. Any contradiction of this is a bug in this module and should be reported as such."""

Other variants are, of course, welcome. Can o' worms, I know, but I think it would be so healthy.

[smortex]

@jflorian +1 for variant A. In my experience, it allows more readable profiles. If upstream follows semver, a config file default value change should result in a major version change, which will alert the administrator and he may decide to update his configuration accordingly. However, this should have no impact on the profile code itself, so no version change is to be expected for the abstraction layer that this module implements.

As a rule a thumb, I try to do this:

1. modules use default upstream settings, but allow changing them if the user wants to (if it's possible, do not duplicate the default configuration from the software to the module — what we are speaking about);
2. use profiles (roles and profile pattern) to setup the site-specific configuration, using resource-like declarations and explicitely setting all parameters you want to enforce;
3. if some machines need a slightly different configuration, add a parameter to the profile, and set this parameter value through Hiera.

You currently can't follow strictly the role and profile pattern with the bacula module, but fixing this is actually quite high on my TODO list 😄. Here is my current bacula_server profile:

class profile::bacula_server (
  Hash $pools,
  Hash $schedules,
) {
  include profile::bacula_client

  $fqdn = $facts.dig('networking', 'fqdn')

  class { 'bacula::director':
    messages => {
      'Daemon'       => {
        mname   => 'Daemon',
        console => 'all, !skipped, !saved',
        append  => '"/var/log/bacula/log" = all, !skipped',
      },
      'Standard-dir' => {
        mname       => 'Standard',
        console     => 'all, !skipped, !saved',
        append      => '"/var/log/bacula/log" = all, !skipped',
        catalog     => 'all',
        mail        => 'sysadmins@opus-codium.fr = all, !skipped',
        mailcmd     => "\"/usr/sbin/bsmtp -h localhost -f \\\"(Bacula) <bacula@${fqdn}>\\\" -s \\\"Bacula: %t %e of %c %l\\\" %r\"",
        operatorcmd => "\"/usr/sbin/bsmtp -h localhost -f \\\"(Bacula) <bacula@${fqdn}>\\\" -s \\\"Bacula: Intervention needed for %j\\\" %r\"", # lint:ignore:140chars
      },
    },
  }

  # Edit: A wrong comment was here, because it's not possible to strike-over text
  # in code, it was removed.  Sorry for the inconvenience.
  class { 'bacula::storage':
    storage => $trusted['certname'],
    address => $trusted['certname'],
    device  => '/home/bacula',
  }

  create_resources(bacula::director::pool, $pools)
  create_resources(bacula::schedule, $schedules)
}

[zachfi]

I agree with the points above, and indeed I try to follow those as much as possible, with respect to default values in upstream software. Sometimes defaults do change upstream, which can present some amount of trouble for the modules, but I don't know of a case where that has happened to this module.

By all means, a ternary in the ERB, or some method that returns the correct string, all would be appropriate. Ideally it remains readable for the curious adventurer to happens to open the template, etc.

Thanks for the efforts!

[smortex]

@jflorian are you on this? If no, I can put this on my TODO list :)

[jflorian]

I could be, but no immediate plans. Let's agree to post notice here upon starting so neither wastes effort.

…

On July 4, 2018 9:03:39 AM EDT, "Romain Tartière" ***@***.***> wrote: @jflorian are you on this? If no, I can put this on my TODO list :) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #121 (comment)

-- John

[jflorian]

Also, on further reflection I too am on board with variant A. One of things that has scared me away from the puppetlabs-apache module is just how invasive it is. My own, intentionally wimpy (just enough) apache module reflects the platform defaults, primarily by omission. Sounds familiar, right? 😁

[smortex]

I might have some time to look into this soon.

Epp templates offer a much more readable syntax when it comes to access out of scope variables (e.g. $bacula::tls_verify_peer vs scope['bacula::tls_verify_peer']) and convenient expandability with custom functions (I am thinking about some kind yesno2str() if we add a Yesno custom type which accepts true, "true", "yes", false, "false" and "no").
Do you have objection in respect to switching from erb to epp templates?

[jflorian]

@smortex, the call isn't mine to make but FWIW, I think your proposal sounds like the best approach, especially the custom type. I've not used epp yet myself. I know the syntax is more apropos but haven't investigated to learn if it's as versatile as erb.

[zachfi]

I'm +1 on using epp @smortex. It sounds like a whole pile of work, and I don't anticipate having time in the near future to help, but if you're willing, 👍 from me.

[smortex]

Just a quick follow up to tell you that things are going well. I finished the conversion to EPP, it took a bit more work than expected but the more strict EPP templates syntax allowed to spot and fix some (minor) issues, and I wrote a basic tool to help identify consistency issues.

The WIP is currently available in the epp branch of our fork. I am in the process of cleaning-up all this for reviewing and opening focused Pull Requests. Thank you @xaque208 for the quick merging, it allows me to rebase my changes on top of master and have less and less changes to think about 😃

[jflorian]

@smortex That tool is some beautiful code (and it looks useful too). Once again, this module seems to have so much know-how stuffed in. The way exported resources get used here taught me a lot and now there's a great example of yacc, which is something I've always wanted to learn. Thanks for sharing it!

[smortex]

Hi @jflorian! Please see #128 if you are interested in testing this 😉 !