puppetlabs / puppetlabs-concat

File concatenation system for Puppet
Apache License 2.0
171 stars 303 forks source link

Allow user defined tag or list of tags #790

Open Lightning- opened 1 year ago

Lightning- commented 1 year ago

Summary

Additional Context

Generating fragments for a target path is one thing but generating snippets in a more "neutral" or "generic" way (not tied to the path) has plenty of additional use cases not covered by this method. Best example are multiple certificate authority hosts exporting a concat_fragment/concat::fragment with a descriptive tag to PuppetDB for each CA certificate they are responsible for. A random manifest then can realize a concat_file/concat gathering a list of tags (e.g. ['cacert_intermediate_2', 'cacert_intermediate_1', 'cacert_root']).
Actually that was my initial use case when I was patching the native types many years ago and I'm running these patches for years now ;). Time to contribute!

I kept the original behavior of the defined types by making the tag parameter(s) optional and stick with the current default(s) if not used. That way it's a bare feature addition that shouldn't break any scenario out there.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

Lightning- commented 1 year ago

I wanna add a comment to unit testing and acceptance testing: I'm actually pretty unsure how to treat this here.
Making the tag a configurable parameter opens up various ways of combining fragments. For example it would be possible to combine a fragment declared for a specific path with a fragment that has the tag X and another one that has the tag Y. In current tests and specs the tag is just ignored. So ... testing might escalate a little bit ;)

smortex commented 1 year ago

This will clash with the tag metaparameter available on all resources. I guess this need another name?

Lightning- commented 1 year ago

This will clash with the tag metaparameter available on all resources. I guess this need another name?

Uhm, is this a clash (actually this was on purpose)? The native types use the tag already that way; I didn't add this, just expanded it a little bit. concat_fragment finally strips the tagging before it creates the regular file, so there's no "pollution" in the end: https://github.com/puppetlabs/puppetlabs-concat/blob/main/lib/puppet/type/concat_file.rb#L343-L349

Lightning- commented 1 year ago

Ahhh, now I'm getting what you're pointing to. Think you wanna avoid the server side warning for the defined types, right?

[puppetserver] Puppet tag is a metaparam; this value will inherit to all contained resources in the concat definition
[puppetserver] Puppet tag is a metaparam; this value will inherit to all contained resources in the concat::fragment definition

I was trying to stick the param names as close as possible to the way the native code works. IMHO this makes it more understandable.
But for sure we could change the naming here to e.g. tagging if you prefere it that way :).

Lightning- commented 1 year ago

Anyone seeing any more issues?

b4ldr commented 1 year ago

@Lightning- i think i may be missing something how is this new parameter different from the current tag metaparameter? i.e. you can currently do

node "exporting_host" {
@@concat::fragment { 'foobar':
  path => /foo/bar
  tag => ['foobar']
}
}
node "importing_host" {
  Concat::fragment < tag == 'foobar' >
} 
Lightning- commented 1 year ago

@Lightning- i think i may be missing something how is this new parameter different from the current tag metaparameter? i.e. you can currently do

node "exporting_host" {
@@concat::fragment { 'foobar':
  path => /foo/bar
  tag => ['foobar']
}
}
node "importing_host" {
  Concat::fragment < tag == 'foobar' >
} 

Yes, this works but is not what my change is about: concatenating a single one (concat)file from fragments with different tags is the important change here. The changes to the native types are the relevant feature addition.

Basically explained here: https://github.com/Lightning-/puppetlabs-concat/blob/feature_taglist/lib/puppet/type/concat_file.rb#L26-L34

Lightning- commented 1 year ago

I don't wanna be annoying but may I kindly ask if we could get this processed? I'd love to get the official repo aligned to my branch and move my production environments back to there :innocent: