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

config dump with tests #1485

Merged
merged 1 commit into from
Mar 18, 2022
Merged

config dump with tests #1485

merged 1 commit into from
Mar 18, 2022

Conversation

LesnyRumcajs
Copy link
Member

@LesnyRumcajs LesnyRumcajs commented Mar 18, 2022

Summary of changes
Changes introduced in this pull request:

  • added config dump subcommand which should dump the current configuration to stdout in a TOML format.
  • added basic CLI tests
  • fixed a minor bug with token used via CLI

Reference issue to close (if applicable)

Closes #1478

Other information and links

@LesnyRumcajs LesnyRumcajs force-pushed the dump-config-subcommand branch from cae74c1 to d4ce645 Compare March 18, 2022 17:22
@elmattic
Copy link
Contributor

Btw, any reason you have implemented it as config dump and not just dumpconfig?

@LesnyRumcajs
Copy link
Member Author

Btw, any reason you have implemented it as config dump and not just dumpconfig?

Basically to make it more extendible from start. I'd imagine in the future we could manipulate at least part of the configuration at runtime. Plus I took some inspiration from Lotus CLI :) . Does it make sense @elmattic ?

@elmattic
Copy link
Contributor

Btw, any reason you have implemented it as config dump and not just dumpconfig?

Basically to make it more extendible from start. I'd imagine in the future we could manipulate at least part of the configuration at runtime. Plus I took some inspiration from Lotus CLI :) . Does it make sense @elmattic ?

Yes, makes lot of sense :)

@LesnyRumcajs LesnyRumcajs force-pushed the dump-config-subcommand branch from d4ce645 to 6481298 Compare March 18, 2022 17:40
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #1485 (6481298) into main (8cfdc9b) will increase coverage by 0.10%.
The diff coverage is 79.06%.

@@            Coverage Diff             @@
##             main    #1485      +/-   ##
==========================================
+ Coverage   44.19%   44.29%   +0.10%     
==========================================
  Files         321      323       +2     
  Lines       28373    28414      +41     
==========================================
+ Hits        12539    12587      +48     
+ Misses      15834    15827       -7     
Impacted Files Coverage Δ
blockchain/chain_sync/src/chain_muxer.rs 1.74% <ø> (+0.24%) ⬆️
forest/src/cli/config.rs 100.00% <ø> (+100.00%) ⬆️
forest/src/cli/mod.rs 0.00% <0.00%> (ø)
forest/src/subcommand.rs 0.00% <0.00%> (ø)
node/db/src/rocks_config.rs 100.00% <ø> (ø)
node/forest_libp2p/src/config.rs 100.00% <ø> (+100.00%) ⬆️
forest/src/cli/config_cmd.rs 64.28% <64.28%> (ø)
forest/tests/config_tests.rs 100.00% <100.00%> (ø)
vm/actor/src/builtin/miner/mod.rs 33.54% <0.00%> (+0.04%) ⬆️
... and 3 more

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 8cfdc9b...6481298. Read the comment docs.

@LesnyRumcajs LesnyRumcajs merged commit e89d883 into main Mar 18, 2022
@LesnyRumcajs LesnyRumcajs deleted the dump-config-subcommand branch March 18, 2022 18:27
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.

Add a dumpconfig subcommand
3 participants