set group of fragments to gid

[duritong]

gid has now been part of facter for a long time, it is safe to use
it now.

[sathieu]

any new on this PR?

[jhoblitt]

I'm hesitant to assume $::gid is universally present but setting an internal constraint on $::facterversion seems like a lot of complexity for little net functionality.

@hunner Is there a [planned] way to place a dependency on the facter version via metadata.json?

Also, since this PR modifies the public API the README documentation would need to be updated.

[duritong]

I had a quick look at the README and so far only API deprecations are documented. This change doesn't deprecate anything it adds a new feature. If you tell me where you like to have changes like this document, I can adapt the PR

[duritong]

btw: I rebased it into current master

[underscorgan]

Hi @duritong,

Thanks for the contribution! It looks like there aren't actually any docs updates needed since you didn't add any parameters that are exposed through a class/defined type.

This fact was added in facter 2.2, so I think we still need to account for it not being present. Could you wrap the new parameter in something like this and explicitly set $fragment_group to undef if the fact isn't defined?

Thanks!

[duritong]

I updated my pull request as discussed

[underscorgan]

great, thanks @duritong !

[jhoblitt]

uh, this PR changes the public API...

[duritong]

@jhoblitt I specifically asked you where to document it and later @mhaskel agreed with me that we didn't add a new parameter.

So if you want me to document that change, you must be more specific and verbose of what should be added. Thanks.

[underscorgan]

@jhoblitt this only adds in-class variables, no new parameters.

[jhoblitt]

Ignore me -- my case of the flu has been talking. This is 👍

[underscorgan]

feel better!

[sodabrew]

Hi, this means I need Facter 2.0 or higher on all nodes. What's the purpose of this functionality in the first place? I never want the ownership of the concatfragments.sh to be anything other than root.

Edit: oh, I didn't notice this in setup.pp, thanks for including support for older facter! I still don't understand the reason for other ownership though.

  # test on gid fact availability to support older facter versions
  if defined('$gid') ...

[duritong]

It just brings everything in line. Owner and mode are already managed, so why not group?

Additionally, if you don't manage the group, group ownership is in kind of a limbo state, as it's not sure that the group will really be root. Especially, if you copy fragments from a source.
And as you might see from the deprecation messages, you can't actually set the owner, group nor mode it has no effect:

puppetlabs-concat/manifests/fragment.pp

Lines 48 to 56 in 0aa6c63

	if $mode {
	warning('The $mode parameter to concat::fragment is deprecated and has no effect')
	}
	if $owner {
	warning('The $owner parameter to concat::fragment is deprecated and has no effect')
	}
	if $group {
	warning('The $group parameter to concat::fragment is deprecated and has no effect')
	}

And last but not least: We can't hardcode it to root, because this would make the concat module unusable if you run puppet as a unprivileged user. There are tons of valid use cases for that and there is no reason that the concat module requires root to be used.

[sodabrew]

Thanks for the explanation. Looks like it's doing the right thing for me since #280 as a fallback for not having the gid fact on my older nodes.