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

Add new driftfile, hwtimestamps, rtcsync, and dumpdir parameters #82

Merged
merged 5 commits into from
Jul 11, 2020

Conversation

chrekh
Copy link
Contributor

@chrekh chrekh commented Jul 10, 2020

The params added with this PR are:
driftfile, hwtimestamps, rtcsync, and dumpdir. Each with it's separate commit.

@chrekh
Copy link
Contributor Author

chrekh commented Jul 10, 2020

Rebased to master/HEAD because the need to add defaultvalue for $dumpdir for Gentoo in params.pp

REFERENCE.md Outdated

Data type: Boolean

Periodically sync syste time to RTC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in system

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. Do you prefer that I add a small typo-fix commit, or a rebase -i and a force-push?

REFERENCE.md Outdated
Data type: `Variant[Hash,Array[String]]`

This selects interfaces to enable hardware timestamps on. It can be an array of interfaces
or a hash of interfaces to ther respective options.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here too. The code looks correct though. I guess REFERENCE.MD just needs regenerating?

@@ -120,3 +128,12 @@ smoothtime <%= $chrony::smoothtime %>
# https://chrony.tuxfamily.org/doc/3.4/chrony.conf.html#rtconutc
rtconutc
<% } -%>
<% if ! $chrony::hwtimestamps.empty { -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would unless be more readable than if !?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that you should mention that. I myself prefer unless before if ! in any language that supports it. But, I know people that hates unless. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more 'if !' in the template, I can add a separate PR later that changes them all to unless, and se if someone else objects.

@alexjfisher
Copy link
Member

Would you be able to add any extra tests to cover the new options? In particular, it'd be good to test that hwtimestamps works correctly with both Hash and Array values.

@alexjfisher alexjfisher changed the title Add some more params. Add new driftfile, hwtimestamps, rtcsync, and dumpdir parameters Jul 11, 2020
@alexjfisher alexjfisher added the enhancement New feature or request label Jul 11, 2020
@chrekh
Copy link
Contributor Author

chrekh commented Jul 11, 2020

Would you be able to add any extra tests to cover the new options? In particular, it'd be good to test that hwtimestamps works correctly with both Hash and Array values.

Yes, I can do that.

chrekh added 5 commits July 11, 2020 23:43
To avoid having it hardcoded by the template.
To avoid having it hardcoded by the template.
To be able to specify directory for chrony to save measurement in on exit. When dumpdir is
set, both option dumponexit and dumpdir is added to chrony.conf

dumponexit and dumpdir was before b971db0 hardcoded in
the template for archlinux.  This commit is restoring the default value for Archlinux, and
leaving the default unset on other os.
@chrekh
Copy link
Contributor Author

chrekh commented Jul 11, 2020

I have rebased to master, fixed the typos, and added tests in the commits for each param

Copy link
Contributor

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrekh nice work!

@alexjfisher alexjfisher merged commit 60a7be8 into voxpupuli:master Jul 11, 2020
@chrekh chrekh deleted the newparams branch July 12, 2020 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants