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

Client pool state machine #1230

Merged
merged 24 commits into from
Dec 7, 2023
Merged

Client pool state machine #1230

merged 24 commits into from
Dec 7, 2023

Conversation

folkertdev
Copy link
Contributor

No description provided.

ntp-proto/Cargo.toml Outdated Show resolved Hide resolved
ntpd/src/daemon/spawn/nts.rs Outdated Show resolved Hide resolved
nts-pool-ke/src/lib.rs Show resolved Hide resolved
nts-pool-ke/src/lib.rs Outdated Show resolved Hide resolved
nts-pool-ke/src/lib.rs Show resolved Hide resolved
@folkertdev folkertdev added the feedback wanted Needs input from other people label Nov 26, 2023
@folkertdev folkertdev force-pushed the unrecognized-critical-message branch from 0b3884f to d8f3a3e Compare November 27, 2023 15:39
Base automatically changed from unrecognized-critical-message to main November 30, 2023 10:35
@folkertdev folkertdev force-pushed the client-pool-state-machine branch from 9e8c559 to 3a27072 Compare November 30, 2023 15:58
@folkertdev folkertdev changed the base branch from main to check-next-protocol-record November 30, 2023 15:59
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 412 lines in your changes are missing coverage. Please review.

Comparison is base (7633929) 84.63% compared to head (76786f3) 82.85%.

Files Patch % Lines
ntp-proto/src/nts_pool_ke.rs 0.00% 219 Missing ⚠️
nts-pool-ke/src/lib.rs 0.00% 179 Missing ⚠️
ntp-proto/src/nts_record.rs 61.11% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1230      +/-   ##
==========================================
- Coverage   84.63%   82.85%   -1.79%     
==========================================
  Files          64       65       +1     
  Lines       18446    18847     +401     
==========================================
+ Hits        15612    15615       +3     
- Misses       2834     3232     +398     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from check-next-protocol-record to main December 7, 2023 09:18
@squell squell force-pushed the client-pool-state-machine branch from 3a27072 to ae74b60 Compare December 7, 2023 09:22
Copy link
Member

@squell squell left a comment

Choose a reason for hiding this comment

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

Of course the pool-related stuff should be hidden behind feauture flags.

I think (for maintainability) that it is better if the NTS-POOL-KE related decoders are spun off in their own file (nts_record_extensions.rs or nts_pool_record.rs or some such?). That would also cut down on the amount of #[cfg(feature = "nts-pool")] that is needed.

The changes that are not related to the pool-ke look pretty innocuous to me.

@folkertdev folkertdev marked this pull request as ready for review December 7, 2023 11:02
@folkertdev folkertdev added waiting for review and removed feedback wanted Needs input from other people labels Dec 7, 2023
ntp-proto/src/nts_record.rs Outdated Show resolved Hide resolved
ntp-proto/src/nts_record.rs Outdated Show resolved Hide resolved
ntpd/Cargo.toml Outdated Show resolved Hide resolved
@folkertdev folkertdev force-pushed the client-pool-state-machine branch from febc345 to 76786f3 Compare December 7, 2023 13:31
@folkertdev folkertdev requested a review from squell December 7, 2023 13:35
@folkertdev folkertdev added this pull request to the merge queue Dec 7, 2023
Merged via the queue into main with commit a6fbc9f Dec 7, 2023
20 checks passed
@folkertdev folkertdev deleted the client-pool-state-machine branch December 7, 2023 14:00
@cikzh cikzh added this to the nts-pool milestone Dec 18, 2023
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.

3 participants