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

Cli integration #380

Closed
wants to merge 19 commits into from
Closed

Cli integration #380

wants to merge 19 commits into from

Conversation

petarjuki7
Copy link
Member

@petarjuki7 petarjuki7 commented Jun 17, 2024

CLI short description

A new CLI to use for setting up and running nodes.

Summary

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (Add Jira tickets related to this change)

Test plan

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration. Add screenshots or videos for changes in the user-interface.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added a test plan that prove my fix is effective or that my feature works
  • I squashed all commits and provided a meaningful commit message
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • New Features
    • Added a new cli module with various commands (InitCommand, LinkCommand, RunCommand, SetupCommand) for node configuration and management.
  • Refactor
    • Updated command structures and configurations in the cli module for improved functionality.
    • Renamed and adjusted constants, introduced new structs, traits, and functionalities in the config.rs file.
    • Reorganized and introduced a new module named config in the main.rs file.
  • Chores
    • Removed the config module from the public scope in the lib.rs file.

Copy link

coderabbitai bot commented Jun 17, 2024

Walkthrough

The project underwent significant restructuring with the introduction of new CLI modules cli, init, link, run, and setup. Changes involve refactoring command structures, configuration handling, and module organization. Notable updates include transitioning from #[clap] annotations to #[command], introducing new command functionalities, and enhancing configuration management.

Changes

Files Change Summaries
Cargo.toml Added ./crates/cli module
crates/cli/src/cli.rs Updated command structure using #[command], added SetupCommand
crates/cli/src/cli/init.rs Introduced InitCommand for node configuration initialization
crates/cli/src/cli/link.rs Added LinkCommand for symlink setup and configurations
crates/cli/src/cli/run.rs Modified RunCommand with node_name field for configuration loading
crates/cli/src/cli/setup.rs Defined SetupCommand for comprehensive node configuration setup
crates/cli/src/config.rs Renamed constants, introduced InitFile struct, implemented ConfigImpl trait
crates/cli/src/main.rs Added warnings for unused crates, introduced config module
crates/node/src/lib.rs Removed config module from public scope

Poem

In code's embrace, changes bloom,
Rabbit scurries, in the CLI room.
Commands revamped, configurations anew,
A dance of code, a whimsical brew.
The path unfolds, in lines and rhyme,
CodeRabbit hops, in binary chime.


Note

Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://coderabbit.ai

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

crates/cli/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +25 to +33
calimero-node = { path = "../node" }
calimero-network = { path = "../network" }
calimero-application = { path = "../application" }
calimero-node-primitives = { path = "../node-primitives" }
calimero-primitives = { path = "../primitives" }
calimero-server = { path = "../server", features = ["jsonrpc", "websocket", "admin"] }
calimero-store = { path = "../store" }
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't depend on all these crates IMO. CLI should be as small as possible and know as little as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of them are unneeded probably, will clean up.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I'm constructing the config struct to run the node (NodeConfig), I am using other structs from these crates.

Is there a way to get around that?

}

let config = ConfigFile::load(&root_args.home)?;
let config = ConfigFile::load(&path)?;

