There’s been a periodic discussion of merging Facter’s and Puppet’s execution code. I would be in favor of a solution merging the codebase; however, using Facter to generate facts and provide libraries for executing code seems to be somewhat suspect. That being said I like your general plan, but it strikes me that the execution functionality might need to be either pulled out of both libraries and put in a common library, and either require that library or vender it in both.

With respect to backwards incompatible behavior, I have this dream of releasing Facter 2.0, which if it’s a major release it can be backwards incompatible so we can just say “Stopped clobbering the LANG environment variable” and call it good.

Chris Price wrote:

Rather than globally modifying this variable we probably need to temporarily override it when we are about to execute a program whose output we intend to parse, and then restore the previous value as soon as we’ve finished executing the program. In the case of Facter, it appears that most (all?) of this sort of behavior is funneled through the method Facter::Util::Resolution.exec. Assuming that is the case, we should be able to remove the global modification of the environment var from facter.rb and simply override / restore the value inside of this “exec” method.

That, or some equivalent helper method, is the right place to do this.

There are a few utility methods lying around that appear to be intended for this sort of scenario (temporarily overriding / restoring environment variables), but neither of them is currently located in a spot that would make it amenable for use in resolving this ticket.

Bringing one of them into the util code is probably the right solution. Facter is a stand-alone project, so it needs a copy of the code that it owns. (This also means that when it needs different things to Puppet, it doesn’t have to add complexity to match.)

6) Refactor Puppet code to use “withenv” from the Facter code base rather than from puppet/lib/puppet/util/execution.rb (another separate ticket?)

That isn’t appropriate, since there is at least a theoretical desire to make Facter separable and replaceable in Puppet.

a) How to determine if there are any Facter facts that are executing programs and parsing their output without calling into Facter::Util::Resolution.exec to accomplish it? Any such facts could potentially be affected by the fact that they may no longer be running with ENV[‘LANG’] forced to ‘C’.

This only matters for the “core” facts, which are the ones that we ship with the Facter project. If third parties do something wrong in code we don’t ship, that isn’t so much our problem – though we should document this, so they are not flying blind. If they submit code doing the wrong thing code review will catch that before merging.

b) Documentation; customers' custom facts may have been relying on the old behavior without even realizing it… how do we convey the change to them?

Plenty of public communication. We can work out the details outside the ticket.

Adrien Thebo wrote:

it strikes me that the execution functionality might need to be either pulled out of both libraries and put in a common library, and either require that library or vender it in both.

I had that same thought—and after conferring with Daniel it seems like that would be more trouble than it’s worth for this small bit of overlapping code.

With respect to backwards incompatible behavior, I have this dream of releasing Facter 2.0, which if it’s a major release it can be backwards incompatible so we can just say “Stopped clobbering the LANG environment variable” and call it good.

So, that raises another question—is the current “master” branch in Facter going to be 2.x? Or 1.7.x? Regardless, I’ll just branch off of master for this bug, but would be good to sort out.

master is going to be 1.7.x, we’re hoping to get external facts and caching merged into that release, and then structured data into 2.0. It might be worth discussing making a 1.7.x branch, but for now branching from master seems fine.

Updated plan after consultation with Daniel:

• Don’t worry about the existence of a “with_env” in both facter and puppet;
• Don’t refactor anything in puppet in response to the facter changes for this ticket

Still need to determine how to be sure I haven’t screwed up any “core” facts via this change; hopefully existing spec + acceptance tests are sufficient?

Merged in 491d2aafb2c7d7a049c76c1721e8f13427cace6f by Daniel.

Ah, gotcha. Thanks.

On Wed, Jan 25, 2012 at 3:36 PM, tickets@puppetlabs.com wrote:

Issue #12012 has been updated by Daniel Pittman.

• Branch changed from https://github.com/puppetlabs/facter/pull/149 to https://github.com/puppetlabs/facter/pull/159

You forgot to update the branch, Chris, to your newer pull request.¶

Bug #12012: Facter globally overrides LANG environment variable at load time https://projects.puppetlabs.com/issues/12012

• Author: Chris Price
• Status: In Topic Branch Pending Review
• Priority: Normal
• Assignee: Chris Price
• Category: library
• Target version: 1.7.0
• Keywords:
• Branch: https://github.com/puppetlabs/facter/pull/159
• Affected Facter version:

In facter.rb, ~ line 41-44, we have this:

Set LANG to force i18n to C¶

# ENV[‘LANG’] = ‘C’

This can cause problems when users try to perform an ‘exec’ with a program that responds to this environment variable (see Puppet #11860https://projects.puppetlabs.com/issues/11860). However, setting this variable can be necessary / mandatory for cases where we need to parse the output of a command to obtain a status or value (which facter clearly does a LOT of), to ensure that the output of the program is formatted consistently.

Rather than globally modifying this variable we probably need to temporarily override it when we are about to execute a program whose output we intend to parse, and then restore the previous value as soon as we’ve finished executing the program. In the case of Facter, it appears that most (all?) of this sort of behavior is funneled through the method Facter::Util::Resolution.exec. Assuming that is the case, we should be able to remove the global modification of the environment var from facter.rb and simply override / restore the value inside of this “exec” method.

There are a few utility methods lying around that appear to be intended for this sort of scenario (temporarily overriding / restoring environment variables), but neither of them is currently located in a spot that would make it amenable for use in resolving this ticket. They are:

facter/spec/unit/util/loader_spec.rb, ~ line 24: method “with_env”; can’t currently use this because it’s defined in test code rather than production code puppet/lib/puppet/util/execution.rb, ~ line 6: method “withenv”; can’t use this from Facter because it’s defined in puppet.

Proposals:

1) Copy “withenv” from puppet/util/execution.rb into Facter codebase so that it can be used by both code trees (should this be a separate ticket?)

2) Refactor loader_spec.rb to use “withenv” from new location

3) Remove redundant “with_env” definition from facter/spec/unit/util/loader_spec.rb

4) Remove line that sets ENV[‘LANG’] in facter.rb

5) Refactor Facter::Util::Resolution.exec to use new “withenv” to temporarily override LANG for the scope of that method

6) Refactor Puppet code to use “withenv” from the Facter code base rather than from puppet/lib/puppet/util/execution.rb (another separate ticket?)

7) Remove “withenv” from puppet/util/execution.rb so that we are not maintaining two copies of this functionality

Concerns:

a) How to determine if there are any Facter facts that are executing programs and parsing their output without calling into Facter::Util::Resolution.exec to accomplish it? Any such facts could potentially be affected by the fact that they may no longer be running with ENV[‘LANG’] forced to ‘C’.

b) Documentation; customers' custom facts may have been relying on the old behavior without even realizing it… how do we convey the change to them?

c) (puppet) If we remove “withenv” from puppet/util/execution.rb, are existing spec tests exhaustive enough to confirm that we’ve successfully refactored all of the places in the puppet code that were calling the old version?

d) (puppet) If we remove “withenv” from puppet/util/execution.rb, any concerns that we will break modules / customer code?

Any comments / input greatly appreciated…¶

You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: http://projects.puppetlabs.com/my/account