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.

Sign up for GitHub

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

Jump to bottom

Handle quotation marks in section names #115

Merged
merged 7 commits into from
Jul 7, 2014
Merged

Handle quotation marks in section names #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
961447d
`be_true` and `be_false` are deprecated
johnsyweb Jun 26, 2014
41fc8bf
Failing spec to describe my problem
johnsyweb Jun 26, 2014
49960e1
Handle quotation marks in section names
johnsyweb Jun 26, 2014
cabc678
Merge branch 'deprecation-warnings' into handle-quotation-marks-in-se…
johnsyweb Jun 26, 2014
bd99244
Test allowing $ signs in section names
johnsyweb Jun 28, 2014
069aa56
Merge remote-tracking branch 'origin/master' into handle-quotation-ma…
johnsyweb Jul 4, 2014
b3fed85
Spec for MODULES-1194
johnsyweb Jul 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/puppet/provider/ini_setting/ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def self.namevar(section_name, setting)
end

def exists?
ini_file.get_value(section, setting)
!ini_file.get_value(section, setting).nil?
end

def create
Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/util/ini_file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ module Puppet
module Util
class IniFile

@@SECTION_REGEX = /^\s*\[([\w\d\.\\\/\-\:\s]*[\w\d\.\\\/\-])\]\s*$/
@@SECTION_REGEX = /^\s*\[([^\]]*)\]\s*$/
Copy link
Contributor

Choose a reason for hiding this comment

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

Following up on some similar discussion around this from #4 (diff) , one example concerning non-sections that I came up with was

[something "long"://aoeu1234]
[key] = value[aoeu]

And it matches correctly :). So +1

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know of a case when a [ would begin a key and not be closed (and which a ] would then have to occur in the value).

Should we exclude = from the section titles? Like [^\]=]*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. We could add a spec for

[something "long"://aoeu1234]
[key] = value[aoeu]

I can't think of a good reason to exclude = from section names. As long as the section name appears on a line by itself, in square brackets, I think we're good.

The only other character I could think of that we may wish to exclude is [, but couldn't find a sensible example as a test case.

@@SETTING_REGEX = /^(\s*)([^\[#;][\w\d\.\\\/\-\s\[\]\']*[\w\d\.\\\/\-\]])([ \t]*=[ \t]*)([\S\s]*?)\s*$/
@@COMMENTED_SETTING_REGEX = /^(\s*)[#;]+(\s*)([^\[]*[\w\d\.\\\/\-]+[\w\d\.\\\/\-\[\]\']+)([ \t]*=[ \t]*)([\S\s]*?)\s*$/

Expand Down
109 changes: 75 additions & 34 deletions spec/unit/puppet/provider/ini_setting/ruby_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:setting => 'yahoo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -172,7 +172,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section:sub', :setting => 'yahoo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -200,7 +200,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:setting => 'baz', :value => 'bazvalue2'))
provider = described_class.new(resource)
provider.exists?.should == 'bazvalue'
provider.exists?.should be true
provider.value=('bazvalue2')
validate_file(<<-EOS
# This is a comment
Expand All @@ -227,7 +227,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section:sub', :setting => 'subby', :value => 'foo'))
provider = described_class.new(resource)
provider.exists?.should == 'bar'
provider.exists?.should be true
provider.value.should == 'bar'
provider.value=('foo')
validate_file(<<-EOS
Expand Down Expand Up @@ -255,7 +255,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:setting => 'url', :value => 'http://192.168.0.1:8080'))
provider = described_class.new(resource)
provider.exists?.should == 'http://192.168.1.1:8080'
provider.exists?.should be true
provider.value.should == 'http://192.168.1.1:8080'
provider.value=('http://192.168.0.1:8080')

Expand Down Expand Up @@ -284,14 +284,14 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:setting => 'baz', :value => 'bazvalue'))
provider = described_class.new(resource)
provider.exists?.should == 'bazvalue'
provider.exists?.should be true
end

it "should add a new section if the section does not exist" do
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => "section3", :setting => 'huzzah', :value => 'shazaam'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -321,7 +321,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => "section:subsection", :setting => 'huzzah', :value => 'shazaam'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -351,7 +351,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => "section1", :setting => 'setting1', :value => 'hellowworld', :path => emptyfile))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file("
[section1]
Expand All @@ -363,7 +363,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => "section:subsection", :setting => 'setting1', :value => 'hellowworld', :path => emptyfile))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file("
[section:subsection]
Expand All @@ -375,7 +375,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => "section1", :setting => 'master', :value => true))
provider = described_class.new(resource)
provider.exists?.should == 'true'
provider.exists?.should be true
provider.value.should == 'true'
end

Expand All @@ -397,7 +397,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => '', :setting => 'bar', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand All @@ -414,7 +414,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => '', :setting => 'foo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should == 'blah'
provider.exists?.should be true
provider.value.should == 'blah'
provider.value=('yippee')
validate_file(<<-EOS
Expand All @@ -431,7 +431,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => '', :setting => 'foo', :value => 'blah'))
provider = described_class.new(resource)
provider.exists?.should == 'blah'
provider.exists?.should be true
end
end

