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

ntp client #138

Merged
merged 10 commits into from
Jun 14, 2022
Merged

ntp client #138

merged 10 commits into from
Jun 14, 2022

Conversation

folkertdev
Copy link
Contributor

a very rough draft for the client (but everything seems to work)

@folkertdev folkertdev changed the base branch from main to observe-peer-state June 7, 2022 15:43
Base automatically changed from observe-peer-state to main June 9, 2022 11:46
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #138 (bf40d02) into main (2506f01) will decrease coverage by 1.30%.
The diff coverage is 2.66%.

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
- Coverage   80.53%   79.22%   -1.31%     
==========================================
  Files          20       22       +2     
  Lines        4223     4294      +71     
==========================================
+ Hits         3401     3402       +1     
- Misses        822      892      +70     
Impacted Files Coverage Δ
ntp-daemon/src/config/dynamic.rs 0.00% <0.00%> (ø)
ntp-daemon/src/main.rs 3.44% <0.00%> (-0.40%) ⬇️
ntp-daemon/src/observer.rs 0.00% <ø> (ø)
ntp-daemon/src/tracing.rs 0.00% <0.00%> (ø)
ntp-client/src/main.rs 2.85% <2.85%> (ø)
ntp-daemon/src/config/mod.rs 74.54% <100.00%> (ø)
ntp-proto/src/config.rs 97.77% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2506f01...bf40d02. Read the comment docs.

Copy link
Member

@davidv1992 davidv1992 left a comment

Choose a reason for hiding this comment

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

Mostly minor comments, though the licensing issue will need fixing.

Other than those, this may also be the time to move the client out of the test-binaries into its own crate. What is your view on that?

test-binaries/src/bin/client.rs Outdated Show resolved Hide resolved
test-binaries/src/bin/client.rs Outdated Show resolved Hide resolved
ntp-daemon/src/tracing.rs Show resolved Hide resolved
test-binaries/src/bin/client.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidv1992 davidv1992 left a comment

Choose a reason for hiding this comment

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

Looks good, nice set of changes

@davidv1992 davidv1992 merged commit a869aba into main Jun 14, 2022
@davidv1992 davidv1992 deleted the ntp-client branch June 14, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants