Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user defined tag or list of tags #790

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Lightning-
Copy link

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- Lightning- requested review from a team, b4ldr, bastelfreak, ekohl and smortex as code owners July 25, 2023 15:12
@CLAassistant
Copy link

CLAassistant commented Jul 25, 2023

CLA assistant check
All committers have signed the CLA.

@Lightning-
Copy link
Author

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
Copy link
Collaborator

smortex commented Jul 25, 2023

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

@Lightning-
Copy link
Author

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-
Copy link
Author

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-
Copy link
Author

Anyone seeing any more issues?

@b4ldr
Copy link
Collaborator

b4ldr commented Aug 10, 2023

@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-
Copy link
Author

@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-
Copy link
Author

Lightning- commented Aug 30, 2023

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 😇

manifests/init.pp Outdated Show resolved Hide resolved
make it possible to concatenate fragments identified by different tags
into a file; allow parameter "tag" of type "concat_file" to be a string
or array of strings therefore

additionally handle nonexistent tags (as being optional in both native
types) correctly
allow a user defined tag for a fragment; keep previous default behavior
if not supplied
allow a user defined tag or list of tags for fragments to build the
concat_file out of
avoid potential problems and warnings by overlapping with the
metaparameter "tag"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants