refactoring in order to address #39, #57

  - primary groups are not created via puppet User resource
  - first create users, then groups and add members into groups

.rspec

@@ -1,4 +1,4 @@
 --format documentation
 --color
 --pattern "spec/*/*_spec.rb"
---fail-fast
+#--fail-fast

lib/puppet/parser/functions/accounts_group_members.rb

@@ -25,7 +25,7 @@ module Puppet::Parser::Functions
         res[g]['members'] = [] unless res[g].key?('members')
         res[g]['require'] = [] unless res[g].key?('require')
       end
-      res[g]['members'] << user
+      res[g]['members'] << user unless res[g]['members'].include? user
       res[g]['require'] << "User[#{user}]"
     end
 
@@ -34,28 +34,25 @@ module Puppet::Parser::Functions
     args[0].each do |user, val|
       # don't assign users marked for removal to groups
       next if val.key? 'ensure' and val['ensure'] == 'absent'
+      val['primary_group'] = user.to_s unless val.key? 'primary_group'
+      val['manage_group'] = true unless val.key? 'manage_group'
+      if val['manage_group']
+        g = val['primary_group']
+        assign_helper.call(res, g, user)
+        if val.key? 'gid'
+          res[g]['gid'] = val['gid'] # manually override GID
+        end
+      end
       if val.key? 'groups'
         val['groups'].each do |g|
-          if primary_groups.key? g and primary_group['members'].include? user
-            # don't assign user to his primary group
-            # it would cause a dependency cycle
-          else
-            assign_helper.call(res, g, user) unless primary_groups.key? g
-          end
+          assign_helper.call(res, g, user)
         end
       elsif args.size == 4
         args[3].each do |g|
           assign_helper.call(res, g, user)
         end
       end
     end
-    # make sure any primary group is present
-    args[0].each do |user, val|
-      if val.key? 'primary_group'
-        res.delete(val['primary_group'])
-      end
-    end
-
     res
   end
 

manifests/init.pp

@@ -18,34 +18,32 @@
   $groups_h = hiera_hash('accounts::groups', {})
 
   $_users = merge($users, $users_h)
+  anchor { 'accounts::users_created': }
 
   class{'::accounts::config':
     options => $options,
-  } ->
-  anchor { 'accounts::primary_groups_created': }
+    before => Anchor['accounts::users_created'],
+  }
 
   if $manage_users {
     $udef = merge($user_defaults, {
       home_permissions => $::accounts::params::home_permissions,
-      require => Anchor['accounts::primary_groups_created'],
+      require          => Anchor['accounts::users_created'],
     })
     create_resources(accounts::user, $_users, $udef)
   }
 
   if $manage_groups {
     $_groups = merge($groups, $groups_h)
-    $primary_groups = accounts_primary_groups($_users, $_groups)
-    create_resources(accounts::group, $primary_groups, {
-      before => Anchor['accounts::primary_groups_created'],
-    })
 
     if has_key($user_defaults, 'groups'){
       $default_groups = $user_defaults['groups']
     } else {
       $default_groups = []
     }
     # Merge group definition with user's assignment to groups
-    $members = accounts_group_members($_users, $_groups, $primary_groups, $default_groups)
+    # No anchor is needed, all requirements are defined individially for each resource
+    $members = accounts_group_members($_users, $_groups, {}, $default_groups)
     create_resources(accounts::group, $members)
   }
 }

manifests/user.pp

