On Tue, 22 Mar 2011 23:35:11 +0100, Stefan Schulte wrote: > [...]
> + # This method is important for prefetching and is called from the
> parsedfile provider.
> + # We get one record (one line of /etc/services) and a hash of resources
> (what the user
> + # specified in manifests). This hash is build in transaction.rb and uses
> uniqueness_key
> + # as a hashkey.
The last sentence should probably s/build/built/, since it's in the past tense.
> + # Normally the parsedfileprovider loops over every record and uses
> record[:name] to
> + # find a corresponding resources[name]. That works if we only have one
> namevar
> + # because uniqueness_key of this resource will equal record[:name].
> Because we use
> + # a composite key the parsedfile provider would never find a resource that
> matches
> + # a given record.
> + # Even worse: The parsedfileprovider cannot calculate the uniqueness_key
> of a
> + # specific record.
> + def self.match(record,resources)
> + # This should never happen but who knows
> + return false unless name = record[:name] and protocol = record[:protocol]
> +
> + # We now calculate the uniqueness_key of the resource we want to find
> + uniq_key = [name, protocol]
> + resources[uniq_key] # will be nil if the user doesnt manage record
> + end
> +end
The early return looks completely redundant, and given the comment above
it seems a bit like voodoo coding. Is there a specific failure mode you
had in mind that this would cover? If not, it seems like the line and
comment should be removed.
[...]
> diff --git a/spec/fixtures/unit/provider/port/parsed/nonuniq
> b/spec/fixtures/unit/provider/port/parsed/nonuniq
> new file mode 100644
> index 0000000..e4eb25a
> --- /dev/null
> +++ b/spec/fixtures/unit/provider/port/parsed/nonuniq
> @@ -0,0 +1,6 @@
> +# We test a few comments here
> +# and anotherone
> +telnet 23/tcp # Telnet
> +telnets 992/tcp # telnet protocol over TLS/SSL
> +telnets 992/ud
> +telnet 23/udp
Maybe add another comment line here to explain whether 'telnets 992/ud'
is supposed to be a bogus line?
Actually...after reading through the reset of the patch, it doesn't look
like any of the fixtures introduced are actually used. Was this
intentional?
[...]
> + it "should extrace name from the first field" do
> + @provider.parse_line(@example_line)[:name].should == 'telnet'
> + end
[...]
> + it "should extrace protocol tcp from third field" do
> + @provider.parse_line('telnet 23/tcp')[:protocol].should == :tcp
> + end
[...]
> + it "should extrace name from the first field" do
> + @provider.parse_line(@example_line)[:name].should == 'telnet'
> + end
[...]
> + it "should extrace name from the first field" do
> + @provider.parse_line(@example_line)[:name].should == 'telnet'
> + end
[...]
> + it "should extrace name from the first field" do
> + @provider.parse_line(@example_line)[:name].should == @result[0]
> + end
'extrace'? ;)
[...]
> + it "should be able to generate an entry with one alias" do
> + port = mkport(
> + :name => 'pcx-pin',
> + :protocol => :tcp,
> + :number => '4005',
> + :port_aliases => 'pcx-pin',
> + :ensure => :present
> + )
> + genport(port).should == "pcx-pin\t4005/tcp\tpcx-pin\n"
> + end
> +
> + it "should be able to generate an entry with more than one alias" do
> + port = mkport(
> + :name => 'pcx-splr-ft',
> + :protocol => :udp,
> + :number => '4003',
> + :port_aliases => [ 'pcx-splr-ft', 'rquotad' ],
> + :ensure => :present
> + )
> + genport(port).should == "pcx-splr-ft\t4003/udp\tpcx-splr-ft rquotad\n"
> + end
The name and the alias really should be different, so we can make
sure they didn't get mixed up somehow.
[...]
> + it "should be able to generate an entry with more than one alias and a
> comment" do
> + port = mkport(
> + :name => 'foo',
> + :protocol => :udp,
> + :number => '3000',
> + :port_aliases => [ 'bar', 'baz', 'zap' ],
> + :description => 'Bazinga!',
> + :ensure => :present
> + )
> + genport(port).should == "foo\t3000/udp\tbar baz zap\t# Bazinga!\n"
> + end
I think I'm going to have to start using "Bazinga!" more. ;)
--
Jacob Helwig
signature.asc
Description: Digital signature
