(PUP-6452) Allow defaultfor to accept regex

[petems]

• Allows easier matching for defaults. For example, Fedora 22+ uses DNF. It's easier to just give a regex of 22-29 rather than add a new one each time

Code is rough right now, but working according to the specs! 😄

[puppetcla]

CLA signed by all contributors.

[branan]

I was just thinking earlier today that this would be super useful (especially for things like yum/dnf and systemd). Thanks for taking the time to open it! I'm not gonna do a serious review at 22:30, but I'll get back to you on this as soon as I have some spare cycles.

[nacc]

Would it be possible to make this more flexible than just regex? AIUI, even for Fedora, it's really F22+, not just F2{2-9}. That is, when F30 comes out, the regex is no longer correct and there would be a regression, right? Similarly for Ubuntu, we want to say 15.04 was the turning point for systemd being the default, and just be able to go off that value as to what the default init-script provider is.

[kylog]

Another idea: have defaultfor follow the pattern of confine and allow a code block. Then for the dnf case the code could be something like:

defaultfor :true begin
  Facter.value(:operatingsystem) == :fedora && Facter.value(:operatingsystemmajrelease).to_i >= 22
end

I haven't looked to see if this would fly or not, just tossing out ideas :)

[branan]

@kylog that's probably the way to go moving forward. In my mind that makes regex not super useful - but I'm also not sure anyone on Client will have the bandwidth to make that change. I don't think we should block small improvements on us having a better idea but not actually doing it

@petems I'm happy to merge this as-is, unless you feel like you want to take on the defaultfor accepting a block change, in which case I'd consider that to supersede this PR

[petems]

@branan Right now I wouldn't have the bandwidth without some pairing on how to do that (never really got block argument stuff in Ruby). How do we feel about going for Regex in the mean time?

[branan]

@petems I'd much rather have regex than nothing, and I don't think implementing regex actively prevents us from doing more complex block stuff in the future. How do you feel about just getting this in @kylog ?

[kylog]

Yeah, I'm +1.

[petems]

Ok, I'm cool with getting this merged and opening a ticket for more complex block arguments in the future? 👍

[petems]

@kylog @branan Can I get this unblocked and merged? 👍

[joshcooper]

I think we need to anchor non-regexp values, otherwise we'll match on substrings, e.g.

irb(main):001:0> a = /foo/
=> /foo/
irb(main):002:0> b = "bar"
=> "bar"
irb(main):003:0> Regexp.union(a, b)
=> /(?-mix:foo)|bar/
irb(main):004:0> c = Regexp.union(a, b)
=> /(?-mix:foo)|bar/
irb(main):005:0> c =~ 'foo'
=> 0
irb(main):006:0> c =~ 'bar'
=> 0
irb(main):007:0> c =~ 'oobar'
=> 2

Note we match oobar. I'm thinking the regexp would need to be a union of regexp and strings of the form /^#{value}$/ for each non-regexp value, e.g.

irb(main):011:0> d = Regexp.union(a, /^#{b}$/)
=> /(?-mix:foo)|(?-mix:^bar$)/
irb(main):012:0> d =~ 'oobar'
=> nil
irb(main):013:0> d =~ 'bar'
=> 0

Also if values can contain multiline text (eg user-specified input), I'd recommend using \A and \Z instead.

[petems]

So, I ended up asking a question over on Code Review on Stack Exchange, because I felt this logic was overly complex, any I got a suggestion that would fix the above issue and make he code simpler:

fact_val = Facter.value(fact).to_s.downcase
fact_val != "" and [*values].any? { |v| v === fact_val }

See: http://codereview.stackexchange.com/questions/149811/regex-detection-for-puppet-configuration/149826#149826

So this works, and all the unit tests pass, except the ones about multiple defaultfor... have you got any ideas why that would be?

[petems]

Failures:

  1) Puppet::Provider default providers should find the default provider
     Failure/Error: expect(subject.name).to eq(type.defaultprovider.name)

       expected: :nondefault
            got: :default

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -:nondefault
       +:default

     # ./spec/unit/provider_spec.rb:279:in `block (3 levels) in <top (required)>'

  2) Puppet::Provider default providers should consider any true value enough to be default
     Failure/Error: expect(subject.name).to eq(type.defaultprovider.name)

       expected: :nondefault
            got: :default

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -:nondefault
       +:default

     # ./spec/unit/provider_spec.rb:381:in `block (3 levels) in <top (required)>'

  3) Puppet::Provider default providers when there are multiple defaultfor's of equal specificity should be default for the first defaultfor
     Failure/Error: expect(provider).to be_default
       expected `Puppet::Type::Test::ProviderDefault.default?` to return true, got false
     # ./spec/unit/provider_spec.rb:332:in `block (4 levels) in <top (required)>'

  4) Puppet::Provider default providers when there are multiple defaultfor's of equal specificity should be default for the last defaultfor
     Failure/Error: expect(provider).to be_default
       expected `Puppet::Type::Test::ProviderDefault.default?` to return true, got false
     # ./spec/unit/provider_spec.rb:339:in `block (4 levels) in <top (required)>'

  5) Puppet::Provider default providers when there are multiple defaultfor's with different specificity should be default for a more specific, but matching, defaultfor
     Failure/Error: expect(provider).to be_default
       expected `Puppet::Type::Test::ProviderDefault.default?` to return true, got false
     # ./spec/unit/provider_spec.rb:357:in `block (4 levels) in <top (required)>'

  6) Puppet::Provider default providers when there are multiple defaultfor's with different specificity should be default for a more specific, but matching, defaultfor with regex
     Failure/Error: expect(provider).to be_default
       expected `Puppet::Type::Test::ProviderDefault.default?` to return true, got false
     # ./spec/unit/provider_spec.rb:365:in `block (4 levels) in <top (required)>'

  7) Puppet::Provider default providers when there are multiple defaultfor's with different specificity should be default for a less specific, but matching, defaultfor
     Failure/Error: expect(provider).to be_default
       expected `Puppet::Type::Test::ProviderDefault.default?` to return true, got false
     # ./spec/unit/provider_spec.rb:372:in `block (4 levels) in <top (required)>'

Finished in 2.86 seconds (files took 1.4 seconds to load)
233 examples, 7 failures

[MosesMendoza]

@petems resurrecting this PR - do you want to push up your latest code that caused the test failures you referenced in your last comment? We can help debug it.

[petems]

@MosesMendoza Done!

[Magisus]

The spec failures are equality failures: the first group of failures are due to values containing "Darwin" and being compared against "darwin". In the second group, values contains a Symbol, :os1, but the Facter value is a string. So it seems like this logic might actually get more complex to correctly handle all these different cases... Basically, it seems like === works great if you know you have a Regex, but is less predictable beyond that.

[Magisus]

@petems will you have time to play with this more? Probably going to need a different solution than ===.

[MosesMendoza]

@petems i'm going to push a commit up to your branch that addresses these failures if i have access, otherwise I'll close this and open a new with your changes and mine

[MosesMendoza]

ok I don't have push access so i'll open a new branch/pr

[MosesMendoza]

superseded by #6292