Very nice. +1 On Oct 20, 2009, at 7:56 PM, Markus Roberts wrote:
> > There were a number of problems here (duplicated code, meaningless > tests, etc.) but the core was that the last definition of target > ignored the provided value if there was also a user specified. > > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/provider/ssh_authorized_key/parsed.rb | 36 +---------- > spec/unit/provider/ssh_authorized_key/parsed.rb | 79 +++++++ > +-------------- > 2 files changed, 29 insertions(+), 86 deletions(-) > > diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/ > puppet/provider/ssh_authorized_key/parsed.rb > index 6df7f8a..28a68b3 100644 > --- a/lib/puppet/provider/ssh_authorized_key/parsed.rb > +++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb > @@ -36,14 +36,6 @@ > Puppet::Type.type(:ssh_authorized_key).provide(:parsed, > :rts => /^\s+/, > :match => /^(?:(.+) )?(\d+) (\d+) (\d+)(?: (.+))?$/ > > - def target > - @resource.should(:target) > - end > - > - def user > - @resource.should(:user) > - end > - > def dir_perm > # Determine correct permission for created directory and file > # we can afford more restrictive permissions when the user > is known > @@ -67,39 +59,13 @@ > Puppet::Type.type(:ssh_authorized_key).provide(:parsed, > end > > def target > - if user > - File.expand_path("~%s/.ssh/authorized_keys" % user) > - elsif target = @resource.should(:target) > - target > - end > + @resource.should(:target) || File.expand_path("~%s/.ssh/ > authorized_keys" % user) > end > > def user > @resource.should(:user) > end > > - def dir_perm > - # Determine correct permission for created directory and file > - # we can afford more restrictive permissions when the user > is known > - if target > - if user > - 0700 > - else > - 0755 > - end > - end > - end > - > - def file_perm > - if target > - if user > - 0600 > - else > - 0644 > - end > - end > - end > - > def flush > # As path expansion had to be moved in the provider, we > cannot generate new file > # resources and thus have to chown and chmod here. It smells > hackish. > diff --git a/spec/unit/provider/ssh_authorized_key/parsed.rb b/spec/ > unit/provider/ssh_authorized_key/parsed.rb > index 0f32a6f..ade738b 100755 > --- a/spec/unit/provider/ssh_authorized_key/parsed.rb > +++ b/spec/unit/provider/ssh_authorized_key/parsed.rb > @@ -95,85 +95,61 @@ describe provider_class do > File.stubs(:chown) > end > > - describe "and a user has been specified" do > + describe "and both a user and a target have been specified" > do > before :each do > - @resource.stubs(:should).with(:user).returns "nobody" > - target = File.expand_path("~nobody/.ssh/ > authorized_keys") > + Puppet::Util.stubs(:uid).with("random_bob").returns > 12345 > + @resource.stubs(:should).with(:user).returns > "random_bob" > + target = "/tmp/.ssh_dir/place_to_put_authorized_keys" > @resource.stubs(:should).with(:target).returns target > end > > it "should create the directory" do > - > Dir.expects(:mkdir).with(File.expand_path("~nobody/.ssh"), 0700) > + File.stubs(:exist?).with("/tmp/.ssh_dir").returns > false > + Dir.expects(:mkdir).with("/tmp/.ssh_dir", 0700) > @provider.flush > end > > it "should chown the directory to the user" do > - uid = Puppet::Util.uid("nobody") > - File.expects(:chown).with(uid, nil, > File.expand_path("~nobody/.ssh")) > + uid = Puppet::Util.uid("random_bob") > + File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir") > @provider.flush > end > > it "should chown the key file to the user" do > - uid = Puppet::Util.uid("nobody") > - File.expects(:chown).with(uid, nil, > File.expand_path("~nobody/.ssh/authorized_keys")) > + uid = Puppet::Util.uid("random_bob") > + File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir/ > place_to_put_authorized_keys") > @provider.flush > end > > it "should chmod the key file to 0600" do > - File.expects(:chmod).with(0600, > File.expand_path("~nobody/.ssh/authorized_keys")) > - @provider.flush > - end > - end > - > - describe "and a target has been specified" do > - before :each do > - @resource.stubs(:should).with(:user).returns nil > - @resource.stubs(:should).with(:target).returns "/ > tmp/.ssh/authorized_keys" > - end > - > - it "should make the directory" do > - Dir.expects(:mkdir).with("/tmp/.ssh", 0755) > - @provider.flush > - end > - > - it "should chmod the key file to 0644" do > - File.expects(:chmod).with(0644, "/tmp/.ssh/ > authorized_keys") > + File.expects(:chmod).with(0600, "/tmp/.ssh_dir/ > place_to_put_authorized_keys") > @provider.flush > end > end > > - end > -end > - > -describe provider_class do > - before :each do > - @resource = stub("resource", :name => "foo") > - @resource.stubs(:[]).returns "foo" > - @provider = provider_class.new(@resource) > - end > - > - describe "when flushing" do > - before :each do > - # Stub file and directory operations > - Dir.stubs(:mkdir) > - File.stubs(:chmod) > - File.stubs(:chown) > - end > - > - describe "and a user has been specified" do > + describe "and a user has been specified with no target" do > before :each do > @resource.stubs(:should).with(:user).returns "nobody" > @resource.stubs(:should).with(:target).returns nil > + # > + # I'd like to use random_bob here and something like > + # > + # > File.stubs(:expand_path).with("~random_bob/.ssh").returns "/users/r/ > random_bob/.ssh" > + # > + # but mocha objects strenuously to stubbing > File.expand_path > + # so I'm left with using nobody. > + @dir = File.expand_path("~nobody/.ssh") > end > > it "should create the directory" do > - > Dir.expects(:mkdir).with(File.expand_path("~nobody/.ssh"), 0700) > + File.stubs(:exist?).with(@dir).returns false > + Dir.expects(:mkdir).with(@dir,0700) > @provider.flush > end > > it "should chown the directory to the user" do > uid = Puppet::Util.uid("nobody") > - File.expects(:chown).with(uid, nil, > File.expand_path("~nobody/.ssh")) > + File.expects(:chown).with(uid, nil, @dir) > @provider.flush > end > > @@ -189,19 +165,20 @@ describe provider_class do > end > end > > - describe "and a target has been specified" do > + describe "and a target has been specified with no user" do > before :each do > @resource.stubs(:should).with(:user).returns nil > - @resource.stubs(:should).with(:target).returns "/ > tmp/.ssh/authorized_keys" > + @resource.stubs(:should).with(:target).returns("/ > tmp/.ssh_dir/place_to_put_authorized_keys") > end > > it "should make the directory" do > - Dir.expects(:mkdir).with("/tmp/.ssh", 0755) > + File.stubs(:exist?).with("/tmp/.ssh_dir").returns > false > + Dir.expects(:mkdir).with("/tmp/.ssh_dir", 0755) > @provider.flush > end > > it "should chmod the key file to 0644" do > - File.expects(:chmod).with(0644, "/tmp/.ssh/ > authorized_keys") > + File.expects(:chmod).with(0644, "/tmp/.ssh_dir/ > place_to_put_authorized_keys") > @provider.flush > end > end > -- > 1.6.4 > > > > -- Health is merely the slowest possible rate at which one can die. --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
