Please review pull request #726: (#14229) Add a path type to settings opened by (pcarlisle)
Description:
This path type will expand all paths in a string separated by File::PATH_SEPARATOR, but does not add the paths to the settings catalog. This helps the most on Windows, where it means that paths given to settings will consistently use File::SEPARATOR, no matter how they were entered by the user.
- Opened: Fri Apr 27 19:20:46 UTC 2012
- Based on: puppetlabs:master (b5847f4b2d6af7afc839a4f8e15c37675ab378a0)
- Requested merge: pcarlisle:ticket/master/14229-path-setting (b41ee6d1f3bec80c3ea3f279f2cdab20d09231b1)
Diff follows:
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index 0090b4e..47f61c8 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -13,6 +13,7 @@ class Puppet::Util::Settings
require 'puppet/util/settings/string_setting'
require 'puppet/util/settings/file_setting'
require 'puppet/util/settings/directory_setting'
+ require 'puppet/util/settings/path_setting'
require 'puppet/util/settings/boolean_setting'
attr_accessor :files
@@ -530,7 +531,7 @@ def newsetting(hash)
:string => StringSetting,
:file => FileSetting,
:directory => DirectorySetting,
- :path => StringSetting,
+ :path => PathSetting,
:boolean => BooleanSetting,
} [type]
raise ArgumentError, "Invalid setting type '#{type}'"
@@ -665,7 +666,6 @@ def set_value(param, value, type, options = {})
end
end
- value = setting.munge(value) if setting.respond_to?(:munge)
setting.handle(value) if setting.has_hook? and not options[:dont_trigger_handles]
if read_only_settings.include? param and type != :application_defaults
raise ArgumentError,
@@ -893,6 +893,8 @@ def value(param, environment = nil, bypass_interpolation = false)
param = param.to_sym
environment &&= environment.to_sym
+ setting = @config[param]
+
# Short circuit to nil for undefined parameters.
return nil unless @config.include?(param)
@@ -921,7 +923,7 @@ def value(param, environment = nil, bypass_interpolation = false)
raise Puppet::SettingsError.new("Error converting value for param '#{param}': #{err}")
end
-
+ val = setting.munge(val) if setting.respond_to?(:munge)
# And cache it
@cache[environment||"none"][param] = val
val
diff --git a/lib/puppet/util/settings/directory_setting.rb b/lib/puppet/util/settings/directory_setting.rb
index 8b7579c..c50ee95 100644
--- a/lib/puppet/util/settings/directory_setting.rb
+++ b/lib/puppet/util/settings/directory_setting.rb
@@ -2,6 +2,6 @@
class Puppet::Util::Settings::DirectorySetting < Puppet::Util::Settings::FileSetting
def type
- return :directory
+ :directory
end
-end
\ No newline at end of file
+end
diff --git a/lib/puppet/util/settings/file_setting.rb b/lib/puppet/util/settings/file_setting.rb
index 0f308cc..d311541 100644
--- a/lib/puppet/util/settings/file_setting.rb
+++ b/lib/puppet/util/settings/file_setting.rb
@@ -45,20 +45,13 @@ def use_service_user?
@settings[:mkusers] or @settings.service_user_available?
end
- # Set the type appropriately. Yep, a hack. This supports either naming
- # the variable 'dir', or adding a slash at the end.
def munge(value)
- # If it's not a fully qualified path...
- if value.is_a?(String) and value !~ /^\$/ and value != 'false'
- # Preserve trailing slash (stripped by expand_path)
- isdir = value =~ /\/$/
+ if value.is_a?(String)
value = File.expand_path(value)
- value += '/' if isdir
end
value
end
-
def type
:file
end
diff --git a/lib/puppet/util/settings/path_setting.rb b/lib/puppet/util/settings/path_setting.rb
new file mode 100644
index 0000000..b18ad19
--- /dev/null
+++ b/lib/puppet/util/settings/path_setting.rb
@@ -0,0 +1,10 @@
+require 'puppet/util/settings/string_setting'
+
+class Puppet::Util::Settings::PathSetting < Puppet::Util::Settings::StringSetting
+ def munge(value)
+ if value.is_a?(String)
+ value = value.split(File::PATH_SEPARATOR).map { |d| File.expand_path(d) }.join(File::PATH_SEPARATOR)
+ end
+ value
+ end
+end
diff --git a/spec/integration/defaults_spec.rb b/spec/integration/defaults_spec.rb
index 27fefe8..a916a94 100755
--- a/spec/integration/defaults_spec.rb
+++ b/spec/integration/defaults_spec.rb
@@ -121,7 +121,7 @@
[:modulepath, :factpath].each do |setting|
it "should configure '#{setting}' not to be a file setting, so multi-directory settings are acceptable" do
- Puppet.settings.setting(setting).should be_instance_of(Puppet::Util::Settings::StringSetting)
+ Puppet.settings.setting(setting).should be_instance_of(Puppet::Util::Settings::PathSetting)
end
end
diff --git a/spec/unit/application/agent_spec.rb b/spec/unit/application/agent_spec.rb
index 2f3a9a0..0d3f09b 100755
--- a/spec/unit/application/agent_spec.rb
+++ b/spec/unit/application/agent_spec.rb
@@ -7,6 +7,8 @@
require 'puppet/daemon'
describe Puppet::Application::Agent do
+ include PuppetSpec::Files
+
before :each do
@puppetd = Puppet::Application[:agent]
@puppetd.stubs(:puts)
@@ -277,9 +279,10 @@
end
it "should exit after printing puppet config if asked to in Puppet config" do
- Puppet[:modulepath] = '/my/path'
+ path = make_absolute('/my/path')
+ Puppet[:modulepath] = path
Puppet[:configprint] = "modulepath"
- Puppet::Util::Settings.any_instance.expects(:puts).with('/my/path')
+ Puppet::Util::Settings.any_instance.expects(:puts).with(path)
expect { @puppetd.setup }.to exit_with 0
end
diff --git a/spec/unit/face/module/install_spec.rb b/spec/unit/face/module/install_spec.rb
index 3fbca55..88d7c7c 100644
--- a/spec/unit/face/module/install_spec.rb
+++ b/spec/unit/face/module/install_spec.rb
@@ -3,6 +3,7 @@
require 'puppet/module_tool'
describe "puppet module install" do
+ include PuppetSpec::Files
subject { Puppet::Face[:module, :current] }
@@ -24,10 +25,10 @@
end
let(:sep) { File::PATH_SEPARATOR }
- let(:fakefirstpath) { "/my/fake/modpath" }
- let(:fakesecondpath) { "/other/fake/path" }
+ let(:fakefirstpath) { make_absolute("/my/fake/modpath") }
+ let(:fakesecondpath) { make_absolute("/other/fake/path") }
let(:fakemodpath) { "#{fakefirstpath}#{sep}#{fakesecondpath}" }
- let(:fakedirpath) { "/my/fake/path" }
+ let(:fakedirpath) { make_absolute("/my/fake/path") }
context "without any options" do
it "should require a name" do
@@ -49,7 +50,7 @@
end
it "should accept the --target-dir option" do
- options[:target_dir] = "/foo/puppet/modules"
+ options[:target_dir] = make_absolute("/foo/puppet/modules")
expected_options.merge!(options)
expected_options[:modulepath] = "#{options[:target_dir]}#{sep}#{fakemodpath}"
diff --git a/spec/unit/module_tool_spec.rb b/spec/unit/module_tool_spec.rb
old mode 100644
new mode 100755
index edee553..53e778b
--- a/spec/unit/module_tool_spec.rb
+++ b/spec/unit/module_tool_spec.rb
@@ -1,3 +1,4 @@
+#!/usr/bin/env rspec
# encoding: UTF-8
require 'spec_helper'
@@ -111,11 +112,14 @@
end
end
describe '.set_option_defaults' do
+ include PuppetSpec::Files
+ let (:setting) { {:environment => "foo", :modulepath => make_absolute("foo")} }
+
[:environment, :modulepath].each do |value|
describe "if #{value} is part of options" do
let (:options) { {} }
before(:each) do
- options[value] = "foo"
+ options[value] = setting[value]
Puppet[value] = "bar"
end
it "should set Puppet[#{value}] to the options[#{value}]" do
@@ -124,13 +128,13 @@
end
it "should not override options[#{value}]" do
subject.set_option_defaults options
- options[value].should == "foo"
+ options[value].should == setting[value]
end
end
describe "if #{value} is not part of options" do
let (:options) { {} }
before(:each) do
- Puppet[value] = "bar"
+ Puppet[value] = setting[value]
end
it "should populate options[#{value}] with the value of Puppet[#{value}]" do
subject.set_option_defaults options
@@ -138,7 +142,7 @@
end
it "should not override Puppet[#{value}]" do
subject.set_option_defaults options
- Puppet[value].should == "bar"
+ Puppet[value].should == setting[value]
end
end
end
diff --git a/spec/unit/util/settings/file_setting_spec.rb b/spec/unit/util/settings/file_setting_spec.rb
index 4c60649..d075af3 100755
--- a/spec/unit/util/settings/file_setting_spec.rb
+++ b/spec/unit/util/settings/file_setting_spec.rb
@@ -286,14 +286,5 @@
@file.to_resource[:links].should == :follow
end
end
-
- describe "when munging a filename" do
- it "should preserve trailing slashes" do
- settings = mock 'settings'
- file = Puppet::Util::Settings::FileSetting.new(:settings => settings, :desc => "eh", :name => :myfile, :section => "mysect")
- path = @basepath + '/'
- file.munge(path).should == path
- end
- end
end
diff --git a/spec/unit/util/settings/path_setting_spec.rb b/spec/unit/util/settings/path_setting_spec.rb
new file mode 100755
index 0000000..2536c6d
--- /dev/null
+++ b/spec/unit/util/settings/path_setting_spec.rb
@@ -0,0 +1,30 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+
+describe Puppet::Util::Settings::PathSetting do
+ subject { described_class.new(:settings => mock('settings'), :desc => "test") }
+
+ context "#munge" do
+ it "should expand all path elements" do
+ munged = subject.munge("hello#{File::PATH_SEPARATOR}good/morning#{File::PATH_SEPARATOR}goodbye")
+ munged.split(File::PATH_SEPARATOR).each do |p|
+ Puppet::Util.should be_absolute_path(p)
+ end
+ end
+
+ it "should leave nil as nil" do
+ subject.munge(nil).should be_nil
+ end
+
+ context "on Windows", :if => Puppet.features.microsoft_windows? do
+ it "should convert \\ to /" do
+ subject.munge('C:\test\directory').should == 'C:/test/directory'
+ end
+
+ it "should work with UNC paths" do
+ subject.munge('//server/some/path').should == '//server/some/path'
+ subject.munge('\\\\server\some\path').should == '//server/some/path'
+ end
+ end
+ end
+end
diff --git a/spec/unit/util/settings_spec.rb b/spec/unit/util/settings_spec.rb
index d7fefa5..bc157af 100755
--- a/spec/unit/util/settings_spec.rb
+++ b/spec/unit/util/settings_spec.rb
@@ -665,22 +665,23 @@
before do
@settings = Puppet::Util::Settings.new
@settings.stubs(:service_user_available?).returns true
- @file = "/some/file"
+ @file = make_absolute("/some/file")
+ @userconfig = make_absolute("/test/userconfigfile")
@settings.define_settings :section, :user => { :default => "suser", :desc => "doc" }, :group => { :default => "sgroup", :desc => "doc" }
@settings.define_settings :section,
- :config => { :type => :file, :default => "/some/file", :desc => "eh" },
+ :config => { :type => :file, :default => @file, :desc => "eh" },
:_one_ => { :default => "ONE", :desc => "a" },
:two => { :default => "$one TWO", :desc => "b" },
:three => { :default => "$one $two THREE", :desc => "c" }
- @settings.stubs(:user_config_file).returns("/test/userconfigfile")
- FileTest.stubs(:exist?).with("/some/file").returns true
- FileTest.stubs(:exist?).with("/test/userconfigfile").returns false
+ @settings.stubs(:user_config_file).returns(@userconfig)
+ FileTest.stubs(:exist?).with(@file).returns true
+ FileTest.stubs(:exist?).with(@userconfig).returns false
end
it "should not ignore the report setting" do
@settings.define_settings :section, :report => { :default => "false", :desc => "a" }
# This is needed in order to make sure we pass on windows
- myfile = File.expand_path("/my/file")
+ myfile = File.expand_path(@file)
@settings[:config] = myfile
text = <<-CONF
[puppetd]
@@ -704,9 +705,9 @@
end
it "should not try to parse non-existent files" do
- FileTest.expects(:exist?).with("/some/file").returns false
+ FileTest.expects(:exist?).with(@file).returns false
- File.expects(:read).with("/some/file").never
+ File.expects(:read).with(@file).never
@settings.parse
end
@@ -748,7 +749,7 @@
end
it "should support specifying all metadata (owner, group, mode) in the configuration file" do
- @settings.define_settings :section, :myfile => { :type => :file, :default => "/myfile", :desc => "a" }
+ @settings.define_settings :section, :myfile => { :type => :file, :default => make_absolute("/myfile"), :desc => "a" }
otherfile = make_absolute("/other/file")
text = "[main]
@@ -761,13 +762,12 @@
end
it "should support specifying a single piece of metadata (owner, group, or mode) in the configuration file" do
- @settings.define_settings :section, :myfile => { :type => :file, :default => "/myfile", :desc => "a" }
+ @settings.define_settings :section, :myfile => { :type => :file, :default => make_absolute("/myfile"), :desc => "a" }
otherfile = make_absolute("/other/file")
text = "[main]
myfile = #{otherfile} {owner = service}
"
- file = "/some/file"
@settings.expects(:read_file).returns(text)
@settings.parse
@settings[:myfile].should == otherfile
@@ -888,20 +888,22 @@
describe "when reparsing its configuration" do
before do
+ @file = make_absolute("/test/file")
+ @userconfig = make_absolute("/test/userconfigfile")
@settings = Puppet::Util::Settings.new
@settings.define_settings :section,
- :config => { :type => :file, :default => "/test/file", :desc => "a" },
+ :config => { :type => :file, :default => @file, :desc => "a" },
:_one_ => { :default => "ONE", :desc => "a" },
:two => { :default => "$one TWO", :desc => "b" },
:three => { :default => "$one $two THREE", :desc => "c" }
- FileTest.stubs(:exist?).with("/test/file").returns true
- FileTest.stubs(:exist?).with("/test/userconfigfile").returns false
- @settings.stubs(:user_config_file).returns("/test/userconfigfile")
+ FileTest.stubs(:exist?).with(@file).returns true
+ FileTest.stubs(:exist?).with(@userconfig).returns false
+ @settings.stubs(:user_config_file).returns(@userconfig)
end
it "should use a LoadedFile instance to determine if the file has changed" do
file = mock 'file'
- Puppet::Util::LoadedFile.expects(:new).with("/test/file").returns file
+ Puppet::Util::LoadedFile.expects(:new).with(@file).returns file
file.expects(:changed?)
@@ -910,7 +912,7 @@
end
it "should not create the LoadedFile instance and should not parse if the file does not exist" do
- FileTest.expects(:exist?).with("/test/file").returns false
+ FileTest.expects(:exist?).with(@file).returns false
Puppet::Util::LoadedFile.expects(:new).never
@settings.expects(:parse).never
@@ -920,7 +922,7 @@
it "should not reparse if the file has not changed" do
file = mock 'file'
- Puppet::Util::LoadedFile.expects(:new).with("/test/file").returns file
+ Puppet::Util::LoadedFile.expects(:new).with(@file).returns file
file.expects(:changed?).returns false
@@ -930,8 +932,8 @@
end
it "should reparse if the file has changed" do
- file = stub 'file', :file => "/test/file"
- Puppet::Util::LoadedFile.expects(:new).with("/test/file").returns file
+ file = stub 'file', :file => @file
+ Puppet::Util::LoadedFile.expects(:new).with(@file).returns file
file.expects(:changed?).returns true
@@ -945,7 +947,7 @@
text = "[main]\none = disk-init\n"
file = mock 'file'
file.stubs(:changed?).returns(true)
- file.stubs(:file).returns("/test/file")
+ file.stubs(:file).returns(@file)
@settings[:one] = "init"
@settings.files = [file]
@@ -992,13 +994,13 @@
it "should retain in-memory values if the file has a syntax error" do
# Init the value
text = "[main]\none = initial-value\n"
- @settings.expects(:read_file).with("/test/file").returns(text)
+ @settings.expects(:read_file).with(@file).returns(text)
@settings.parse
@settings[:one].should == "initial-value"
# Now replace the value with something bogus
text = "[main]\nkenny = killed-by-what-follows\n1 is 2, blah blah florp\n"
- @settings.expects(:read_file).with("/test/file").returns(text)
+ @settings.expects(:read_file).with(@file).returns(text)
@settings.parse
# The originally-overridden value should not be replaced with the default
@@ -1182,12 +1184,12 @@
@settings.stubs(:service_user_available?).returns true
@settings.define_settings :main, :noop => { :default => false, :desc => "", :type => :boolean }
@settings.define_settings :main,
- :maindir => { :type => :directory, :default => "/maindir", :desc => "a" },
- :seconddir => { :type => :directory, :default => "/seconddir", :desc => "a"}
+ :maindir => { :type => :directory, :default => make_absolute("/maindir"), :desc => "a" },
+ :seconddir => { :type => :directory, :default => make_absolute("/seconddir"), :desc => "a"}
@settings.define_settings :main, :user => { :default => "suser", :desc => "doc" }, :group => { :default => "sgroup", :desc => "doc" }
- @settings.define_settings :other, :otherdir => {:type => :directory, :default => "/otherdir", :desc => "a", :owner => "service", :group => "service", :mode => 0755}
- @settings.define_settings :third, :thirddir => { :type => :directory, :default => "/thirddir", :desc => "b"}
- @settings.define_settings :files, :myfile => {:type => :file, :default => "/myfile", :desc => "a", :mode => 0755}
+ @settings.define_settings :other, :otherdir => {:type => :directory, :default => make_absolute("/otherdir"), :desc => "a", :owner => "service", :group => "service", :mode => 0755}
+ @settings.define_settings :third, :thirddir => { :type => :directory, :default => make_absolute("/thirddir"), :desc => "b"}
+ @settings.define_settings :files, :myfile => {:type => :file, :default => make_absolute("/myfile"), :desc => "a", :mode => 0755}
end
it "should provide a method that writes files with the correct modes" do
@@ -1196,7 +1198,7 @@
it "should provide a method that creates directories with the correct modes" do
Puppet::Util::SUIDManager.expects(:asuser).with("suser", "sgroup").yields
- Dir.expects(:mkdir).with("/otherdir", 0755)
+ Dir.expects(:mkdir).with(make_absolute("/otherdir"), 0755)
@settings.mkdir(:otherdir)
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" 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-dev?hl=en.
