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

fix(config): Duration fields #4587

Merged
merged 4 commits into from
Jun 14, 2022
Merged

fix(config): Duration fields #4587

merged 4 commits into from
Jun 14, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We have a few fields of the Zebra config that are of type Duration. This are at the moment network.crawl_new_peer_interval and eviction_memory_time.

This fields are displayed as follows in the configuration file:

[mempool.eviction_memory_time]
nanos = 0
secs = 3600

We want a more user friendly way for this entries of the config. Close #2847

Solution

Added humantime-serde to this fields as suggested in the ticket as it seems the best option. Now the fields with the problem will look as:

eviction_memory_time = '1h'

Which is a lot better. The change also allowed to move the debug fields to the end of the config section.

Review

I think anyone can review this.

@oxarbitrage oxarbitrage requested review from a team as code owners June 9, 2022 18:09
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team June 9, 2022 18:09
@oxarbitrage oxarbitrage changed the title Issue2847 fix(config): Duration fields Jun 9, 2022
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #4587 (5c3ad75) into main (2e50ccc) will decrease coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4587      +/-   ##
==========================================
- Coverage   78.84%   78.73%   -0.11%     
==========================================
  Files         304      304              
  Lines       37318    37318              
==========================================
- Hits        29423    29382      -41     
- Misses       7895     7936      +41     

Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks good.

@teor2345
Copy link
Collaborator

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 14, 2022

update

✅ Branch has been successfully updated

mergify bot added a commit that referenced this pull request Jun 14, 2022
@mergify mergify bot merged commit 6d9bb22 into main Jun 14, 2022
@mergify mergify bot deleted the issue2847 branch June 14, 2022 06:21
upbqdn pushed a commit that referenced this pull request Jun 14, 2022
* use `humantime_serde` for config durations

* move debug config option to the bottom

* fix deserialization

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

Make config Duration easier to write in the toml file
3 participants