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

Evaluate confine block in case-insensitive way #2705

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

mhashizume
Copy link
Contributor

@mhashizume mhashizume commented Apr 15, 2024

Prior to this commit when a user provided a confine block, Facter would downcase the value when evaluating it.

For example:

confine :kernel do |value|
  value == "Linux"
end

While Facter's public documentation states that this is a valid way to write a confine block, it would incorrectly and unexpectedly evaluate as false on Linux systems.

However, these other styles of confine would return true:

confine :kernel => "Linux"

confine kernel: "Linux"

This downcasing behavior was introduced in 7a81945 as a way of comparing values in a case-insensitive way. However when block confines were introduced in e4c8689, it added block evaluation before value comparison, making the case-insensitive comparison moot with blocks.

This commit retains existing behavior of evaluating a confine block with a downcased fact value, while adding evaluation with the raw fact value to ensure expected behavior.

Prior to this commit when a user provided a confine block, Facter would
downcase the value when evaluating it.

For example:

confine :kernel do |value|
  value == "Linux"
end

While Facter's public documentation states that this is a valid way to
write a confine block, it would incorrectly and unexpectedly evaluate
as false on Linux systems.

However, these other styles of confine would return true:

confine :kernel => "Linux"

confine kernel: "Linux"

This downcasing behavior was introduced in 7a81945 as a way of comparing
values in a case-insensitive way. However when block confines were
introduced in e4c8689, it added block evaluation before value
comparison, making the case-insensitive comparison moot with blocks.

This commit retains existing behavior of evaluating a confine block with
a downcased fact value, while adding evaluation with the raw fact value
to ensure expected behavior.
@mhashizume mhashizume added the bug Something isn't working label Apr 15, 2024
@mhashizume mhashizume requested a review from a team as a code owner April 15, 2024 21:04
@mhashizume
Copy link
Contributor Author

This was originally tried in #2699 but in that PR I changed

return !!@block.call(value)

To return !@block.call(value).nil because the original code triggered Rubocop's Style/DoubleNegation cop. Those two approaches are not equivalent, so in this PR I simply disabled the cop.

We realized the behavior on the first PR was not correct because it gave the wrong output for a custom fact used internally in Puppet. That custom fact relies on using multiple resolutions with OS-specific confine blocks like so:

Facter.add('test_fact') do
  confine :kernel do |value|
    value == 'darwin'
  end

  setcode do
    'foo'
  end
end

Facter.add('test_fact') do
  confine :kernel do |value|
    value == 'linux'
  end

  setcode do
    'bar'
  end
end

In this PR I've added test coverage for such circumstances.

@cthorn42 cthorn42 merged commit 5194a37 into puppetlabs:main Apr 18, 2024
17 checks passed
@mhashizume mhashizume deleted the FACT-3156/main/confine-block branch May 8, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants