Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Add config command #294

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add config command #294

wants to merge 21 commits into from

Conversation

abhi3700
Copy link
Contributor

@abhi3700 abhi3700 commented Jan 29, 2024

Description

This PR adds config command to the CLI tool that would allow user to change (if they wish for) the set params (during init command) like:

  • show already set config
  • switch chain
  • set farm-size
  • set reward-address
  • set node directory
  • set farm directory
  • set all params together

This also covers the issue: #248 (comment)

@abhi3700 abhi3700 added the enhancement New feature or request label Jan 29, 2024
@abhi3700 abhi3700 self-assigned this Jan 29, 2024
@abhi3700 abhi3700 linked an issue Jan 30, 2024 that may be closed by this pull request
1 task
- wrapped config options with 'Option' type
- show config is working fine!
- added fn in utils
- prettify the config details
- removed redundant check for MIN_FARM_SIZE
- added config command in arrow_key_mode
- added usages except node, farm directory
- removed small fn from utils & used the code directly into placeholders
- corrected the order of commands in interactive mode
- thorough manual testing of `create_or_move_data` fn was done
- added usage for `node-dir` & `farm-dir` params & all params at a time.
@abhi3700 abhi3700 requested review from ParthDesai and nazar-pc and removed request for nazar-pc and ParthDesai January 31, 2024 18:04
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Please update against latest main


// update (optional) the chain
if let Some(c) = chain {
config.chain = ChainConfig::from_str(&c)?;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check if the chain has changed before wiping it?

Copy link
Contributor Author

@abhi3700 abhi3700 Jan 31, 2024

Choose a reason for hiding this comment

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

Although redundant.
Added here in this commit.

Additionally, added check for parsing already set chain.

@nazar-pc nazar-pc requested a review from ParthDesai January 31, 2024 18:11
@abhi3700 abhi3700 closed this Jan 31, 2024
@abhi3700 abhi3700 deleted the add-config-cmd branch January 31, 2024 19:08
@abhi3700 abhi3700 restored the add-config-cmd branch January 31, 2024 19:09
…nfig-cmd

# Conflicts:
#	Cargo.toml
#	pulsar/src/commands/config.rs
@abhi3700 abhi3700 reopened this Jan 31, 2024
@abhi3700 abhi3700 marked this pull request as ready for review January 31, 2024 19:29
@abhi3700 abhi3700 requested a review from nazar-pc January 31, 2024 20:38
pulsar/src/utils.rs Outdated Show resolved Hide resolved
pulsar/src/utils.rs Outdated Show resolved Hide resolved
pulsar/src/main.rs Show resolved Hide resolved
pulsar/src/commands/config.rs Outdated Show resolved Hide resolved
@abhi3700 abhi3700 requested a review from ParthDesai February 6, 2024 13:07
@@ -175,3 +177,48 @@ fn custom_log_dir_test() {
#[cfg(target_os = "windows")]
assert!(log_path.ends_with("AppData/Local/pulsar/logs"));
}

#[cfg(test)]
mod create_or_move_data_tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just have mod test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the aim is to keep the related tests under one mod label.
Let me know your thoughts.


/// Ensuring the function correctly handles the case where the old and new
/// directories are the same.
#[test]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to test a scenario where we have actual files in old directory which we move into new and delete the old.

for entry in entries {
let entry = entry?;
let file_name = entry.file_name();
fs::rename(entry.path(), new_dir.join(file_name))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if this fails? We are left with some data in old directory and some data in new one. Which means after this happen, user won't be able to use pulsar and have to fix the situation by hand.

I would like to have atomic move implemented here. Either we move everything or we move nothing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

🎁 [Feature Request]: Add support for specifying a config directory
3 participants