Cleanup legacy facts and add puppet strings

[cruelsmith]

Pull Request (PR) description

• Fix legacy fact usage
• Fix top var scope usage
• Fix some indent issues
• Add minimal puppet strings formatting

JFYI: REFERENCE.md can be updated after this PR via

# setup bundle
bundle config set --local path "vendor/bundle"
bundle config set --local without "system_tests"
bundle install --jobs $(nproc)

# generate REFERENCE.md
bundle exec puppet strings generate --format markdown --out REFERENCE.md

This Pull Request (PR) fixes the following issues

None

[djjudas21]

@cruelsmith I just merged #198 so could you rebase this and fix conflicts please. Thanks

[cruelsmith]

I also found some heredocs that is could not fix since the parameter comma was before the end of the heredoc. Best would be to remove the heredoc usage at all.

puppet-freeradius/manifests/blank.pp

Line 14 in ca53e00

content => @(BLANK/L),

puppet-freeradius/manifests/radsniff.pp

Line 40 in 54a728c

content => @("SYSCONFIG"),

Interesting was also that you are inconsistent on when to use a variable like $freeradius::fr_version or freeradius::params::fr_version:

puppet-freeradius/manifests/init.pp

Line 26 in 3871bcc

if $freeradius::fr_version !~ /^3/ {

FYI: There are still some PDK warnings that has not been fixed by me. See forexample https://github.com/djjudas21/puppet-freeradius/actions/runs/5879050878/job/15942322938?pr=199#step:4:16
I would recommend to add --fail-on-warnings to .puppet-lint.rc so that the CI fails when also warnings exist. This can be also be added with

Rakefile:
  linter_fail_on_warnings: true

into .sync.yaml to make it persistent on pdk updates.

[cruelsmith]

Removed my require freeradius::params proposal since it will likely break things. At least in my test that i did to migrate the module to use hiera everything of the rspec breaks. Will open a separate issue for that one.
The remaining changes are fine as they are.

Just now saw that the https://github.com/djjudas21/puppet-freeradius/blob/main/README.md?plain=1#L79 provides a description of the paramters that i did not migrate to the puppet strings. Cloud be also be a todo for this PR.

[nward]

I have added a few comments. In general I think this is good tidy up work - thanks @cruelsmith !

I am opposed to adding empty @param lines to the documentation, as this sets a poor precedent I think. Instead we should either:

1. write documentation
2. mark them to be ignored, require documentation with PRs introducing new params, and create some issues to go and write documentation for the existing ones (perhaps as smaller PRs rather than one big PR addressing the whole lot)

[nward]

My suggestion is that here (and for all the other indent changes where this is relevant) instead of jumping straight from 2 to 6 space indent, we include the intermediate indent on its own line, so that it is clear why this is happening.

Suggested change

	Optional[Enum[
	Optional[
	Enum[

[nward]

Hi @cruelsmith - lots of great work here, thank you very much.

On the documentation commit:
Great stuff. I note that there are some params documentation lines with no text, just the prefix. Can we remove these, or if they are easy (lots of them are source, ensure, etc. which will be the same across the board) let's add in some documentation.

In the cases where adding documentation for a param is going to be a bit more work than you want to put in (and you have put in a lot, so I am not asking you to put in any more!) let's remove the documentation prefix line and then either:

1. suppress the warning with the linter comment
2. accept that the warning will be generated

My preference is (2) because it will encourage us to actually go and sort out those few documentation lines, however as you are clearly focussed on removing linter warnings I think (1) would be fine as well - as we can very easily grep for suppressed documentation lines, and reference them in an issue.

I see you have copied the documentation from the current README verbatim which is probably the right approach here. Separate to this, I think we need to make a conscious decision re. the documentation to either:

1. document how FreeRADIUS will behave when the parameter is used
2. document how this module will behave when the parameter is used

I appreciate that we currently do (1), and I am not proposing we change that in this PR, but we should have a discussion about it. I think we would be well advised to use (2), and simply say that x parameter will set y parameter in z module, rather than try and track what FreeRADIUS behaviour is and how it changes over times and between different versions.

I will create an issue to discuss this.

I will also create an issue to proof read the params documentation - there are a few little typos and things but these should not be your responsibility :-)

On the radsniff commit:
I will need to review this further, as there was some reason that this weird logic (and I accept that it is pretty weird) existed.
I am not clear why this commit is part of this PR - can we break it out in to a separate PR instead, targeted at tidying up radsniff specifically, to keep the changes per PR low and easier to review?

[cruelsmith]

Removing the @param without comments again is not an option. When they are removed the puppet strings generation would skip these parameters and the resulting REFERENCE.md would also miss them. Which means nobody will know that they exist at all without also reading the code itself.
In my 👀 doing a review with #205 is sufficient for this.

That strange radsniff stuff is also touched with #202. Both remove that unneeded / not nice used logic.
Yes could also do a separated PR for that one. Since you asked and i was on it i added it here.

[nward]

Removing the @param without comments again is not an option. When they are removed the puppet strings generation would skip these parameters and the resulting REFERENCE.md would also miss them. Which means nobody will know that they exist at all without also reading the code itself.

Ah, I see. Perhaps we can add some # TODO: add docs comments to the end of the empty params lines, so they are easy to find in the future?

Yes let's break the radsniff rework in to a separate PR. For clarity, is your suggestion that I do that, or, did you want to do that?

[cruelsmith]

Rebased and moved the radsniff rework to #208 instead.