Skip to content

Loading…

Refactor GH-1540, add specs, resolve windows spec failures #1582

Closed
wants to merge 6 commits into from

3 participants

@adrienthebo
Puppet Labs member

This pull request adds yardoc to a few methods around the parsedfile provider and the fileparsing methods. It refactors the parsedfile find_resource method into resource_for_record and modifies the specs to more cleanly test the desired resolution for http://projects.puppetlabs.com/issues/2251

adrienthebo added some commits
@adrienthebo adrienthebo Revert "Revert "Merge branch 'pull-1540'""
This reverts commit c992fb5.
c417f50
@adrienthebo adrienthebo (#2251) Rewrite parsedfile method names for clarity
This renames the ParsedFile `self.find_resource` method to
`self.resource_for_record` and changes the method return to return the
resource found. It also adds some yardoc for the methods involved.
8018c7c
@adrienthebo adrienthebo (#2251) add specs around resource_for_record 7d49767
@adrienthebo adrienthebo (maint) Mark parsedfile match_providers_with_resources as private 8f7e1b4
@adrienthebo adrienthebo (#2251) correct crontab matching specs
The previous implementation of this spec was exhibiting failures on
windows and was testing behavior out of the scope of the unit test. This
narrows the scope of the given test.
61d96e4
@puppetcla

CLA Signed by adrienthebo on 2011-03-07 21:00:00 -0800

@adrienthebo adrienthebo (maint) Use stub over puppet type object
Using an object of class Puppet::Type::Cron is too error prone, as
instantiation of those objects have a lot of side effects. This replaces
the actual object with a stub to minimize the possible side effects.
a2c1145
@jeffmccune

Is this ready for review?

@adrienthebo
Puppet Labs member

I hope so, yes.

@jeffmccune jeffmccune added a commit that closed this pull request
@jeffmccune jeffmccune Merge branch 'adrienthebo-maint-cron_refactor_resource_for_record'
* adrienthebo-maint-cron_refactor_resource_for_record:
  (maint) Use stub over puppet type object
  (#2251) correct crontab matching specs
  (maint) Mark parsedfile match_providers_with_resources as private
  (#2251) add specs around resource_for_record
  (#2251) Rewrite parsedfile method names for clarity
  Revert "Revert "Merge branch 'pull-1540'""

closes #1582
a9c4cdb
@jeffmccune jeffmccune closed this in a9c4cdb
@jeffmccune

summary: Merged into master as a9c4cdb. This should be released in Puppet 3.2. Thanks again for the contribution!

-Jeff

@adrienthebo adrienthebo added a commit that referenced this pull request
@adrienthebo adrienthebo (maint) Readd execute permission to bin/puppet
While doing testing around GH-1582 I wound up using a shared filesystem
with Windows, which casually stomped the execute permission for
bin/puppet. This commit readds the execute bit.
dea0284
@adrienthebo adrienthebo deleted the adrienthebo:maint-cron_refactor_resource_for_record branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 5, 2013
  1. @adrienthebo

    Revert "Revert "Merge branch 'pull-1540'""

    adrienthebo committed
    This reverts commit c992fb5.
  2. @adrienthebo

    (#2251) Rewrite parsedfile method names for clarity

    adrienthebo committed
    This renames the ParsedFile `self.find_resource` method to
    `self.resource_for_record` and changes the method return to return the
    resource found. It also adds some yardoc for the methods involved.
  3. @adrienthebo
  4. @adrienthebo
  5. @adrienthebo

    (#2251) correct crontab matching specs

    adrienthebo committed
    The previous implementation of this spec was exhibiting failures on
    windows and was testing behavior out of the scope of the unit test. This
    narrows the scope of the given test.
  6. @adrienthebo

    (maint) Use stub over puppet type object

    adrienthebo committed
    Using an object of class Puppet::Type::Cron is too error prone, as
    instantiation of those objects have a lot of side effects. This replaces
    the actual object with a stub to minimize the possible side effects.
View
0 bin/puppet 100755 → 100644
File mode changed.
View
21 lib/puppet/provider/cron/crontab.rb
@@ -88,6 +88,27 @@ def to_line(record)
end
end
+ # Look up a resource with a given name whose user matches a record target
+ #
+ # @api private
+ #
+ # @note This overrides the ParsedFile method for finding resources by name,
+ # so that only records for a given user are matched to resources of the
+ # same user so that orphaned records in other crontabs don't get falsely
+ # matched (#2251)
+ #
+ # @param [Hash<Symbol, Object>] record
+ # @param [Array<Puppet::Resource>] resources
+ #
+ # @return [Puppet::Resource, nil] The resource if found, else nil
+ def self.resource_for_record(record, resources)
+ resource = super
+
+ if resource and record[:target] == resource[:user]
+ resource
+ end
+ end
+
# Return the header placed at the top of each generated file, warning
# users that modifying this file manually is probably a bad idea.
def self.header
View
23 lib/puppet/provider/parsedfile.rb
@@ -217,6 +217,11 @@ def self.prefetch(resources = nil)
match_providers_with_resources(resources)
end
+ # Match a list of catalog resources with provider instances
+ #
+ # @api private
+ #
+ # @param [Array<Puppet::Resource>] resources A list of resources using this class as a provider
def self.match_providers_with_resources(resources)
return unless resources
matchers = resources.dup
@@ -224,11 +229,10 @@ def self.match_providers_with_resources(resources)
# Skip things like comments and blank lines
next if skip_record?(record)
- if name = record[:name] and resource = resources[name]
+ if (resource = resource_for_record(record, resources))
resource.provider = new(record)
elsif respond_to?(:match)
if resource = match(record, matchers)
- # Remove this resource from circulation so we don't unnecessarily try to match
matchers.delete(resource.title)
record[:name] = resource[:name]
resource.provider = new(record)
@@ -237,6 +241,21 @@ def self.match_providers_with_resources(resources)
end
end
+ # Look up a resource based on a parsed file record
+ #
+ # @api private
+ #
+ # @param [Hash<Symbol, Object>] record
+ # @param [Array<Puppet::Resource>] resources
+ #
+ # @return [Puppet::Resource, nil] The resource if found, else nil
+ def self.resource_for_record(record, resources)
+ name = record[:name]
+ if name
+ resources[name]
+ end
+ end
+
def self.prefetch_all_targets(resources)
records = []
targets(resources).each do |target|
View
5 lib/puppet/util/fileparsing.rb
@@ -141,6 +141,11 @@ def handle_text_line(line, record)
end
# Try to match a record.
+ #
+ # @param [String] line The line to be parsed
+ # @param [Puppet::Util::FileType] record The filetype to use for parsing
+ #
+ # @return [Hash<Symbol, Object>] The parsed elements of the line
def handle_record_line(line, record)
ret = nil
if record.respond_to?(:process)
View
3 spec/fixtures/integration/provider/cron/crontab/moved_cronjob_input1
@@ -6,6 +6,9 @@
17-19,22 0-23/2 * * 2 /bin/unnamed_regular_command
+# Puppet Name: My daily failure
+MAILTO=""
+@daily /bin/false
# Puppet Name: Monthly job
SHELL=/bin/sh
MAILTO=mail@company.com
View
5 spec/fixtures/integration/provider/cron/crontab/moved_cronjob_input2
@@ -1,7 +1,6 @@
# HEADER: some simple
# HEADER: header
-# Puppet Name: My daily failure
-MAILTO=""
-@daily /bin/false
# Puppet Name: some_unrelevant job
* * * * * /bin/true
+# Puppet Name: My daily failure
+@daily /bin/false
View
3 spec/integration/provider/cron/crontab_spec.rb
@@ -160,7 +160,7 @@ def expect_output(fixture_name)
run_in_catalog(resource)
expect_output('modify_entry')
end
- it "should be able to move an entry from one file to another" do
+ it "should not try to move an entry from one file to another" do
# force the parsedfile provider to also parse user1's crontab
random_resource = Puppet::Type.type(:cron).new(
:name => 'foo',
@@ -171,6 +171,7 @@ def expect_output(fixture_name)
resource = Puppet::Type.type(:cron).new(
:name => 'My daily failure',
:special => 'daily',
+ :command => "/bin/false",
:target => crontab_user2,
:user => crontab_user2
)
View
57 spec/unit/provider/cron/crontab_spec.rb
@@ -122,8 +122,8 @@ def compare_crontab_record(have, want)
end
context "when adding a cronjob with the same command as an existing job" do
- let(:resource) { Puppet::Type::Cron.new(:name => "test", :user => "root", :command => "/bin/true") }
let(:record) { {:name => "existing", :user => "root", :command => "/bin/true", :record_type => :crontab} }
+ let(:resource) { Puppet::Type::Cron.new(:name => "test", :user => "root", :command => "/bin/true") }
let(:resources) { { "test" => resource } }
before :each do
@@ -148,4 +148,59 @@ def compare_crontab_record(have, want)
subject.prefetch(resources)
end
end
+
+ context "when prefetching an entry now managed for another user" do
+ let(:resource) do
+ s = stub(:resource)
+ s.stubs(:[]).with(:user).returns 'root'
+ s
+ end
+
+ let(:record) { {:name => "test", :user => "nobody", :command => "/bin/true", :record_type => :crontab} }
+ let(:resources) { { "test" => resource } }
+
+ before :each do
+ subject.stubs(:prefetch_all_targets).returns([record])
+ end
+
+ it "should try and use the match method to find a more fitting record" do
+ subject.expects(:match).with(record, resources)
+ subject.prefetch(resources)
+ end
+
+ it "should not match a provider to the resource" do
+ resource.expects(:provider=).never
+ subject.prefetch(resources)
+ end
+
+ it "should not find the resource when looking up the on-disk record" do
+ subject.prefetch(resources)
+ subject.resource_for_record(record, resources).should be_nil
+ end
+ end
+
+ context "when matching resources to existing crontab entries" do
+ let(:first_resource) { Puppet::Type::Cron.new(:name => :one, :user => 'root', :command => '/bin/true') }
+ let(:second_resource) { Puppet::Type::Cron.new(:name => :two, :user => 'nobody', :command => '/bin/false') }
+
+ let(:resources) {{:one => first_resource, :two => second_resource}}
+
+ describe "with a record with a matching name and mismatching user (#2251)" do
+ # Puppet::Resource objects have #should defined on them, so in these
+ # examples we have to use the monkey patched `must` alias for the rspec
+ # `should` method.
+
+ it "doesn't match the record to the resource" do
+ record = {:name => :one, :user => 'notroot', :record_type => :crontab}
+ subject.resource_for_record(record, resources).must be_nil
+ end
+ end
+
+ describe "with a record with a matching name and matching user" do
+ it "matches the record to the resource" do
+ record = {:name => :two, :target => 'nobody', :command => '/bin/false'}
+ subject.resource_for_record(record, resources).must == second_resource
+ end
+ end
+ end
end
View
17 spec/unit/provider/parsedfile_spec.rb
@@ -72,6 +72,23 @@
end
end
+ describe "when matching resources to existing records" do
+ let(:first_resource) { stub(:one, :name => :one) }
+ let(:second_resource) { stub(:two, :name => :two) }
+
+ let(:resources) {{:one => first_resource, :two => second_resource}}
+
+ it "returns a resource if the record name matches the resource name" do
+ record = {:name => :one}
+ subject.resource_for_record(record, resources).should be first_resource
+ end
+
+ it "doesn't return a resource if the record name doesn't match any resource names" do
+ record = {:name => :three}
+ subject.resource_for_record(record, resources).should be_nil
+ end
+ end
+
describe "when flushing a file's records to disk" do
before do
# This way we start with some @records, like we would in real life.
Something went wrong with that request. Please try again.