puppetlabs-concat icon indicating copy to clipboard operation
puppetlabs-concat copied to clipboard

Allow user defined tag or list of tags

Open Lightning- opened this issue 1 year ago • 9 comments

Summary

  • Allow native types to handle a list of tags
    speaking: allow a concat_file to get assembled of fragments matching the list of tags
  • Make the tag (optionally) configurable for the Puppet defined types (concat/concat::fragment)

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.

Lightning- avatar Jul 25 '23 15:07 Lightning-

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 25 '23 15:07 CLAassistant

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 ;)

Lightning- avatar Jul 25 '23 15:07 Lightning-

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

smortex avatar Jul 25 '23 23:07 smortex

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- avatar Jul 26 '23 11:07 Lightning-

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- avatar Jul 26 '23 12:07 Lightning-

Anyone seeing any more issues?

Lightning- avatar Aug 07 '23 08:08 Lightning-

@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' >
} 

b4ldr avatar Aug 10 '23 13:08 b4ldr

@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- avatar Aug 10 '23 13:08 Lightning-

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:

Lightning- avatar Aug 30 '23 08:08 Lightning-