Add Hiera overrides

[ekohl]

It is possible to explode the Hiera overrides and generate a table in the reference.

This is an incomplete draft, but does work at least with the reference. https://github.com/voxpupuli/puppet-openvmtools/pull/42 is an example.

I know that the place where Hiera data is loaded is wrong. It's inefficient to load it for every class while it should only be loaded once per module. It also has no tests and doesn't generate anything in HTML or in JSON.

Another thing to verify is whether the Forge can render the details. Currently I've only tested it on Github. On Github it renders like this:

Note that the details can be expanded but are collapsed by default to deal with the potentially large tables.

Fixes #250

[CLAassistant]

All committers have signed the CLA.

[ekohl]

Also other cases that should be tested:

• Modules without Hiera
• Graceful fallback if legacy Hiera data is used
• Data files without interpolation (data/common.yaml)
• Rendering of complex data (hashes etc)

I can't commit to actually finishing all of that, but would love to collaborate on it. Also wouldn't mind if someone else takes this work and brings it to the finish line.

[ekohl]

Other test cases:

unbound::service_name: unbound
unbound::restart_cmd: "systemctl restart %{lookup('unbound::service_name')}"

Another would be to define the merge strategy with for example hashes and arrays.

[b4ldr]

Modules without Hiera

I think the patch already works wit that or am i missing something?

Other test cases:

unbound::service_name: unbound
unbound::restart_cmd: "systemctl restart %{lookup('unbound::service_name')}"

Personally i think its fine to display the raw value without doing the interpolation i.e. "systemctl restart %{lookup('unbound::service_name')}" instead of "systemctl restart unbond".

if there is a need to do interpolation then i think we really should look at reusing Puppet.lookup somehow otherwise we will just end up with a hiera5 lite lib.

Rendering of complex data (hashes etc)

can you give an example?

[ekohl]

I think the patch already works wit that or am i missing something?

I mentioned it as a testcase. When I hacked it up I only tested it with a module that has data. I wrote some code, but it deserves some testing.

Personally i think its fine to display the raw value without doing the interpolation i.e. "systemctl restart %{lookup('unbound::service_name')}" instead of "systemctl restart unbond".

I agree, but the question is how well it renders.

if there is a need to do interpolation then i think we really should look at reusing Puppet.lookup somehow otherwise we will just end up with a hiera5 lite lib.

I think that would actually be a downside. The lookup happens at runtime so the user should know this. Otherwise they may be surprised when the change the value that's looked up.

Rendering of complex data (hashes etc)

can you give an example?

My test cases until now always had a small string. In the table that rendered fine. If you have a large hash, the rendering may end up completely unreadable.

[b4ldr]

I wrote some code, but it deserves some testing.

Yes sure, same, haven't even though about spec test yet :)

Personally i think its fine to display the raw value without doing the interpolation i.e. "systemctl restart %{lookup('unbound::service_name')}" instead of "systemctl restart unbond".

I agree, but the question is how well it renders.

on this on the other the other hiera point i think we agree

My test cases until now always had a small string. In the table that rendered fine. If you have a large hash, the rendering may end up completely unreadable.

yes could be, but could also argue that if the input hash is that big then you probably need to refactor (i know massive sweeping statement)

[chelnak]

@ekohl - what are your thoughts on this PR? Are you still interested in moving it forward?

[ekohl]

I'd love to see it move forward, but given my current workload I can't say I'll have the time any time soon. I've rebased it to see about the tests, but I'd love it if someone could take over.

[chelnak]

Cool. Yeah I would like to see this move forward too.

I'll note that it is up for grabs..

Maybe it's something I'll try to circle back on once I've finished my current batch of work.

[chelnak]

@ekohl Would their be anyone from vox that would like to move this PR forward?

If not, I think closing it would be a good idea since it's going to fall behind.

[david22swan]

@ekohl We are currently taking a look into this and were interested in knowing more about this. Is it something that you are interested in moving forward yourself and if not is it something that you would be willing to give over for others to finish. If you no longer have the bandwidth to finish this work then how close would you say it is to being ready to publish?

[ekohl]

Is it something that you are interested in moving forward yourself and if not is it something that you would be willing to give over for others to finish. If you no longer have the bandwidth to finish this work then how close would you say it is to being ready to publish?

I still think it's a worthy addition, but currently don't have the bandwidth to finish this myself. If anyone is willing to take it over, that would be great.

It probably needs verification in various scenarios (since I did only minimal testing) and someone needs to think about the formats. Right now it's limited to HTML. There's a commented line in the to_hash that I think solves it, but I didn't put any work in trying that out.

[jordanbreen28]

hey @ekohl - we've began a little investigation into the work still outstanding in this PR to see if we can help get this over the line for you, so I was just hoping to clarify a couple of things with you:

1. As it sits at the moment, bundle exec puppet strings generate --format markdown --out REFERENCE.md seems to error for me with Error: Processing plans/acceptance/pe_server.pp:7 with /path/to/puppet-strings/lib/puppet-strings/markdown/templates/classes_and_defines.erb => no implicit conversion of String into Integer. Fully aware this PR is not complete by any stretch, so just checking if this is something you are aware of, or if something has changed and broken since you last worked on this. They are bolt plans that I'm seeing this issue with.

2. From my understanding of the examples used this is limited to just the markdown format at the moment is that correct?

Once that's cleared up I can go back to the team and see about picking this up and getting it moving again, we much appreciate your help and work on this!

[ekohl]

1. As it sits at the moment, bundle exec puppet strings generate --format markdown --out REFERENCE.md seems to error for me with Error: Processing plans/acceptance/pe_server.pp:7 with /path/to/puppet-strings/lib/puppet-strings/markdown/templates/classes_and_defines.erb => no implicit conversion of String into Integer. Fully aware this PR is not complete by any stretch, so just checking if this is something you are aware of, or if something has changed and broken since you last worked on this. They are bolt plans that I'm seeing this issue with.

I can imagine I missed some things. For example, I didn't expect this part: https://github.com/puppetlabs/puppet-strings/blob/b6cd7823424c3398887f95330696b93d57199314/lib/puppet-strings/markdown/puppet_plan.rb#L12 The filename implies it's only used for classes and defines, so I didn't account for plans. I also don't use plans myself, so i didn't show up in my testing.

2. From my understanding of the examples used this is limited to just the markdown format at the moment is that correct?

Correct, but IMHO it should be considered. I just didn't know what was the best way.

[jordanbreen28]

1. As it sits at the moment, bundle exec puppet strings generate --format markdown --out REFERENCE.md seems to error for me with Error: Processing plans/acceptance/pe_server.pp:7 with /path/to/puppet-strings/lib/puppet-strings/markdown/templates/classes_and_defines.erb => no implicit conversion of String into Integer. Fully aware this PR is not complete by any stretch, so just checking if this is something you are aware of, or if something has changed and broken since you last worked on this. They are bolt plans that I'm seeing this issue with.

I can imagine I missed some things. For example, I didn't expect this part:

https://github.com/puppetlabs/puppet-strings/blob/b6cd7823424c3398887f95330696b93d57199314/lib/puppet-strings/markdown/puppet_plan.rb#L12

The filename implies it's only used for classes and defines, so I didn't account for plans. I also don't use plans myself, so i didn't show up in my testing.

2. From my understanding of the examples used this is limited to just the markdown format at the moment is that correct?

Correct, but IMHO it should be considered. I just didn't know what was the best way.

Thanks for getting back and clarifying! We can start mapping out now how to proceed!