calimero_node::start(calimero_node::NodeConfig {
Copy link
Member

Choose a reason for hiding this comment

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

CLI should start a node binary, not invoke the start method.

Copy link
Member

Choose a reason for hiding this comment

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

Also CLI should keep track of all started binaries, so the user can stop/interact with any of the running nodes.


/// Force initialization even if the directory already exists
#[clap(short, long)]
pub force: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Why would the user want to init node again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Someone mentioned that it would be nice to restart the node, so Xabi mentioned that deleting the keys would block users to rejoin the context. So I kept it like this.
Can remove if needed

@petarjuki7 petarjuki7 force-pushed the petarjuki7/cli branch 2 times, most recently from dad0c20 to 8548123 Compare June 18, 2024 15:12
Comment on lines +14 to +16
/// Name of node
#[arg(short, long, value_name = "NAME")]
pub node_name: camino::Utf8PathBuf,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this also need to be part of the RootArgs to prevent duplication?

Comment on lines +60 to +63
pub struct InitFile {
#[serde(with = "calimero_primitives::identity::serde_identity")]
pub identity: identity::Keypair,
}
Copy link
Member

Choose a reason for hiding this comment

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

this seems mostly useless imo

  1. init will happen once, in the lifetime of a node
  2. this will be a file with one thing.. identity
  3. if you want to define your own custom keypair, then init with the random one and change it

not convinced we need a special file just for initialization

and if we need to provide some way to dynamically rotate keys, consider how IPFS's kubo has it implemented

$ jq '.Identity' ~/.ipfs/config
{
  "PeerID": "12D3KooWLJmoNJAbxeMW6uX7VPAmNSEUhXzNmxyk1DbFvFkS9oVB",
  "PrivKey": "CAESQFPkkrc3fZvD2FQC0ek4QyAAI/v20C1AeSdCUdNxp1lnm9vaIWptv079wdl8tYgDzMWmGr6rdXpeiKlDEm4Kooo="
}

$ ipfs key rotate
generating ED25519 keypair...done
peer identity: 12D3KooWQ1WCd1To5qN7QYxjYbawPuEfqQaejkRF7kskATmsw2Z8

$ jq '.Identity' ~/.ipfs/config
{
  "PeerID": "12D3KooWQ1WCd1To5qN7QYxjYbawPuEfqQaejkRF7kskATmsw2Z8",
  "PrivKey": "CAESQG4zbb2YW2chk+NgNu0Ag0CdzG76V5qAeh3NuaN6jtWk0t4WjUgdE3nAhYRF+TH0IkZquffMtOL/hqkxGqAouXs="
}

pub home: camino::Utf8PathBuf,
}

impl RootCommand {
pub async fn run(self) -> eyre::Result<()> {
let _c = RootCommand::parse();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let _c = RootCommand::parse();

pub version: String,
}

fn validate_version(v: &str) -> Result<String, String> {
Copy link
Member

Choose a reason for hiding this comment

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

semver::Version didn't work?


/// Setup symlink to application in the node
#[derive(Debug, Parser)]
pub struct LinkCommand {
Copy link
Member

Choose a reason for hiding this comment

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

This should be simple and we want to override the application for the context, not the application itself:

And I believe it makes sense to scope things out under an initial context subcommand:

$ calimero context <context-id> install <resource>

where

resource := (application-spec[@version]) {OR} (file-path)
application-spec := (scoped-spec) {OR} (unique-spec)
scoped-spec := (<user-name>/<package-name>)
unique-spec := (bs58-encoded-unique-32-byte-application-id)
file-path := (relative-path) {OR} (absolute-path)

for example:

  • Download the latest version of the application from the registry

     $ calimero context 33ovekjBBk2GxfZUueMPKMUe5ofqfp2AoEcXpAM71aAQ install petar/only-peers
  • Download version 0.2.0 of the application from the registry

     $ calimero context 33ovekjBBk2GxfZUueMPKMUe5ofqfp2AoEcXpAM71aAQ install petar/[email protected]
  • Download the latest version of the application from the registry

     $ calimero context 33ovekjBBk2GxfZUueMPKMUe5ofqfp2AoEcXpAM71aAQ install 4TdrU3ruw6VquHforZ1ojjQ46dmnRwSP3faKB8evjviB
  • Download version 0.3.4 of the application from the registry

     $ calimero context 33ovekjBBk2GxfZUueMPKMUe5ofqfp2AoEcXpAM71aAQ install [email protected]
  • Installs the specified wasm file (copied to the apps folder)

     $ calimero context 33ovekjBBk2GxfZUueMPKMUe5ofqfp2AoEcXpAM71aAQ install ./path/to/binary.wasm
  • Installs the specified wasm file (linked into the apps folder)

     $ calimero context 33ovekjBBk2GxfZUueMPKMUe5ofqfp2AoEcXpAM71aAQ install --link ./path/to/binary.wasm

Comment on lines +71 to +74
match symlink(self.path, app_path.join("binary.wasm")) {
Ok(_) => {}
Err(err) => eyre::bail!("Symlinking failed: {}", err),
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match symlink(self.path, app_path.join("binary.wasm")) {
Ok(_) => {}
Err(err) => eyre::bail!("Symlinking failed: {}", err),
}
if let Err(err) = symlink(self.path, app_path.join("binary.wasm")) {
eyre::bail!("Symlinking failed: {}", err);
}

self.node_name,
app_path.join("binary.wasm")
);
return Ok(());
Copy link
Member

Choose a reason for hiding this comment

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

move this to the end of the function

Comment on lines +87 to +89
} else {
eyre::bail!("You have to initialize the node first \nRun command node init -n <NAME>");
}
Copy link
Member

Choose a reason for hiding this comment

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

move this to the top

if !ConfigFile::exists(..) {
    eyre::bail!(..);
}

// at this point the file exists

should help flatten out the indentation

Comment on lines +208 to +210
calimero_store::Store::open(&calimero_store::config::StoreConfig {
path: path.join(config_new.store.path),
})?;
Copy link
Member

Choose a reason for hiding this comment

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

the db should've been initialized on init


/// Initialize node configuration
#[derive(Debug, Parser)]
pub struct SetupCommand {
Copy link
Member

Choose a reason for hiding this comment

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

this pattern was supposed to go away, since we can't exhaustively define all the options

// todo! simplify this, by splitting the steps
// todo! $ calimero node init
// todo! $ calimero node config 'swarm.listen:=["", ""]' discovery.mdns:=false
// todo! $ calimero node config discovery.mdns

IPFS's kubo uses this design, we can learn from it

@petarjuki7 petarjuki7 closed this Jun 24, 2024
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.

6 participants