Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#17917) Allow regexp confines #371

Closed
wants to merge 1 commit into from
Closed

Conversation

dalen
Copy link

@dalen dalen commented Dec 3, 2012

No description provided.

@zaphod42
Copy link

zaphod42 commented Dec 4, 2012

@dalen Can you clean up that test file? The long list of confines that is used across all of those tests makes it hard to follow what each case is that it is checking.

@dalen
Copy link
Author

dalen commented Dec 5, 2012

I've separated the confine creations for each test so it is a bit more clear what each test case is tesing. And fixed one of them so it actually tests what it says it is testing (if any of the provided symbol values matches the fact's string value).

@zaphod42
Copy link

zaphod42 commented Dec 5, 2012

@dalen Thanks for doing that, it is a lot nicer now. There ends up being a lot of duplication between the tests, which is what the one Confine creation before was trying to get rid of. Looking at the tests, the values that tell us the most about what it is trying to do are:

  • The value of the fact
  • The confinement value
  • Whether or not the confinement is true of false

Those three things together entirely define the test and should be called out more clearly. Maybe something like:

it "confines by regular expression" do
  confined(:fact_value => "blah", :by => /blah/).should be_true
  confined(:fact_value => "blah", :by => /foo/).should be_false
end

def confined(options)

  # setup the fact
  # create the confine
  # return the result of the confine.true? call

end

Also cleaned up the spec tests around confines a bit.
@dalen
Copy link
Author

dalen commented Dec 6, 2012

okay, updated a bit.

@zaphod42
Copy link

zaphod42 commented Dec 6, 2012

Great! Thanks for doing that. I'm going to hand this over to @jeffmccune to finish it off.

return true if value == v
end
return false
return @values.any? { |v| convert(v) === value }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire patch is great. My only comment is that the commit message doesn't closely align with the implementation. True, this patch provides regular expression confinement functionality, but it does so by implementing Case Equality matching, of which regular expression matching is a subset.

I've simply updated the commit message to more closely align the change in code with the description of what we're changing. No action required to get this merged.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the new commit message:

    (#17917) Allow case equality confinement matching

    Without this patch applied the confine functionality is limited to
    testing equality.  This is a problem because some facts have different,
    but similar, values over time.  For example, the use case motivating
    this feature is:

        I have some facts I want to run on all generations of dell machines,
        but the manufacturer fact has changed between them so I want to be
        able to use something like /.*Dell.*/

    This patch implements case equality which provides a superset of the
    regular expression matching that addresses the problem.

    This patch also cleans up the spec tests around confines a bit.

@jeffmccune
Copy link

Merged

Into 1.7.x as a0b1acb. This will be released in Facter 1.7.0 and 2.0.0.

Thanks for the contribution Erik, please keep 'em coming!

-Jeff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants