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

init: change refclocks data type to Hash #190

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bmagistro
Copy link

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 bmagistro force-pushed the bugfix-refclock-config branch 2 times, most recently from d2613f5 to 9922326 Compare January 4, 2024 16:28
@bmagistro
Copy link
Author

need guidance on how to regenerate ereference.md

@benjunmun
Copy link

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

smortex commented Feb 17, 2024

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?

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 voxpupuli#141 and voxpupuli#79. Rather than
restore the full variations that were supported prior to voxpupuli#79 a simplified
hash structure was chosen.

Fixes voxpupuli#189

Signed-off-by: Ben Magistro <koncept1@gmail.com>
@kenyon kenyon changed the title fix refclock generation init: fix refclocks data type Feb 26, 2024
@kenyon
Copy link
Member

kenyon commented Feb 26, 2024

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:

@kenyon kenyon changed the title init: fix refclocks data type init: change refclocks data type to Hash Feb 26, 2024
@smortex
Copy link
Member

smortex commented Feb 27, 2024

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

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.

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.

Cannot configure refclocks
4 participants