On Mon, 04 Apr 2011 17:13:02 -0700, Max Martin wrote: > > This code has not actually been merged yet, but is currently being reviewed > by Jacob Helwig. Matt and I decided to go ahead and send out this e-mail to > see if anyone has comments/suggestions/issues, since this is a pretty big > feature and a lot of people have touched the code, so please reply if > anything stands out to you. >
I already gave these comments to Matt & Max, but just to keep everyone
else in the loop:
The original form ended up generating a lot of text to wade through, so
a form like this might be easier to read.
diff --git a/lib/puppet/indirector/certificate_status/file.rb
b/lib/puppet/indirector/certificate_status/file.rb
index e44de1c..092747e 100644
--- a/lib/puppet/indirector/certificate_status/file.rb
+++ b/lib/puppet/indirector/certificate_status/file.rb
@@ -20,12 +20,12 @@ class Puppet::Indirector::CertificateStatus::File <
Puppet::Indirector::Code
Puppet::SSL::Key,
].collect do |part|
if part.indirection.destroy(request.key)
- deleted << "#{part} for host #{request.key} was deleted"
+ deleted << "#{part}"
end
end
return "Nothing was deleted" if deleted.empty?
- deleted.join(", ")
+ "Deleted for '#{request.key}': #{deleted.join(", ")}"
end
def save(request)
Minor grammar nit, details isn't used, also the 'invalid' status
functionality appears to have been lost from an earlier iteration. From
talking with Matt & Max, the code has changed enough that the earlier
approach no longer works. Matt said he'd open a ticket to add the
'invalid' status support.
diff --git a/lib/puppet/ssl/host.rb b/lib/puppet/ssl/host.rb
index d00efd9..d43ed1d 100644
--- a/lib/puppet/ssl/host.rb
+++ b/lib/puppet/ssl/host.rb
@@ -100,7 +100,7 @@ class Puppet::SSL::Host
# Specify how we expect to interact with our certificate authority.
def self.ca_location=(mode)
modes = CA_MODES.collect { |m, vals| m.to_s }.join(", ")
- raise ArgumentError, "CA Mode can only be #{modes}" unless
CA_MODES.include?(mode)
+ raise ArgumentError, "CA Mode can only be one of: #{modes}" unless
CA_MODES.include?(mode)
@ca_location = mode
@@ -302,11 +302,11 @@ class Puppet::SSL::Host
begin
Puppet::SSL::CertificateAuthority.new.verify(my_cert)
return 'signed'
- rescue Puppet::SSL::CertificateAuthority::CertificateVerificationError =>
details
+ # Couldn't this also mean that it's invalid?
+ rescue Puppet::SSL::CertificateAuthority::CertificateVerificationError
return 'revoked'
end
end
-
end
require 'puppet/ssl/certificate_authority'
There's an easier way to create a temp directory, updating tests to
reflect the message change earlier, and using the two argument form of
'.should raise_error'.
diff --git a/spec/unit/indirector/certificate_status/file_spec.rb
b/spec/unit/indirector/certificate_status/file_spec.rb
index 0a090cf..ab10343 100644
--- a/spec/unit/indirector/certificate_status/file_spec.rb
+++ b/spec/unit/indirector/certificate_status/file_spec.rb
@@ -6,16 +6,15 @@ require 'puppet/indirector/certificate_status'
require 'tempfile'
describe "Puppet::Indirector::CertificateStatus::File" do
+ include PuppetSpec::Files
+
before do
Puppet::SSL::CertificateAuthority.stubs(:ca?).returns true
@terminus = Puppet::SSL::Host.indirection.terminus(:file)
- @tmpdir = Tempfile.new("certificate_status_ca_testing")
- @tmpdir.close
- File.unlink(@tmpdir.path)
- Dir.mkdir(@tmpdir.path)
- Puppet[:confdir] = @tmpdir.path
- Puppet[:vardir] = @tmpdir.path
+ @tmpdir = tmpdir("certificate_status_ca_testing")
+ Puppet[:confdir] = @tmpdir
+ Puppet[:vardir] = @tmpdir
# localcacert is where each client stores the CA certificate
# cacert is where the master stores the CA certificate
@@ -61,7 +60,7 @@ describe "Puppet::Indirector::CertificateStatus::File" do
describe "when creating the CA" do
it "should fail if it is not a valid CA" do
Puppet::SSL::CertificateAuthority.expects(:ca?).returns false
- lambda { @terminus.ca }.should raise_error(ArgumentError)
+ lambda { @terminus.ca }.should raise_error(ArgumentError, "This process
is not configured as a certificate authority")
end
end
@@ -158,12 +157,12 @@ describe "Puppet::Indirector::CertificateStatus::File" do
signed_host = Puppet::SSL::Host.new("clean_signed_cert")
generate_signed_cert(signed_host)
signed_request = Puppet::Indirector::Request.new(:certificate_status,
:delete, "clean_signed_cert", signed_host)
- @terminus.destroy(signed_request).should == "Puppet::SSL::Certificate
for host clean_signed_cert was deleted, Puppet::SSL::Key for host
clean_signed_cert was deleted"
+ @terminus.destroy(signed_request).should == "Deleted for host
clean_signed_cert: Puppet::SSL::Certificate, Puppet::SSL::Key"
requested_host = Puppet::SSL::Host.new("clean_csr")
generate_csr(requested_host)
csr_request = Puppet::Indirector::Request.new(:certificate_status,
:delete, "clean_csr", requested_host)
- @terminus.destroy(csr_request).should ==
"Puppet::SSL::CertificateRequest for host clean_csr was deleted,
Puppet::SSL::Key for host clean_csr was deleted"
+ @terminus.destroy(csr_request).should == "Deleted for host
clean_signed_cert: Puppet::SSL::CertificateRequest, Puppet::SSL::Key"
end
end
@@ -186,5 +185,4 @@ describe "Puppet::Indirector::CertificateStatus::File" do
results.should ==
[["ca","signed"],["requested_host","requested"],["revoked_host","revoked"],["signed_host","signed"]]
end
end
-
end
--
Jacob Helwig
signature.asc
Description: Digital signature
