group: convert to properties with descriptions and improve comma separated parsing

lib/chef/resource/group.rb

@@ -1,7 +1,7 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
 # Author:: Tyler Cloke (<tyler@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -27,38 +27,31 @@ class Group < Chef::Resource
       allowed_actions :create, :remove, :modify, :manage
       default_action :create
 
-      def initialize(name, run_context = nil)
-        super
-        @members = []
-        @excluded_members = []
-      end
+      property :group_name, String,
+               name_property: true, identity: true,
+               description: "The name of the group."
 
-      property :group_name, String, name_property: true, identity: true
-      property :gid, [ String, Integer ]
+      property :gid, [ String, Integer ],
+               description: "The identifier for the group."
 
-      def members(arg = nil)
-        converted_members = arg.is_a?(String) ? arg.split(",") : arg
-        set_or_return(
-          :members,
-          converted_members,
-          kind_of: [ Array ]
-        )
-      end
+      property :members, [String, Array], default: lazy { [] },
+               coerce: proc { |arg| arg.is_a?(String) ? arg.split(/\s*,\s*/) : arg },
+               description: "Which users should be set or appended to a group. This can be either an array or a comma separated list."
 
-      alias_method :users, :members
+      property :excluded_members, [String, Array], default: lazy { [] },
+               coerce: proc { |arg| arg.is_a?(String) ? arg.split(/\s*,\s*/) : arg },
+               description: "Remove users from a group. May only be used when append is set to true."
+
+      property :append, [ TrueClass, FalseClass ], default: false,
+               description: "How members should be appended and/or removed from a group. When true, members are appended and excluded_members are removed. When false, group members are reset to the value of the members property."
 
-      def excluded_members(arg = nil)
-        converted_members = arg.is_a?(String) ? arg.split(",") : arg
-        set_or_return(
-          :excluded_members,
-          converted_members,
-          kind_of: [ Array ]
-        )
-      end
+      property :system, [ TrueClass, FalseClass ], default: false,
+               description: "Sets the group to belong to the system group."
 
-      property :append, [ TrueClass, FalseClass ], default: false
-      property :system, [ TrueClass, FalseClass ], default: false
-      property :non_unique, [ TrueClass, FalseClass ], default: false
+      property :non_unique, [ TrueClass, FalseClass ], default: false,
+               description: "Allow gid duplication. May only be used with the Groupadd provider."
+
+      alias_method :users, :members
     end
   end
 end

spec/functional/resource/group_spec.rb

@@ -1,7 +1,7 @@
 #
 # Author:: Chirag Jog (<chirag@clogeny.com>)
 # Author:: Siddheshwar More (<siddheshwar.more@clogeny.com>)
-# Copyright:: Copyright 2013-2016, Chef Software Inc.
+# Copyright:: Copyright 2013-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -21,16 +21,12 @@
 require "functional/resource/base"
 require "chef/mixin/shell_out"
 
-# Chef::Resource::Group are turned off on Mac OS X 10.6 due to caching
-# issues around Etc.getgrnam() not picking up the group membership
-# changes that are done on the system. Etc.endgrent is not functioning
-# correctly on certain 10.6 boxes.
-describe Chef::Resource::Group, :requires_root_or_running_windows, :not_supported_on_mac_osx_106 do
+describe Chef::Resource::Group, :requires_root_or_running_windows do
   include Chef::Mixin::ShellOut
 
   def group_should_exist(group)
-    case ohai[:platform_family]
-    when "debian", "fedora", "rhel", "suse", "gentoo", "slackware", "arch"
+    case ohai[:os]
+    when "linux"
       expect { Etc.getgrnam(group) }.not_to raise_error
       expect(group).to eq(Etc.getgrnam(group).name)
     when "windows"
@@ -54,8 +50,8 @@ def user_exist_in_group?(user)
   end
 
   def group_should_not_exist(group)
-    case ohai[:platform_family]
-    when "debian", "fedora", "rhel", "suse", "gentoo", "slackware", "arch"
+    case ohai[:os]
+    when "linux"
       expect { Etc.getgrnam(group) }.to raise_error(ArgumentError, "can't find group for #{group}")
     when "windows"
       expect { Chef::Util::Windows::NetGroup.new(group).local_get_members }.to raise_error(ArgumentError, /The group name could not be found./)
@@ -297,8 +293,8 @@ def create_group
   end
 
   let(:group_name) { "group#{SecureRandom.random_number(9999)}" }
-  let(:included_members) { nil }
-  let(:excluded_members) { nil }
+  let(:included_members) { [] }
+  let(:excluded_members) { [] }
   let(:group_resource) do
     group = Chef::Resource::Group.new(group_name, run_context)
     group.members(included_members)

spec/unit/resource/group_spec.rb

@@ -1,7 +1,7 @@
 #
-# Author:: AJ Christensen (<aj@chef.io>)
+# Author:: AJ Christensen (<aj@junglistheavy.industries>)
 # Author:: Tyler Cloke (<tyler@chef.io>);
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -72,7 +72,7 @@
   end
 
   it "does not allow a hash" do
-    expect { resource.send(:group_name, { aj: "is freakin awesome" }) }.to raise_error(ArgumentError)
+    expect { resource.send(:group_name, { some_other_user: "is freakin awesome" }) }.to raise_error(ArgumentError)
   end
 end
 
@@ -85,31 +85,31 @@
   end
 
   it "does not allow a hash" do
-    expect { resource.send(:gid, { aj: "is freakin awesome" }) }.to raise_error(ArgumentError)
+    expect { resource.send(:gid, { some_other_user: "is freakin awesome" }) }.to raise_error(ArgumentError)
   end
 end
 
 describe Chef::Resource::Group, "members" do
   let(:resource) { Chef::Resource::Group.new("fakey_fakerton") }
 
   [ :users, :members].each do |method|
-    it "(#{method}) allows and convert a string" do
-      resource.send(method, "aj")
-      expect(resource.send(method)).to eql(["aj"])
+    it "(#{method}) allows a String and coerces it to an Array" do
+      resource.send(method, "some_user")
+      expect(resource.send(method)).to eql(["some_user"])
     end
 
-    it "(#{method}) should split a string on commas" do
-      resource.send(method, "aj,adam")
-      expect(resource.send(method)).to eql( %w{aj adam} )
+    it "(#{method}) coerces a comma separated list of users to an Array" do
+      resource.send(method, "some_user, other_user ,another_user,just_one_more_user")
+      expect(resource.send(method)).to eql( %w{some_user other_user another_user just_one_more_user} )
     end
 
-    it "(#{method}) allows an array" do
-      resource.send(method, %w{aj adam})
-      expect(resource.send(method)).to eql( %w{aj adam} )
+    it "(#{method}) allows an Array" do
+      resource.send(method, %w{some_user other_user})
+      expect(resource.send(method)).to eql( %w{some_user other_user} )
     end
 
-    it "(#{method}) does not allow a hash" do
-      expect { resource.send(method, { aj: "is freakin awesome" }) }.to raise_error(ArgumentError)
+    it "(#{method}) does not allow a Hash" do
+      expect { resource.send(method, { some_user: "is freakin awesome" }) }.to raise_error(ArgumentError)
     end
   end
 end
@@ -127,7 +127,7 @@
   end
 
   it "does not allow a hash" do
-    expect { resource.send(:gid, { aj: "is freakin awesome" }) }.to raise_error(ArgumentError)
+    expect { resource.send(:gid, { some_other_user: "is freakin awesome" }) }.to raise_error(ArgumentError)
   end
 
   describe "when it has members" do