Refactor identity show command to use rpc abstraction and to stop mapping errors manually

[adrianbenavides]

Command: identity show

Current

The current implementation is calling std::process::exit, which is something we want to stop using to handle errors properly and give the user a better-formatted output when a command fails.

It's also using the connect_to function, which can be simplified by using the Rpc utils. Check out how this is done in the subscription commands, for example.

Desired

• run method (main entry point of the subcommand) should contain a single line of code calling a function (run_impl/rpc/...) that returns a crate::Result<()>
• std::process::exit should not be used manually to map the errors of the command's logic
• run method will return (). The error will be internally handled within the node_rpc function
• If you need to replace an std::process::exit by an actual error, you can use something like Err(crate::error::Error::new(exitcode::..., anyhow!(...))
• Use rpc abstraction instead of connect_to

You can take the subscription commands as a reference.

---

We love helping new contributors!
If you have questions or need help as you work on your first Ockam contribution, please leave a comment on this discussion.
If you're looking for other issues to contribute to, checkout this discussion and labels - good first issue or help wanted

[divyank-aggarwal]

Hi, I'd like to contribute to this project.

I have a few queries related to this issue:

1. Is there a test I can run to see if I've refactored the code correctly?
2. I'm assuming the old show_identity function that is in use should now be deprecated (and hence deleted)
3. There is an error that was not directly convertible to the crate::Result error type, namely api::show_identity().to_vec()
   For this I'm using Err(crate::error::Error::new(exitcode::DATAERR, anyhow!("An error occurred while converting short identity to vector"))
   Is this ok?

Thank you for the reply in advance, really appreciate the help.

[adrianbenavides]

Hey @divyank-rs , thanks for showing your interest in the project. Let me answer your questions:

Is there a test I can run to see if I've refactored the code correctly?

We currently have some tests in place in the tests/commands.bats file. We don't have an explicit test for this but this command is being tested indirectly by the "inlet/outlet example with credentials, not provided" test, for example.

You could write a new test for this command checking that it runs successfully. You could also check that the first character of the output equals the IdentityId prefix P.

I'm assuming the old show_identity function that is in use should now be deprecated (and hence deleted)

Yeah, you can create the requests directly as in the subscription command linked in the description.

There is an error that was not directly convertible to the crate::Result error type

Don't worry about this. Once you have the rpc in place, you won't need to convert the request into a vector.

---

Let me know if you need anything else!

[divyank-aggarwal]

Hey @adrianbenavides ,

Your advice was really helpful and I've written the code. However upon running the tests, half of them including the inlet/outlet example with credentials, not provided test requires ockam enroll to have already occurred. What is the process through which I can run these tests?

[adrianbenavides]

You don't need to run those, as they test cloud-related functionalities. You can skip them by setting the ORCHESTRATOR_TESTS env variable. Likewise, you'll want to set the LONG_TESTS env variable:

$ export ORCHESTRATOR_TESTS=1
$ export LONG_TESTS=1

Back to this PR, the identity show command only needs a local node to work, so the basic setup would be to create one with the ockam node create command. Try this first on your terminal to see how it works, it will probably help you understand what to expect:

$ ockam node delete -af # To delete all your running nodes and start clean
$ ockam node create n1
$ ockam identity show --node n1

[divyank-aggarwal]

Hi @adrianbenavides ,

Thanks a lot for your help. I managed to test it locally by writing a short test in the bats file and it is working as expected.

1. I've added myself as a new contributor here
2. I've created a PR for this issue here

Please let me know if something more is needed from my side. Looking forward to contributing more to the project!

[divyank-aggarwal]

Hey @adrianbenavides,

I've deleted the unused methods as you requested and here is the test I used to verify the identity show command

@test "test the identity show command" {

  run $OCKAM node create n1 

  assert_success

  run $OCKAM identity show --node n1

  assert_success

  assert_output --regexp '^P'
}