changed attribute_membership to auth_members

[crayfishx]

I tracked this down after chasing an issue with the group resource type under certain conditions that fails to recognise a group as in sync if it's members aren't in alphabetical order. The behaviour only happens when attribute_membership is set to inclusive.

After looking at this for a while, it looks as if this is the wrong attribute to use for what is trying to be achieved and is in fact something only really implemented for the AIX provider. This is highlighted by a recent documentation ticket https://tickets.puppetlabs.com/browse/DOCUMENT-356 where the group types docs have now been updated (see puppetlabs/puppet#5765)

I believe changing this to auth_membership => true is correct. (it also solves the strange behaviour I was seeing on RHEL)

[deric]

@crayfishx Thanks for all your effort! It's rather tricky code, it might look simple very simple but when you get under the hood, you'd realize it's much more complicated.

I did a re-write of the module, though I'm still not very happy with it.

I won't be able to accept the PR as it is. I think we should allow changing attribute_membership and maybe even auth_membership for each group. Default values might differ for each OS. Currently this module uses gpasswd commad (which is not available e.g. on Solaris) to enfoce inclusive strategy (list of group members is considered to be complete). Using auth_membership => true might be proper way how to implement expected behavior, without injecting too much code into Puppet.

[crayfishx]

@deric thanks for the detailed response

Your references to where attribute_membership is used makes sense, however, I note that you are using a fork of onyxpoint/gpasswd - if you check the upstream from where you forked, this issue has already been addressed - specifically here:

simp/puppet-gpasswd@af7354a#diff-2c5ae985f9a33a994f52522e9fc47a1eL61

This is still a valid PR IMHO but probably will involve testing against the upstream gpasswd provider?

[deric]

Right, good catch. Basically we were misusing (same as the upstream onyxpoint module in previous versions) the parameter auth_membership because there's no other way how to pass custom attribute to a Puppet resource.

Yeah, the PR is definitely valid, all I'm saying is that when moving forward I'd prefer making parameters configurable:

define accounts::group (
  $groupname       = $title,
  $ensure          = 'present',
  $members         = [],
  $gid             = undef,
  $auth_membership = true,
) {
  ensure_resource('group', $groupname, {
    'ensure'  => $ensure,
    'gid'     => $gid,
    'members' => sort(unique($members)),
    'auth_membership' => $auth_membership,
  })
}

[crayfishx]

Ah ok thats cool - I can add that to this PR

[crayfishx]

@deric I've updated this PR now from your comments

[crayfishx]

Thanks for merging that - you've added this to the 1.6 milestone..... Is there anyway of getting this out in a 1.5 release? 1.6 has some changes that Im not very keen on, especially overriding the Puppet core providers :-(

Shouldn't 1.6 actually be 2.0?

[deric]

1.6 branch is at this point mostly experimental branch.
Overriding user provider isn't nice, but currently it's the only workaround for #77. If there would be parameter for disabling automatic primary group creation I'd happily use it. Probably the best solution would be to move the code to a separate module and allow anyone to choose whether he wants to override a provider or not.

Anyway I'd like to support setting custom providers for both users and groups. There's also membership parameter on User resource. Currently the DSL merges group assignment from user and group level. If we'd follow Puppet's approach it would be probably confusing:

accounts::groups:
  www-data:
    members: ['john', 'trudy']
    membership: inclusive
accounts::users:
  john:
    groups: ["sudo", "users"]
    membership: minimal
  trudy:
    groups: ["sudo", "users"]
    membership: inclusive

you could easily create conflicting assignments. In this case trudy would be removed from www-data and afterwards added again. So I'm not sure such change would be useful for end users.