-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: ilp-cli tool for interacting with nodes #334
Conversation
examples/simple/README.md
Outdated
test_equals_or_exit '{"balance":"500"}' test_http_response_body -H "Authorization: Bearer admin-a" http://localhost:7770/accounts/node_b/balance | ||
test_equals_or_exit '{"balance":"-500"}' test_http_response_body -H "Authorization: Bearer admin-b" http://localhost:8770/accounts/node_a/balance | ||
test_equals_or_exit '{"balance":"500"}' test_http_response_body -H "Authorization: Bearer admin-b" http://localhost:8770/accounts/bob/balance | ||
test_equals_or_exit '{"balance":"-500"}' test_http_response_body -H "Authorization: Bearer admin-example" http://localhost:7770/accounts/alice/balance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd prefer that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmm difficult question.
This simply checks, finally it worked well or not. So using cli
here means mixing adulterants in. That said, actually now this is working as an integration test, using cli
makes it possible to test cli
itself. It might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like the idea of using this to test the CLI as well; since this is a monorepo I think it's reasonable to make the CI test the examples and the CLI at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few comments / changes above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left some comments.
If we use cli
in examples, we have to amend the other examples too.
I'd be happy to take it on if you want.
examples/simple/README.md
Outdated
test_equals_or_exit '{"balance":"500"}' test_http_response_body -H "Authorization: Bearer admin-a" http://localhost:7770/accounts/node_b/balance | ||
test_equals_or_exit '{"balance":"-500"}' test_http_response_body -H "Authorization: Bearer admin-b" http://localhost:8770/accounts/node_a/balance | ||
test_equals_or_exit '{"balance":"500"}' test_http_response_body -H "Authorization: Bearer admin-b" http://localhost:8770/accounts/bob/balance | ||
test_equals_or_exit '{"balance":"-500"}' test_http_response_body -H "Authorization: Bearer admin-example" http://localhost:7770/accounts/alice/balance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmm difficult question.
This simply checks, finally it worked well or not. So using cli
here means mixing adulterants in. That said, actually now this is working as an integration test, using cli
makes it possible to test cli
itself. It might be better.
Also note that this initial PR doesn't yet update any of the Docker side of things; wasn't sure if that ought to be left to a future PR. |
@dora-gt, I'm actually using the examples as my to-do list for which APIs to implement first, so no need. :) However, I don't intend on updating the docker side of the examples right now, and I'd be happy to have you work on that (but don't worry about doing that until after I have the examples updated). |
I've just force-pushed a rebased and squashed version with all changes since the last review. Things not yet done:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments but overall looking good
@@ -283,111 +288,72 @@ else | |||
--> | |||
|
|||
```bash | |||
# We merely use this symbolic link here to pretend that ilp-cli is installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# We merely use this symbolic link here to pretend that ilp-cli is installed | |
# We merely use this symbolic link here to pretend that ilp-cli is installed | |
# You would not need this line if you installed `ilp-cli` by running `cargo install ilp-cli` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with suggesting that people use cargo install
is that I worry about a situation where someone has a stale version of the CLI installed which might end up causing errors that are frustrating for users to diagnose down the road. Let me know if you think the risk here isn't worth worrying about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not worth it in this case. If the CLI isn't stable right now, it should be very soon
@@ -385,77 +389,60 @@ else | |||
--> | |||
|
|||
```bash | |||
ln -s ../../target/debug/ilp-cli ilp-cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't do this if it doesn't work when run from a different directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the preferred alternative is; this is replacing the hideous cargo run --quiet --bin ilp-cli --
that otherwise preceded every command and served to obfuscate what the example was trying to show (and is annoyingly slower in practice, since every individual cargo run
has to check to see if a recompilation should occur). We could cargo install
perhaps, but that seems like a more heavyweight solution and I fear that we're going to have users accidentally managing to run old versions of the CLI and running into maddening errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing the longer command sounds good as long as it works when called from different directories. I don't actually know if that link is relative to the file it's being called in or from the working directory. If it's the working directory, is there a way to force it to be from the perspective of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, ack from me to proceed with this CLI design, its use is intuitive to me.
crates/ilp-cli/src/interpreter.rs
Outdated
fn put_rates(&self, matches: &ArgMatches) -> reqwest::Result<Response> { | ||
unimplemented!() | ||
fn put_rates(&self, matches: &ArgMatches) -> Result<Response, Error> { | ||
let auth = matches.value_of("authorization_key").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this fails will it show that the authorization key was not found? Maybe replace the unwrap with a more verbose expect message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should sprinkle some more comments around here, basically the gist of this file is that anywhere you see an unwrap
it's "impossible" to actually trigger a panic because if the field were omitted then Clap would have already errored out during the get_matches
stage (I say "impossible" in quotes because there could always be a bug in Clap or something).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear here's what the message looks like today if this field is omitted:
⌁ ./ilp-cli rates set-all -r a 1.0
error: The following required arguments were not provided:
--auth <authorization_key>
USAGE:
ilp-cli rates set-all --auth <authorization_key> --rate <rate> <rate>
For more information try --help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments but overall looking good
refactor(ilp-cli): address low-hanging review comments docs: make nodes in simple example use different passwords refactor(ilp-cli): port simple example to new CLI interface feat(ilp-cli): skeleton of entire HTTP API refactor(ilp-cli): split out all http calls in the interpreter docs: update simple example for ilp-cli docs: update xrp example for ilp-cli
This PR adds a new command-line tool,
ilp-cli
. It's a beginner-friendly interface for interacting with a running interledger node. The intent is forilp-cli
to be forinterledger
whatredis-cli
is forredis-server
. Right now it only includes as much functionality as is necessary (creating new accounts, posting payments, and getting account balances) to port the simple example fromcurl
toilp-cli
. Supporting the remainder of the HTTP API will arrive in future PRs.