Issue #12012 has been updated by Daniel Pittman.
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. ---------------------------------------- Bug #12012: Facter globally overrides LANG environment variable at load time https://projects.puppetlabs.com/issues/12012 Author: Chris Price Status: Unreviewed Priority: Normal Assignee: Chris Price Category: library Target version: 1.7.x Keywords: Branch: 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 #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 -- You received this message because you are subscribed to the Google Groups "Puppet Bugs" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-bugs?hl=en.
