Remove Chef 12-isms from the apt_repository resource

We set a lot of nil defaults and did coersions here that are no longer necessary. Since we did accept nil in a lot of these properties at one point we need to keep it so I wrote a note to future me about that. I also added some specs for the remaining coersion and the defaults.

Signed-off-by: Tim Smith <tsmith@chef.io>

2 files changed, 45 insertions, 9 deletions

diff --git a/lib/chef/resource/apt_repository.rb b/lib/chef/resource/apt_repository.rb
index 8b0d18f6f3..60518131d5 100644
--- a/lib/chef/resource/apt_repository.rb
+++ b/lib/chef/resource/apt_repository.rb
@@ -28,23 +28,28 @@ class Chef
                   " Adding a new repository will update APT package cache immediately."
       introduced "12.9"
 
+      # There's a pile of [ String, nil, FalseClass ] types in these properties.
+      # This goes back to Chef 12 where String didn't default to nil and we had to do
+      # it ourself, which required allowing that type as well. We've cleaned up the
+      # defaults, but since we allowed users to pass nil here we need to continue
+      # to allow that so don't refactor this however tempting it is
       property :repo_name, String,
                regex: [/^[^\/]+$/],
                validation_message: "repo_name property cannot contain a forward slash '/'",
                name_property: true
 
       property :uri, String
-      property :distribution, [ String, nil, false ], default: lazy { node["lsb"]["codename"] }, coerce: proc { |x| x ? x : nil }
+      property :distribution, [ String, nil, FalseClass ], default: lazy { node["lsb"]["codename"] }
       property :components, Array, default: lazy { [] }
-      property :arch, [String, nil, false], default: nil, coerce: proc { |x| x ? x : nil }
+      property :arch, [String, nil, FalseClass]
       property :trusted, [TrueClass, FalseClass], default: false
       # whether or not to add the repository as a source repo, too
       property :deb_src, [TrueClass, FalseClass], default: false
-      property :keyserver, [String, nil, false], default: "keyserver.ubuntu.com", coerce: proc { |x| x ? x : nil }
-      property :key, [String, Array, nil, false], default: lazy { [] }, coerce: proc { |x| x ? Array(x) : nil }
-      property :key_proxy, [String, nil, false], default: nil, coerce: proc { |x| x ? x : nil }
+      property :keyserver, [String, nil, FalseClass], default: "keyserver.ubuntu.com"
+      property :key, [String, Array, nil, FalseClass], default: lazy { [] }, coerce: proc { |x| x ? Array(x) : x }
+      property :key_proxy, [String, nil, FalseClass]
 
-      property :cookbook, [String, nil, false], default: nil, desired_state: false, coerce: proc { |x| x ? x : nil }
+      property :cookbook, [String, nil, FalseClass], desired_state: false
       property :cache_rebuild, [TrueClass, FalseClass], default: true, desired_state: false
 
       default_action :add
diff --git a/spec/unit/resource/apt_repository_spec.rb b/spec/unit/resource/apt_repository_spec.rb
index 296bb89455..bbc80a95c1 100644
--- a/spec/unit/resource/apt_repository_spec.rb
+++ b/spec/unit/resource/apt_repository_spec.rb
@@ -20,14 +20,45 @@ require "spec_helper"
 
 describe Chef::Resource::AptRepository do
   let(:node) { Chef::Node.new }
-  let(:events) { Chef::EventDispatch::Dispatcher.new }
-  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:run_context) do
+    node.automatic[:lsb][:codename] = "superduper"
+    empty_events = Chef::EventDispatch::Dispatcher.new
+    Chef::RunContext.new(node, {}, empty_events)
+  end
   let(:resource) { Chef::Resource::AptRepository.new("multiverse", run_context) }
 
-  it "uses keyserver.ubuntu.com as the keyserver" do
+  it "keyserver defaults to keyserver.ubuntu.com" do
     expect(resource.keyserver).to eql("keyserver.ubuntu.com")
   end
 
+  it "repo_name is the name property" do
+    expect(resource.repo_name).to eql("multiverse")
+  end
+
+  it "distribution defaults to the distro codename" do
+    expect(resource.distribution).to eql("superduper")
+  end
+
+  it "allows setting key to an Array of keys and does not coerce it" do
+    resource.key = %w{key1 key2}
+    expect(resource.key).to eql(%w{key1 key2})
+  end
+
+  it "allows setting key to nil and does not coerce it" do
+    resource.key = nil
+    expect(resource.key).to be_nil
+  end
+
+  it "allows setting key to false and does not coerce it" do
+    resource.key = false
+    expect(resource.key).to be false
+  end
+
+  it "allows setting key to a String and coerces it to an Array" do
+    resource.key = "key1"
+    expect(resource.key).to eql(["key1"])
+  end
+
   it "fails if the user provides a repo_name with a forward slash" do
     expect { resource.repo_name "foo/bar" }.to raise_error(ArgumentError)
   end