add options support

[b4ldr]

This pull request allows on to add values to Dpkg::Options, specifically --force-conf*

[daenney]

Please leave the new line in place.

[daenney]

This is missing tests and documentation.

[b4ldr]

tests and documentation added

[jyaworski]

I think this looks good now, but I'm still really wary of what @daenney mentioned. Looking into it, that really does appear to be the only way to do it in ruby.

Would it be possible to call puppet's validate_bool?

[b4ldr]

@jyaworski, I updated the boolean check to use the following which is the same code that validate_bool eventully runs

if value.is_a?(TrueClass) || value.is_a?(FalseClass) then

validate_bool calls is_bool[1] which does the same as above[2]

[1]https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/lib/puppet/parser/functions/validate_bool.rb#L27
[2]https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/lib/puppet/parser/functions/is_bool.rb#L16

[daenney]

Why are we complicating this so much? If the value is actually false, as in the false in Puppet value in the ERB will be falsey, else truthy. Since we've already validated that it's a bool in the Puppet manifest, we don't need any additional checks to figure out if it's a TrueClass or a FalseClass, it's already guaranteed as it wouldn't pass the validate_bool otherwise.

Never mind, there's too many things named value that are being checked for things, this is getting confusing.

[daenney]

Why is there an additional check here? We've already checked if they're a boolean or not so they contain a value.

[b4ldr]

We check if it is a boolean or not so that we can provide a usefull error message to the user i.e. this value must be a boolean. We then we check if the value is true.

[b4ldr]

I could add individual checks in the puppet manifest like

validate_bool($options['force_confdef'])
validate_bool($options['force_confold'])
validate_bool($options['force_confnew'])
validate_bool($options['force_confmiss'])

And remove the check in the erb if you would prefer

[jyaworski]

@b4ldr I would prefer in-manifest checks.

[b4ldr]

@daenney @jyaworski hows that?

[jyaworski]

As you mentioned earlier:

validate_bool($options['force_confdef'])
validate_bool($options['force_confold'])
validate_bool($options['force_confnew'])
validate_bool($options['force_confmiss'])

[daenney]

We check if it is a boolean or not so that we can provide a usefull error message to the user i.e. this value must be a boolean. We then we check if the value is true.

Ah, I see what you're getting at.

And yes, I'd be happier with 4 validate_bool calls actually, catch it as early as possible so we keep the logic in the template to a minimum. This could further be improved once we start moving to Puppet 4 only for our modules and leverage the type system instead.

[b4ldr]

As you mentioned earlier:

Yes i have made the update, i meant is it ok now?

And yes, I'd be happier with 4 validate_bool calls actually

done :)

[daenney]

This looks good. One quick question, we're default to confed and confold to being true. That's the default behaviour?

[b4ldr]

@daenney I think the default behaviour is confdef true and everything else false. This means that apt will always ask you what to do if there is a conflict. I set confold to true because it makes most sense in a puppet world i.e. you are probably managing config files and you want your managed config to stay.

But i can also see the argument that we should keep everything default and let the user choose.

happy to change if you prefer

[daenney]

I'll take a look at what the default behaviour is. We should ensure to mimic that so we don't surprise users with different behaviour. We can introduce new features without major version bumps but we have to be careful to not introduce them in such a way that they would alter anything. We can always flip that default on a major version bump and in the mean time have a recommended settings entry in the README instead.

[b4ldr]

Happy to follow advice here i am far from an expert in apt or dpkg

[jyaworski]

Ping @daenney

[jyaworski]

This needs rebased.

[b4ldr]

rebased

[anarcat]

@daenney I think the default behaviour is confdef true and everything else false. This means that apt will always ask you what to do if there is a conflict. I set confold to true because it makes most sense in a puppet world i.e. you are probably managing config files and you want your managed config to stay.

sorry to take old skeletons out of the closet, but i think this is not quite correct, and out of scope of this module. --force-confold applies not only to unattended-upgrades run (which is, in fact, unaffected by it) but also all dpkg runs, which seems rather overkill.

i don't see why the unattended-upgrades module should mess with dpkg settings in the first place. this seems unnecessary and confusing. i was surprised to notice this, actually, and it might strike people as odd, in general.

this could be particularly bad if someone tries to do manual upgrades, which are still necessary in some cases (for example during major upgrades), as new configs will fail to be deployed, which may certainly cause breakage.

i'll see if i can make a PR to fix this, but i'm afraid the simplest would just be to back this out completely.

[anarcat]

followup discussion in #190.