Skip to content

(#19845) block confines#410

Closed
dalen wants to merge 1 commit intopuppetlabs:masterfrom
dalen:19845_block_confine
Closed

(#19845) block confines#410
dalen wants to merge 1 commit intopuppetlabs:masterfrom
dalen:19845_block_confine

Conversation

@dalen
Copy link

@dalen dalen commented Mar 21, 2013

Allow confine to take a block parameter, like:

confine { code... }

Builds on top of pull #408

Choose a reason for hiding this comment

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

You could get rid of the conditional and simply do @block = block. If no block is passed then the block argument will be nil.

Copy link
Author

Choose a reason for hiding this comment

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

yes, but I still needed it for the others. but I can have them in a unless block_given?

Choose a reason for hiding this comment

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

Ah yes, sorry.

So... I don't yet understand how this change set works. If we discard the fact we're confining to, how does Facter know what value to yield to the block?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't yield any value. Either you pass only a block to confine like for example:

confine { File.exist?('/usr/bin/lldpctl') }

Or you pass a fact and one or more values that it should match like

confine :manufacturer => /Dell.*/

The value it is matched against can however also be a Proc object in which case the fact value is yielded to it, like:

confine :ipaddress => lambda { |addr| IPAddr.new('192.168.0.0/16').include? addr }

But the new thing in this pull request compared to Pull #408 is the first variant.

@puppetcla
Copy link

CLA Signed by dalen on 2012-07-17 21:00:00 -0700

@jeffmccune
Copy link

I'm trying to use this and I've written a simple fact to try out the new behavior:

Facter.add("jeff") do
  confine :operatingsystem do |os|
    require 'pry'; binding.pry
    true
  end
  setcode do
    "Jeff #{rand(1000)}"
  end
end

It seems as though the block never gets executed. The confine method in Facter::Util::Resolution#confine which is the receiver of my confine :operatingsystem message, doesn't accept a block...

Could you give me an example of how you're using these block based confinements?

Unfortunately, I don't think we're going to be able to get this into 1.7.0, sorry... I'll look at cleaning it up in the next hour, but I'm going to have to switch back to working against a deadline task if my refactoring goes beyond that.

-Jeff

@dalen dalen mentioned this pull request Mar 22, 2013
@adrienthebo
Copy link

@dalen it's been a week since this has seen activity, and it looks like this PR needs to be rebased. Do you know what the status of this PR is?

@dalen
Copy link
Author

dalen commented Mar 29, 2013

It is just waiting for review the last week. Jeff had some thoughts about how the syntax should be and then I've implemented that.
I can rebase, but it just needs a review now.

Allows a confine to take only a block parameter like:

confine { File.exist? '/bin/foo' }

Or take a fact name and a block:

confine :ipaddress do |addr|
  require 'ipaddr'
  IPAddr.new('192.168.0.0/16').include? addr
end

This is similar to what Puppet allows in provider confines.
@jeffmccune jeffmccune closed this in b7f75cf Apr 3, 2013
@jeffmccune
Copy link

summary: Merged into master as b7f75cf. This should be released in Facter 2. Thanks again for the contribution! Please note, I re-wrote the Facter::Util::Resolution#confine API documentation in commit 49e3f85

The updated documentation provides specific documentation for each overloaded form of #confine, as seen below.

Screen Shot 2013-04-03 at 1 39 31 PM

-Jeff

florindragos pushed a commit that referenced this pull request Jun 15, 2020
* (FACT-2475) Fix os.release for Debian 10
* (FACT-2475) os.distro use os-release
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.

4 participants