Expand All @@ -447,7 +447,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => '', :setting => 'foo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
foo = yippee
Expand All @@ -462,7 +462,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section2', :setting => 'foo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should == 'http://192.168.1.1:8080'
provider.exists?.should be true
provider.value.should == 'http://192.168.1.1:8080'
provider.value=('yippee')
validate_file(<<-EOS
Expand All @@ -476,7 +476,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section2', :setting => 'bar', :value => 'baz'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section2]
Expand Down Expand Up @@ -522,7 +522,7 @@ def self.file_path
:value => 'yippee',
:key_val_separator => '='))
provider = described_class.new(resource)
provider.exists?.should == 'bar'
provider.exists?.should be true
provider.value.should == 'bar'
provider.value=('yippee')
validate_file(<<-EOS
Expand All @@ -539,7 +539,7 @@ def self.file_path
:value => 'baz',
:key_val_separator => '='))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section2]
Expand Down Expand Up @@ -576,7 +576,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section1', :setting => 'foo', :ensure => 'absent'))
provider = described_class.new(resource)
provider.exists?.should be_true
provider.exists?.should be true
provider.destroy
validate_file(<<-EOS
[section1]
Expand All @@ -601,7 +601,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section:sub', :setting => 'foo', :ensure => 'absent'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.destroy
validate_file(<<-EOS
[section1]
Expand Down Expand Up @@ -652,7 +652,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section1', :setting => 'yahoo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -681,7 +681,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section1', :setting => 'bar', :value => 'barvalue2'))
provider = described_class.new(resource)
provider.exists?.should be_true
provider.exists?.should be true
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -709,7 +709,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section2', :setting => 'yahoo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -738,7 +738,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section2', :setting => 'baz', :value => 'bazvalue2'))
provider = described_class.new(resource)
provider.exists?.should be_true
provider.exists?.should be true
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -766,7 +766,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section:sub', :setting => 'yahoo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -795,7 +795,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section:sub', :setting => 'fleezy', :value => 'flam2'))
provider = described_class.new(resource)
provider.exists?.should be_true
provider.exists?.should be true
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -842,7 +842,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section2', :setting => 'foo', :value => 'foo3'))
provider = described_class.new(resource)
provider.exists?.should be_false
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section1]
Expand All @@ -864,7 +864,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section1', :setting => 'foo', :value => 'foo3'))
provider = described_class.new(resource)
provider.exists?.should be_true
provider.exists?.should be true
provider.create
validate_file(<<-EOS
[section1]
Expand All @@ -885,7 +885,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section2', :setting => 'bar', :value => 'bar3'))
provider = described_class.new(resource)
provider.exists?.should be_false
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section1]
Expand All @@ -907,7 +907,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(
common_params.merge(:section => 'section2', :setting => 'baz', :value => 'bazvalue'))
provider = described_class.new(resource)
provider.exists?.should be_false
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section1]
Expand Down Expand Up @@ -938,7 +938,7 @@ def self.file_path
common_params.merge(:section => 'section1', :setting => 'foo', :value => 'foovalue2')
)
provider = described_class.new(resource)
provider.exists?.should be_false
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section1]
Expand All @@ -953,7 +953,7 @@ def self.file_path
common_params.merge(:section => 'section1', :setting => 'bar', :value => 'barvalue2')
)
provider = described_class.new(resource)
provider.exists?.should be_false
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[section1]
Expand Down Expand Up @@ -991,7 +991,7 @@ def self.file_path
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'section - two', :setting => 'yahoo', :value => 'yippee'))
provider = described_class.new(resource)
provider.exists?.should be_nil
provider.exists?.should be false
provider.create
validate_file(<<-EOS
# This is a comment
Expand Down Expand Up @@ -1019,4 +1019,45 @@ def self.file_path

end

context "when sections have spaces and quotations" do
let(:orig_content) do
<<-EOS
[branch "master"]
remote = origin
merge = refs/heads/master

[alias]
to-deploy = log --merges --grep='pull request' --format='%s (%cN)' origin/production..origin/master
[branch "production"]
remote = origin
merge = refs/heads/production
EOS
end

it "should add a missing setting to the correct section" do
resource = Puppet::Type::Ini_setting.new(common_params.merge(
:section => 'alias',
:setting => 'foo',
:value => 'bar'
))
provider = described_class.new(resource)
provider.exists?.should be false
provider.create
validate_file(<<-EOS
[branch "master"]
remote = origin
merge = refs/heads/master

[alias]
to-deploy = log --merges --grep='pull request' --format='%s (%cN)' origin/production..origin/master
foo = bar
[branch "production"]
remote = origin
merge = refs/heads/production
EOS
)
end

end

end