@@ -104,6 +104,9 @@
   if ($gid) {
     $real_gid = $gid
   } else {
+    # Actuall primary group assignment is done later
+    # intentionally omitting primary group in order to avoid dependency cycles
+    # see https://github.com/deric/puppet-accounts/issues/39
     if $ensure == 'present' and $manage_group == true {
       # choose first non empty argument
       $real_gid = pick($primary_group, $username)
@@ -123,7 +126,7 @@
   }
 
   User<| title == $username |> {
-    gid        => $real_gid,
+   # gid        => $real_gid,
     comment    => $comment,
     managehome => $managehome,
     home       => $home_dir,

spec/acceptance/groups_spec.rb

@@ -16,6 +16,7 @@
           },
           users => {
             'john' => {
+              'gid'     => 1550,
               'shell'   => '/bin/bash',
               'groups'  => ['users', 'engineers'],
               'ssh_key' => {'type' => 'ssh-rsa', 'key' => 'public_ssh_key_xxx' }
@@ -73,5 +74,11 @@
       its(:stdout) { is_expected.to match /158\(engineers\)/ }
       its(:stdout) { is_expected.to match /100\(users\)/ }
     end
+
+    describe command('id -g john') do
+      its(:exit_status) { is_expected.to eq 0 }
+      its(:stdout) { is_expected.to match /1550/ }
+    end
+
   end
-end
+end

spec/acceptance/users_spec.rb

@@ -64,11 +64,17 @@ class {'::accounts':
       it { is_expected.to have_uid 1010 }
     end
 
+    # primary group id
+    describe command('id -g deployer') do
+      its(:exit_status) { is_expected.to eq 0 }
+      its(:stdout) { is_expected.to match /1010/ }
+    end
+
     describe command('getent group deployer') do
       its(:exit_status) { is_expected.to eq 0 }
       its(:stdout) { is_expected.to match /deployer:x:(\d+):deployer/ }
     end
 
   end
 
-end
+end

spec/classes/init_spec.rb

@@ -39,7 +39,7 @@
       'name'   => user,
       'ensure' => 'present',
       'uid'    => uid,
-      'gid'    => grp,
+   #   'gid'    => grp, # TODO: find better way how to check gid
     )}
 
     it { is_expected.to contain_accounts__user(user).with(
@@ -70,7 +70,7 @@
 
     it { is_expected.to contain_user('john').with(
       'comment' => 'John Doe',
-      'gid' => 2001
+    #  'gid' => 2001
     )}
     it_behaves_like 'having account', 'john', nil, 'john', 2001
 
@@ -95,7 +95,7 @@
     it do
       is_expected.to contain_user('john').with(
         'comment' => 'John Doe',
-        'gid' => 'john'
+      #  'gid' => 'john'  # TODO: make sure gid is updated from groups
       )
       is_expected.to contain_file('/home/john').with(
         'ensure' => 'directory',

spec/defines/user_spec.rb

@@ -69,7 +69,7 @@
 
     it { is_expected.to contain_user('foobar').with(
       'uid' => 1001,
-      'gid' => 1001
+    #  'gid' => 1001
     )}
 
     it_behaves_like 'having_home_dir', 'foobar', '1001', '/home/foobar'
@@ -448,9 +448,10 @@
 
     it { is_expected.to contain_user('foo').with(
       'name' => 'foo',
-      'gid'  => 'mygroup',
+    #  'gid'  => 'mygroup',
       'home' => '/home/foo'
     )}
+    # primary_group is processed outside of this (user) scope
 
   end
 

spec/functions/accounts_group_members_spec.rb

@@ -35,6 +35,8 @@
           'sudo' => {'members' => [:foo], 'require'=> ['User[foo]']},
           'bar' => {'members' => [:john],'require'=> ['User[john]']},
           'users' => {'members' => [:foo,:john], 'require'=> ['User[foo]','User[john]']},
+          'foo' => {'members' => [:foo], 'require' => ['User[foo]']},
+          'john' => {'members' => [:john], 'require' => ['User[john]']},
         }
       )
     end
@@ -48,6 +50,8 @@
 
       is_expected.to run.with_params(users, {}, {}).and_return(
         {
+          'alice' => {'members' => [:alice], 'require'=> ['User[alice]']},
+          'bob' => {'members' => [:bob], 'require'=> ['User[bob]']},
           'sudo' => {'members' => [:bob], 'require'=> ['User[bob]']},
           'users' => {
             'members' => [:alice, :bob],
@@ -64,7 +68,9 @@
         foo: { 'primary_group' => 'testgroup', 'manage_group' => true},
       }
 
-      is_expected.to run.with_params(users, {}, {}).and_return({})
+      is_expected.to run.with_params(users, {}, {}).and_return(
+        {'testgroup' => {'members' => [:foo], 'require' => ['User[foo]']}}
+      )
     end
 
     it 'finds group with gid' do
@@ -74,7 +80,9 @@
           'require' => []},
       }
 
-      is_expected.to run.with_params(users, {}, {}).and_return({})
+      is_expected.to run.with_params(users, {}, {}).and_return(
+        {"testgroup"=>{"members"=>[:foo], "require"=>["User[foo]"], "gid"=>123}}
+      )
     end
   end
 end