voxpupuli / puppet-chrony

Puppet module for Chrony with Systemd
https://forge.puppet.com/puppet/chrony
Apache License 2.0
13 stars 59 forks source link

init: change `refclocks` data type to `Hash` #190

Open bmagistro opened 10 months ago

bmagistro commented 10 months ago

This updates the refclock parameter data types to be a hash which matches the examples and updates the template to properly generate multiple refclock entries.

This looks to have broken via a combination of #141 and #79. Rather than restore the full variations that were supported prior to #79 a simplified hash structure was chosen.

Fixes #189

bmagistro commented 10 months ago

need guidance on how to regenerate ereference.md

benjunmun commented 8 months ago

need guidance on how to regenerate ereference.md

So I'm not a maintainer, but I believe the right thing to use is puppet strings. Specifically puppet strings generate --format markdown

smortex commented 8 months ago

Update REFERENCE.md with

$ bundle exec rake strings:generate:reference

Add it to the PR and CI should continue.

Can you also update the test suite (in spec/classes/chrony_spec.rb) to make sure this work as expected and we don't break it again in the future?

kenyon commented 8 months ago

I wonder if this is the change we should make. The refclocks parameter has always been an array since it was added in #17. We could just update the documentation to avoid the breaking change in data type.

Here is how I am configuring refclocks, for example:

smortex commented 8 months ago

Following on @kenyon notes, and as someone who never used this module, the parameter looks a bit scary. My guess is that a proper usage of Puppet's Struct/Tuple parameter may make it easier to understand what is expected?

Switching to Struct is necessarily a breaking change, but with a Variant of String and Tuple, I think we can have a backwards-compatible way of making the data validated (untested):

Array[
  Variant[
    String[1], # for backwards compatibility
    Tuple[
      Enum['PHC', 'PPS', 'SOCK'],
      Stdlib::Absolutepath,
      Pattern['\A[^ ]+ [^ ]+\z'],
      2,
      default,
    ],
    Tuple[
      Enum['SHM'],
      Integer[0],
      Pattern['\A[^ ]+ [^ ]+\z'],
      2,
      default,
    ],
  ],
]

No strong opinion regarding the above, but it feels cheaper to fix the doc so that it match the code than adjusting the code so that it match the doc.

bmagistro commented 8 months ago

I can try reverting my changes and see if it generates correctly. I vaguely remember trying an array at one point and not being able to get the multiple entries I needed.