Reuse existing static code validators

[bastelfreak]

For over a decade, we've had existing tools to do static code checks and syntax validation. Some of them are grouped in puppet-syntax. It does:

• Puppet DSL validation
• erp validation
• erb validation
• Puppet plans validation (in puppet DSL, not YAML plans)

Besides that there are other tools. metadata-json-lint does... linting and syntax check for metadata.json. puppet-lint provides support for puppet DSL linting and rubocop lints ruby files. The checks are mostly configured in https://github.com/puppetlabs/puppetlabs_spec_helper/blob/main/lib/puppetlabs_spec_helper/rake_tasks.rb

best practice in the community is to call those rake tasks. Even the Puppet CI does it: https://github.com/puppetlabs/puppetlabs-stdlib/actions/runs/8241458864/job/22544040246#step:5:1

PDK doesn't do that. Instead it reinvents the wheel: https://github.com/puppetlabs/pdk/tree/main/lib/pdk/validate/puppet

I think this doesn't make much sense and it's also dangerous. pdk is typically run locally but not in CI. But pdk locally will report different results compared to CI pipelines. Also the current approach adds a huge maintainer burden because checks have to be maintained in multiple places.

[gavindidrichsen]

Hi @bastelfreak. Adding this to our weekly discussion this Friday.

[LukasAud]

Hey @bastelfreak, just wanted to shed some light on the reason as to why we have two separate validators here.

Just to clarify, rake tasks used to and still are our current official validation method and we still recommend them as best practices.

The reason why PDK has "re-invented" the wheel in this scenario is because, years ago, we were planning to replace rake tasks with the new system you can see in PDK, so that ultimately PDK would manage validation in all modules CI for us. However, due to unforeseen circumstances, we could not complete this project and it has been sitting idle for quite some time.

Hope this helps to clarify the situation.

[bastelfreak]

Hi,
I am aware of the history, this ticket is more about the future. There's a drift now between the rake tasks and CI environments vs pdk and local testing. We had discussions about pdk and it's acceptance in the community in the past Vox Pupuli monthlies. I think it would make sense for pdk to rely on the existing rake tasks and recommend pdk for local testing and use the rake tasks in CI.

Are there plans for this?

[bastelfreak]

@rwaffen debugged a problem today that relates to this topic:
Vox Pupuli rely on the lint (or lint_fix) rake task to call puppet-lint. So do many CI pipelines. puppetlabs_spec_helper configures some defaults. For example some checks are disabled: https://github.com/puppetlabs/puppetlabs_spec_helper/blob/main/lib/puppetlabs_spec_helper/rake_tasks.rb#L168-L180

Now pdk doesn't use the rake tasks but calls puppet-lint directly:

pdk validate
...
pdk (DEBUG): Executing '/Users/*/Development/*/control-repo/site/profile/bin/puppet-lint --json --relative ...

Now if you've a line longer than 140 chars, puppetlabs_spec_helper/the rake task accepts it, pdk doesn't. This leads to inconsistencies.

Because of that I think pdk should reuse the existing tasks instead of reinventing the logic.

[rwaffen]

pdk version info

brew info --cask puppetlabs/puppet/pdk

==> pdk: 3.0.1.3
https://github.com/puppetlabs/pdk
Installed
/opt/homebrew/Caskroom/pdk/3.0.1.3 (123.4MB)
From: https://github.com/puppetlabs/homebrew-puppet/blob/HEAD/Casks/pdk.rb
==> Name
Puppet Development Kit
==> Description
None
==> Artifacts
pdk-3.0.1.3-1-installer.pkg (Pkg)
==> Caveats

    PDK binaries are installed in /opt/puppetlabs/pdk/bin, which is sourced by an /etc/paths.d entry.
    /opt/puppetlabs/pdk/bin may not be included in your current $PATH but should be included in new shells.

[bastelfreak]

@gavindidrichsen @LukasAud @davidsandilands do you have an update here? This came up at a PE customer today. there are some checks in puppet-syntax that would have catched a customer problem, but since pdk doesn't use that, pdk didn't notice it.

[david22swan]

Ah no, it's set to private
Edit: internal is probably a better word, don't think it's possible to make it public.

[davidsandilands]

@bastelfreak I'm afraid this come back to the Jira only internal problem and associated costs / policy if it was made public. I promise we are continuing to come up with a community policy/approach to better update in tickets/issues

[david22swan]

@bastelfreak
Hey just as an update to this, I did an initial attempt at this and found an issue with the output of puppet-syntax, in that while it was user friendly and easy to understand from a usage point, it was not conducive to being consumed and parsed by the PDK, with the .erb template checks being especially difficult from this perspective.
As such if we want to move forward with this we would likely need to add an option to the puppet-syntax to format the output of the validators to be more easily consumed, i.e. how puppet-lint can be output as JSON.
Given that the puppet-syntax is under Vox ownership however I wanted to first reach out and ensure that there would be no issues with this.

[bastelfreak]

Hi, I don't see an issue with that feature in puppet-syntax. That would also be a good step to code annotations in GitHub CI.

[ekohl]

I think parsing human readable output in scripts is always a bad idea so big 👍 for defining a machine readable output mode in puppet-syntax. Then I have a strong preference to reuse an existing output format. I know someone contributed SARIF output to puppet-lint (which GitHub can parse) and I suggest to start investigating that, but also look at how various language servers (for editors) implement syntax checking.